Refactoring and bug fixes of KateSearchBar entering methods
Review Request #128871 - Created Sept. 9, 2016 and updated
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.
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
- 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 *
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.