Refactoring and bug fixes of KateSearchBar entering methods

Review Request #128871 - Created Sept. 9, 2016 and updated

Information
Roman Gilg
ktexteditor
master
Reviewers
kate
cullmann

The old structure for the enterIncrementalMode/PowerMode and
findNext/Previous functions in KateSearchBar was convoluted
and didn't work correctly.

Christoph and I tried to find a solution for some small problem last
month only to create further regressions because of the bad structure.
Because of this I decided to do a complete rewrite of that part.

We distinguish:
1. incremental search: incremental search mode, search bar visible
2. power search: power search mode, search bar visible
3. quick search: searches directly next/previous occurence of
selection, search bar not visible

Major goals of this refactoring were:

1.Unravel code paths when:

  • entering another mode or same one again (default: Ctrl+F / CTRL+R)
  • only searching for a new occurence of a search term (F3 / Shift+F3)

2.Fix following bugs from current master branch:

  • Cursor jumps to end of word when reentering incremental mode
  • Quick search uses old search term (without visual indication)
  • Switching from incremental to power, changing the pattern here and
    switching back again to incremental puts cursor at previous
    incremental search word's end

3.Sane behaviour on user interaction:

  • Don't set search pattern, when nothing is selected
  • Change search pattern only on reentering mode or in quick search
  • In quick search (F3, no visible search bar), always use selection
  • Jump to new occurence in actual search but not when entering mode
  • Focus on search bar when entering mdoe, but not when searching
  • Select word in doc at current/next position when entering/finding

Future Goals:

  • Integrate batched searching / highlighting
  • It probably would be best to differentiate incremental and power
    search completely by subclassing. Most methods in KateSearchBar
    are bound to the respective modes.

* Start testing of SearchBarTest *
Config: Using QtTest library 5.7.0, Qt 5.7.0 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 5.3.1 20160413)
...
...
...
Totals: 44 passed, 0 failed, 0 skipped, 0 blacklisted, 672ms
* Finished testing of SearchBarTest *

Issues

  • 2
  • 0
  • 0
  • 2
Description From Last Updated
You can take QString& or return QString Anthony Fieroni Anthony Fieroni
Don't do this. You can blockSignals auto signalsOldState = m_incUi->pattern->blockSignals(true); m_incUi->pattern->setEditText(updatedPattern); m_incUi->pattern->blockSignals(signalsOldState); Anthony Fieroni Anthony Fieroni
Anthony Fieroni

   
src/search/katesearchbar.cpp (Diff revision 1)
 
 

You can take QString& or return QString

  1. Did you mean specific this method or also enterIncrementalMode? Here I could use a reference without problem, that's right The problem in enterIncrementalMode is: In updateIncrementalPattern I want to send the QString only when calling from the enterIncrementalMode() and not from the findXXX() and I would like to handle this with a default value in defintion. But references are AFAIK not allowed to have a default value. Is there another way? I could call the function in findXXX() probably with QString() as argument and then test in the update function on this. Would this be a good way?

src/search/katesearchbar.cpp (Diff revision 1)
 
 

Don't do this. You can blockSignals
auto signalsOldState = m_incUi->pattern->blockSignals(true);
m_incUi->pattern->setEditText(updatedPattern);
m_incUi->pattern->blockSignals(signalsOldState);

  1. ... or, even better, { QSignalBlocker b(m_incUi->pattern); m_incUi->pattern->setEditText(updatedPattern); }

  2. Thank you both! I just copied this section from some other part in the file (in the enter methods). But probably this stuff wasn't available back when they wrote it, or is it something different down there? :)

Christoph Cullmann

Hi,

as you mention yourself, even the trivial change we did in the last month did cause a regression (stupid me).

Now, this patch actually makes the code even more complex than it is ATM.

I am not sure how we shall accept such changes.

IMHO it would more sense if you create a patch for one bug that is nagging you with the search which provides a minimal invasive change + a test to trigger the bug that fails now and is fixed afterwards.

  1. Hi,

    the regression was not really your fault, but it's more the overall bad structure of the enter methods in this class which caused it. There wasn't much work done in the last years on it anyway when I look at the git logs. That's why it probably needed some bigger rework. The change makes the class not really more complex I would argue. It makes the search bar work correctly on text inputs and invokation, that's why I needed more code in this regard (and it's basically fully put in the updateXXXXPattern functions, which should make it easier to read than before). Other than that it was basically a rearranging, to get the sequence right (in the enterXXXXMode methods).

    But I do understand you, that it's difficult for you guys to decide about a big code change like this from a non-maintainer. And yea, I would love to work on some small patch for one bug, that is nagging me (wouldn't be so much work aswell). The problem is only, that I need Kate and so on for my other dev stuff and it was the faulty behaviour of the searchbar, which was nagging me. ;)

    But I can assure you, that I worked hard on this patch and it should work flawlessly if you try it out in your dev setup. I would also offer you to walk you through the code in some mumble session or something.

    On the other hand, I'm working on rewriting the class completely as I mentioned above in the "Future goals", which should make the code way more clean, but it's of course an even bigger change...

  2. Hi Roman,

    it's important to chime in here: With KDE Framwroks 5.25, we already had a regression due to refactoring, trying to improve the code of the searchbar.

    With KDE Frameworks 5.26, we were not able to fix this regression in time, that is, as far as I understand, the search behavior of KTextEditor in KDE Frameworks 5.26 is not as stable as it should be. Worse, since Frameworks 5.26 will probably the 'LTS' release that distributions will pick for 18 months along with KDE Plasma 5.8, we essentially ship a KTextEditor that contains bugs that should not be in there.

    It is important to understand that refactoring just for the sake of refactoring will not help us in the short run. Given our serach behavior is not unit tested, it is highly likely regressions are introduced, which again need to be fixed in a later KDE Frameworks release. But since the KDE Frameworks are released every month, they are "supposed to be stable".

    That is: Whenever you propose a patch, you should already be really sure you don't introduce regressions. We as maintainers are also not alway capable of doing a proper review and finding all glitches.

    That said: Please first write unit tests for all the behaviors we need, that would help use much much more that just claiming that "rewriting the class completely" will help in the end. Even more important: We had some quite capable developers in the past who worked on this and brought the code into the shape we nowadays have -- yeah, it's probably not perfect, but rewriting it will eventually lead to potentially the exactly same state we have right now.

    I'm not in favour of these changes. In fact, I would even recomment to revert all the changes to the search code, since at least then we are sure we have no regressions.

    Please concentrate on unit tests, see the ktexteditor/autotests folder.

    From the mentioned reasons, this patch gets a -1, in favor of reverting the changes recently done.

    PS: I don't mean any ill here, since we depend on every single contribution we can get - but unit tests are the way to go if we want to assert future KDE Frameworks releases will not have regressions again and again.

Loading...