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
cullmann |
KTextEditor | |
VDG |
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
Before
New waiting for action
New while running job
Lint Skipped |
Unit Tests Skipped |
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?
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
I would probably not make the button that big
There are two reasons why it is so big:
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
- setup
- trigger the search part wise via e.g. single shot timer
- 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).
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
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.
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.
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.
That's my first draft of that requested singleShot approach, help is now appreciated.
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)
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?
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.
Notes:
src/search/katesearchbar.h | ||
---|---|---|
153 ↗ | (On Diff #49754) | It's exported class you cannot remove a function, it breaks the ABI |
173–181 | Do not change. | |
174 ↗ | (On Diff #49754) | Same here. |
222–228 | 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. |
It's exported class you cannot...
Argh! How can I see this quickly the next time? KTEXTEDITOR_EXPORT ?
Fixed/Simplified as suggested
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
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
My current status is: No crashes I'm aware of and everything seems to work.
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->... |
src/search/katesearchbar.cpp | ||
---|---|---|
860–865 | Only here or everywhere? |
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.
src/search/katesearchbar.h | ||
---|---|---|
222–228 | This is a purely internal class. |
I'm still looking for a better name for "m_cancelFindOrReplace", suggestions?
src/search/katesearchbar.h | ||
---|---|---|
222–228 | 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. :-) |
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
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 :=)