SearchBar: Add Cancel button to stop long running tasks
ClosedPublic

Authored by loh.tar on Dec 9 2018, 7:20 PM.

Details

Summary

Without this patch could it happens that Kate run an almost endless S&R
job which could lead up to an unusable system due to a run out of memory
with no possibility to cancel the job other than kill Kate, See...

CCBUG: 333517
CCBUG: 244424

Test Plan

Before

New waiting for action

New while running job

Diff Detail

Repository
R39 KTextEditor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
loh.tar created this revision.Dec 9 2018, 7:20 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptDec 9 2018, 7:20 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
loh.tar requested review of this revision.Dec 9 2018, 7:20 PM

Not well tested but looks so far pretty promising.

The only thing what I not really like is these modulo check when to ask for user input. How about a QTimer with 200/300ms?

abetts added a subscriber: abetts.Dec 9 2018, 7:23 PM

I would probably not make the button that big, but I think it is useful

cullmann requested changes to this revision.Dec 9 2018, 7:50 PM
cullmann added a subscriber: cullmann.

The idea is good, I only am not sure that we want it with process events.
That always leads to evil things, like e.g. what happens if you press the X button of the view/window during that.
It would be better to refactor the replaceAll method in a way that it does

  1. setup
  2. trigger the search part wise via e.g. single shot timer
  3. some finalize phase at the end
This revision now requires changes to proceed.Dec 9 2018, 7:50 PM

I would probably not make the button that big

There are two reasons why it is so big:

  1. It's handy. You do not have to move the mouse, no matter which button was clicked
  2. It "covers" both buttons to avoid a re-start of any such action without the need to disable them

The first is of course the main reason.

That always leads to evil things, like e.g. what happens if you press the X button of the view/window during that.

Assumed the changes you want are done, I have problems to understand in which way this will avoid that the Close-Window button can be clicked. For a working Cancel button you have to call in some way the event loop, even direct as currently, or indirect due to some suspend from the current work.

It would be better to refactor the replaceAll method in a way that it does

  1. setup
  2. trigger the search part wise via e.g. single shot timer
  3. some finalize phase at the end

As you know, I'm a bit "slow" at times, Your instructions are not sufficient for me how to do that.

A good explanation for the timer stuff can be found here:

"Solving a Problem Step by Step" on https://doc.qt.io/archives/qq/qq27-responsive-guis.html

That avoids the pitfalls of process events (that is mentioned above in the article, too).

loh.tar updated this revision to Diff 47275.Dec 10 2018, 2:42 PM
  • Check every 500 lines or 100 machtes, whatever is first
  • Fix false check due to wrong math as long no match was found

That always leads to evil things, like e.g. what happens if you press the X button of the view/window during that.

I assume you have read that article a longer time ago and have these paragraph regarding Manual event processing in mind.

This approach has significant drawbacks. For example...

There are three listed

  1. you can't distribute computing power among different tasks
  2. makes the application react with delays to events
  3. the code is difficult to read and analyze

I can't see any hint in the whole article that would avoid to choose some action. It's all about keeping the GUI snappy. And that is always done by enter the event loop.

I think the points 1+2 are not important here. Point 3 do I not understand, at least the "read" part, but I think we have here not much more to analyze than if the Cancel-button works or not.

The S&SR process is not broken in some unclear state. There will only probably remaining matches not replaced.

You mean what happens e.g. when the user continue to edit while the S&R is running. I have just tried it with our good known fat file. It's funny. You can scroll, edit and almost watch how it progress.

There may the need to add some blocking for that. Is it possible to flag the document as "read only" without to break the started S&R?

There may the need to block other things too, like your mentioned view/window close.

One hint there was that timers are not fire without the event loop. I have tried this, and yes, no effect to check if a single shot timer isActive() or not. So I updated/fix the modulo check slightly because I noticed a lag in some cases. But may still not the best solution.

ATM is my conclusion: Your suggested re-design may fix probably responsive lags but not the problem to block ugly actions. Because this whole patch is only in rare cases really needed I would like avoid the effort to re-design the logic.

Hmpf! Some googling didn't help. Just some thoughts.

  • Adding a couple of checks where ever just to block in case of a running S&R could be a little cumbersome
  • What we need would be an QObject::processEvents, so that only the Cancle button is a live but no other stuff
  • Can we install an eventFilter and ignore all except our Cancel button?
  • QCoreApplication::processEvents take an optional argument e.g. QEventLoop::ExcludeUserInputEvents, nice but didn't help

I have tried the S&R plugin with the fat file. That's smart enough to see that the task is too heavy and stopped after a couple of hits when searching. The Cancel button there (almost) didn't appear in this use case when invoke the "replace all" action. Obviously is it only updated between file changes or so. Hence, we can not profit from his functionality.

brauch added a subscriber: brauch.EditedDec 10 2018, 6:15 PM

I would also advise against calling processEvents() to keep UIs responsive in all cases. It's tempting, but it is near impossible to get it right. What about conflicting actions, close/resize events, dbus calls, etc etc that are handled here?

It is something that comes up every once a while, but has just proven to be a bad idea over the last ten-something years.

