SearchBar: Don't block GUI when enter incremental pattern
Needs ReviewPublic

Authored by loh.tar on Feb 26 2019, 7:22 PM.

Details

Reviewers
dhaumann
cullmann
Group Reviewers
KTextEditor
Summary

When the document is very big and there is no early match while you enter the
search pattern the GUI hangs as long the search is running after each key
stroke. This patch break the document in smaller chunks to search to return fast
enough to the event loop to update user input.

  • Disable next/prev buttons when not match
  • Only save search history on some change

BUG:244424

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
loh.tar created this revision.Feb 26 2019, 7:22 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptFeb 26 2019, 7:22 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
loh.tar requested review of this revision.Feb 26 2019, 7:22 PM
loh.tar updated this revision to Diff 52669.Feb 26 2019, 8:10 PM
  • Fix my test chunkSize to some real value

To my surprise are two test fail.
SearchBarTest::testFindNextIncremental()
SearchBarTest::testSetMatchCaseIncremental()
Doing the same by hand looks OK...hm

loh.tar updated this revision to Diff 52728.Feb 27 2019, 2:42 PM
loh.tar edited the summary of this revision. (Show Details)
loh.tar set the repository for this revision to R39 KTextEditor.
  • Don't trigger onIncPatternChanged when update search history
  • Only save search history on some change

One nice issue found, but the test is still failing.
But I think that's a problem of the test. The values are compared while the search has not finished.
Unfortunately would the test finish fine when we exclude the above mentioned special case under "(No)Issues" :-/ But I think that's only a hint that not all cases tested.

The batch processing approach is sane.
My real issue here is: What to do with multi-line expressions that might span more than one line? Won't we miss matches if the ranges just overlap at that locations?

My proposal would be:

  1. not compute the batch ranges beforehand but just remember the complete range and compute the range for the next scan.
  2. after scan is done without a hit, not start behind the range but start in the middle of the current range with again such a batch.

That way we should avoid to skip even multi-line matches if you don't do perversions that match "xxxx" lines, which I think is ok for incremental search.
The drawback is that one searches more text.

Perhaps Dominik has some ideas, too :P

The batch processing approach is sane.

hm, thanks

My real issue here is: What to do with multi-line expressions that might span more than one line? Won't we miss matches if the ranges just overlap at that locations?

Well, yes...that should be the case. I had also some idea but ...

not compute the batch ranges beforehand

...that sound not so nice. What if I would keep it but find some solution?

OTH is that multiline search for me a slightly rare case

The batch processing approach is sane.

hm, thanks

My real issue here is: What to do with multi-line expressions that might span more than one line? Won't we miss matches if the ranges just overlap at that locations?

Well, yes...that should be the case. I had also some idea but ...

not compute the batch ranges beforehand

...that sound not so nice. What if I would keep it but find some solution?

I am not sure that the precomputation saves you a lot of things, you can your compute the next batch in the slot that searches.
But if the issue with the multi-line stuff is solved, I won't t reject this for the precomputation, this still can be "optimized" later on if wanted.

OTH is that multiline search for me a slightly rare case

Yes, that is true.
It should just work reasonable well, if would be bad if e.g. even a trivial case like lala\nlala would break because of badly choosen boundaries.

loh.tar updated this revision to Diff 53281.Mar 6 2019, 2:34 PM
loh.tar retitled this revision from [RFC]SearchBar: Don't block GUI when enter incremental pattern to SearchBar: Don't block GUI when enter incremental pattern.
loh.tar edited the test plan for this revision. (Show Details)
  • Consider multi line pattern
  • Remove notes from Test Plan

TODOs, but how?

  • Fix failing autotest
  • Add autotest for feature?
loh.tar updated this revision to Diff 53752.Mar 12 2019, 7:17 PM
loh.tar set the repository for this revision to R39 KTextEditor.
  • Fix autotest by add QTest::qWait(0)
loh.tar edited the summary of this revision. (Show Details)Mar 12 2019, 7:48 PM

Simple tests with an 12 million lines document (COPYING.LIB concated "a lot") show compare to the current approach usable behavior.
You start to type some word not in the document, it won't free for 10 seconds, it will just search in the background like it should.
I think the largest existing issue is IMHO the multi line stuff. Some more comments could help.

Should the

const int newChunkOverlap = pattern.count(QLatin1Char('\n'));
....

actually disable it for multi line searches?

I thought a bit about the problem with multi-line regex.

I think we can solve this in a consistent way by allowing to incremental do matches that span more than one line.

See the first example code in this commit:

https://cgit.kde.org/ktexteditor.git/commit/?id=dbebf059575f83608c941111ac3a6d374667b5a6

This would allow to handle multi-line regex matching just like normal matching, the search code would take care to on-demand concatenate enough lines for partial matches.