I think the right way to implement this would be something like, copy the required data, run the expensive S&R with QtConcurrent, and replace it back into the UI when done.

You can't really do that in threads, you need to access the buffer, otherwise you can redo all things again in extra logic (like using the ranges for getting replacement ranges....)

I still think the normal single shot stuff should do the trick in most cases.

It is more work than the processEvents call, as one needs to re-adjust the code to be able to have some callable slot that does the batch processing.

I would also advise against calling processEvents()
It is something that comes up every once a while, but has just proven to be a bad idea over the last ten-something years.

I see. Your bad experiences advise you against it.

I still think the normal single shot stuff should do the trick in most cases.

That is complete unclear to me, as I have tried to explain above.

As I have already admitted, have I trouble to understand most of your code. You want that singleShot stuff. Ok. The only different should be than the remaining code below line ~891, right? (Well, in my last update I moved the processEvents call some lines up, but that could be fixed)

} while (!m_cancelSearchOrReplace);

What is there done which can cause big trouble when the user clicked nervous around before that part was processed?

After you return from processEvents, everything could have happened, e.g. the search bar got deleted because the view got delete => all things you do afterwards might do "something".

everything could have happened, e.g. the search bar got deleted because the view got delete

OK, but I still do not see why that will be different with that singleShot approach.

If you get deleted in-between that, the slot is never called again by the timer.
That means you are save.
It will just naturally stop work if the widget/... is destroyed.

loh.tar updated this revision to Diff 47362.Dec 11 2018, 3:41 PM
loh.tar edited the summary of this revision. (Show Details)
  • Code cosmetic
  • Add singleShot stuff

That's my first draft of that requested singleShot approach, help is now appreciated.

  • Replace crashes due to (I think) these pointer instead of reference argument mismatch.
  • Find works odd. Most of the time looks good, searching "5" or "A" seams to run endless in our fat file
  • Can't see how that loop works, "line" is not incremented and I'm not sure if "done" is sufficient set
loh.tar updated this revision to Diff 47379.Dec 11 2018, 7:23 PM
  • Fix crash in replace mode

I am atm a bit busy, but from principle the code goes into the right direction :)
Thanks for taking this up!
For the input range, I think it would make sense to store it as std::unique_ptr<KTextEditor::MovingRange> to have it taking care of "edits" in-between the calls (even if that is no good idea, but that could avoid some issues, and saves the re-creation of the range in each call)

loh.tar updated this revision to Diff 47380.Dec 11 2018, 7:52 PM
  • Fix to remember loop state
loh.tar added a comment.EditedDec 11 2018, 8:00 PM

I am atm a bit busy

Thanks for the hint

For the input range, I think it would make sense to store it as std::unique_ptr<KTextEditor::MovingRange> to have it taking care of "edits" in-between the calls (even if that is no good idea, but that could avoid some issues, and saves the re-creation of the range in each call)

Um, have updated before read this. have droped your std::unique_ptr because im not familar with, no idea if my solution has ugly drawbacks. Is there a need to delete the hold object now?
Will do other things for 1-2 days, so please look closer at it and give advices in DETAIL ;-)

Ah, I noticed that after a some replace the highlight looks a little strage(?) Could it currently not reproduce

For: Um, have updated before read this. have droped your std::unique_ptr because im not familar with, no idea if my solution has ugly drawbacks. Is there a need to delete the hold object now?

> Yes, as keeping a MovingRange alive has a cost (it will need to be updated on each editing action), therefore one should create it before the find/replaceAll and delete it afterwards. A unique_ptr doesn't do much more than ensuring it is deleted at end of life-time of the object, too.

loh.tar updated this revision to Diff 47408.Dec 12 2018, 1:34 AM
  • Use new (dis)connect style in touched functions
  • Clean-Up no longer needed MovingRange

Thanks for the explaining. Sounds like QScopedPointer(?)
I have not used any of them now because it looked to me to have not much benefit in this case(?)

BTW: I Always try to avoid a "Standard" version when there is a "Cute" available ;-) just only because std stuff looks for me ugly.

The noted odd highlighting still happens when you do successive S(&R) tasks. Is it a feature? :-)

I am at the moment a bit busy, if nobody else has time, I will try to take a closer look after x-mas.

loh.tar updated this revision to Diff 47843.Dec 19 2018, 2:11 PM
  • Fix too much stretching of Cancel|Find/Replace button (I misunderstood probably Andres above :-)
  • Add S&R progress hint, Rename showInfoMessage(..) -> showResultMessage() and move logic in what text to show

Notes:

  • The auto hide time of KTextEditor::Message seams to be buggy, stays sometimes much longer on display
  • The showResultMessage is always green/Positive (as it was) also on no matches. Blue/Info may always fit this way or I can add some more checks to display a red/Error message in such case
loh.tar edited the summary of this revision. (Show Details)Dec 19 2018, 9:32 PM
anthonyfieroni added inline comments.
src/search/katesearchbar.h
153

It's exported class you cannot remove a function, it breaks the ABI

172

Do not change.

174

Same here.

221–227

You cannot add new members as well, they change object size. Since this class not use pimpl idiom it will be harder to change anything in header except to add new non-virtual functions.

loh.tar updated this revision to Diff 47891.Dec 20 2018, 3:01 PM
  • Workaround to keep binary compatibility

It's exported class you cannot...

Argh! How can I see this quickly the next time? KTEXTEDITOR_EXPORT ?

anthonyfieroni added inline comments.Dec 20 2018, 3:19 PM
src/search/katesearchbar.cpp
83

It's not needed, right?

97–99

Use directly

delete d_func()->take(foo);
loh.tar updated this revision to Diff 47907.Dec 20 2018, 7:25 PM

Fixed/Simplified as suggested

  • I noticed a reproducible crash when e.g. Kate will closed while a S&R is running. Happens also when a split view is closed (so far so good) and then Kate will closed . I will try to solve this later, but help is appreciated.
anthonyfieroni added inline comments.Dec 20 2018, 7:54 PM
src/search/katesearchbar.cpp
89

You can take another approach

connect(foo, &Qobject::destroyed, d_func, [foo]() { delete d_func()->take(foo); });
94–97

Remove.

loh.tar updated this revision to Diff 47963.Dec 21 2018, 3:12 PM
loh.tar set the repository for this revision to R39 KTextEditor.
  • Fix crash when the document will closed while a S&R job is running
  • Fix crash when the view will closed while a S&R job is running

These fixes seems to work so far but they do not block the user action which is somehow an unusual behaviour.

I have tried to run two S&R jobs at the same time on the same document, seems to works nicely

  • view 1 -> S&R "tab" -> "-"
  • view 2 -> S&R "0" -> "+"

I had canceled both jobs and then resume, that's why at the pic are already replacents to see while the job is still running. Job is almost finished, had taken a couple of minutes and ~2GB RAM


After finish, scrolled down

My current status is: No crashes I'm aware of and everything seems to work.

  • "m_cancelFindOrReplace" may better renamed to "m_jobIsRuning" or similar
  • Is the "Cancel" perhaps better named "Stop"?
  • The new added, and now "d->m_foo" variables by better renamed to "d->foo", but perhaps will never a true d-pointer added but the new variables moved back as member again (?)
anthonyfieroni added inline comments.Dec 21 2018, 3:22 PM
src/search/katesearchbar.cpp
860–865

Maybe not an issue, but you can try to cache value preventing unwanted lookup

auto dd = d(this);
dd->...
loh.tar added inline comments.Dec 21 2018, 3:28 PM
src/search/katesearchbar.cpp
860–865

Only here or everywhere?
At this particular place may that optimized by the compiler(?)

loh.tar updated this revision to Diff 47966.Dec 21 2018, 3:53 PM

Improve the readability and prevent unwanted lookup by caching d(this) as dd

Btw., I have not played with the latest patch yet, but for the binary compatibility stuff: Nothing outside the KTextEditor:: namespace needs to be binary compatible.
We don't install headers for these classes, they are just exported for the unit tests.

cullmann added inline comments.Dec 29 2018, 7:29 PM
src/search/katesearchbar.h
221–227

This is a purely internal class.
It is only exported for the unit tests, no header files are installed, therefore any change of the layout is ok ;=)

Do I have to revert this d-stuff?
If so, some other tweak, or just as it was?

The d-stuff should go out again, beside that i still need to test this more.

loh.tar updated this revision to Diff 48391.Dec 30 2018, 11:49 AM
  • Revert d-stuff
  • Remove BCI stuff

I'm still looking for a better name for "m_cancelFindOrReplace", suggestions?

dhaumann added inline comments.
src/search/katesearchbar.h
221–227

Yes: as rule of thumb: only classes in the KTextEditor namespace are public API. Everything else can be freely changed. In this case, as Christoph points out, this is purely internal API.

PS: maybe we should have a KTEXTEDITOR_TEST_EXPORT macro to make this explicit. :-)

loh.tar updated this revision to Diff 49754.Jan 17 2019, 9:15 PM
loh.tar set the repository for this revision to R39 KTextEditor.
  • Fix to pass autotest

97% tests passed, 2 tests failed out of 67

Total Test time (real) = 137.70 sec

The following tests FAILED:

24 - kateindenttest_testCppstyle (Failed)
66 - vimode_keys (Failed)

Errors while running CTest

cullmann accepted this revision.Jan 20 2019, 12:46 PM

I played with the current state.

I like it ;=)

That the button "Cancel" spans over both "Replace/Find All" things is not bad in my eyes,

This fixes the issue of "Kate/KWrite/... must be killed on long replace operations".

What I would like to have further, now that some "batching" code is there: Try to apply it for the normal search, too.

e.g. if you search for some word in a large file, the GUI might block "long" even during typing, if no early match is found.

But that is a separate issue.

loh.tar, are you interested in that challenge, too?
Given you are now more "into" the searching code.

Btw., here all unit tests pass (beside the Qt 5.12 breackage of kateindenttest_testCppstyle)

I will land this, to have it tested by "all" people :=)

This revision is now accepted and ready to land.Jan 20 2019, 12:46 PM
This revision was automatically updated to reflect the committed changes.

are you interested in that challenge, too?

Um...atm? No.