D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar added a comment. > are you interested in that challenge, too? Um...atm? No. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17459 To: loh.tar, #ktexteditor, #vdg, cullmann Cc: dhaumann, anthonyfieroni, brauch, cullmann, abetts, kwrite-devel, kde-fram

D17459: SearchBar: Add Cancel button to stop long running tasks

This revision was automatically updated to reflect the committed changes. Closed by commit R39:7cf6644e8017: SearchBar: Add Cancel button to stop long running tasks (authored by loh.tar, committed by cullmann). REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/

D17459: SearchBar: Add Cancel button to stop long running tasks

cullmann accepted this revision. cullmann added a comment. This revision is now accepted and ready to land. 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/KWri

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar updated this revision to Diff 49754. loh.tar set the repository for this revision to R39 KTextEditor. loh.tar added a comment. - 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 - kat

D17459: SearchBar: Add Cancel button to stop long running tasks

dhaumann added inline comments. INLINE COMMENTS > cullmann wrote in 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 ;=) Yes: as rule of thumb: only classes in the KText

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar updated this revision to Diff 48391. loh.tar added a comment. - Revert d-stuff - Remove BCI stuff I'm still looking for a better name for "m_cancelFindOrReplace", suggestions? CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17459?vs=47966&id=48391 REVISION DETAIL htt

D17459: SearchBar: Add Cancel button to stop long running tasks

cullmann added a comment. The d-stuff should go out again, beside that i still need to test this more. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17459 To: loh.tar, #ktexteditor, #vdg, cullmann Cc: anthonyfieroni, brauch, cullmann, abetts, kwrite-devel, kde-

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar added a comment. Do I have to revert this d-stuff? If so, some other tweak, or just as it was? REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17459 To: loh.tar, #ktexteditor, #vdg, cullmann Cc: anthonyfieroni, brauch, cullmann, abetts, kwrite-devel, kd

D17459: SearchBar: Add Cancel button to stop long running tasks

cullmann added inline comments. INLINE COMMENTS > anthonyfieroni wrote in katesearchbar.h: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. This

D17459: SearchBar: Add Cancel button to stop long running tasks

cullmann added a comment. 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. REPOSITORY

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar updated this revision to Diff 47966. loh.tar added a comment. Improve the readability and prevent unwanted lookup by caching d(this) as dd REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17459?vs=47963&id=47966 REVISION DETAIL https://phabricat

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar added inline comments. INLINE COMMENTS > anthonyfieroni wrote in katesearchbar.cpp:860-865 > Maybe not an issue, but you can try to cache value preventing unwanted lookup > > auto dd = d(this); > dd->... Only here or everywhere? At this particular place may that optimized by the com

D17459: SearchBar: Add Cancel button to stop long running tasks

anthonyfieroni added inline comments. INLINE COMMENTS > katesearchbar.cpp:860-865 > +d(this)->m_inputRange = inputRange; > +d(this)->m_workingRange = > m_view->doc()->newMovingRange(d(this)->m_inputRange); > +d(this)->m_replacement = replacement; > +d(this)->m_replaceMode = repla

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar added a comment. 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 jo

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar updated this revision to Diff 47963. loh.tar set the repository for this revision to R39 KTextEditor. loh.tar added a comment. - 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 t

D17459: SearchBar: Add Cancel button to stop long running tasks

anthonyfieroni added inline comments. INLINE COMMENTS > katesearchbar.cpp:89 > +ret = new KateSearchBarPrivate; > +d_func()->insert(foo, ret); > +} You can take another approach connect(foo, &Qobject::destroyed, d_func, [foo]() { delete d_func()->take(foo); }); > katesea

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar updated this revision to Diff 47907. loh.tar added a comment. 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 solv

D17459: SearchBar: Add Cancel button to stop long running tasks

anthonyfieroni added inline comments. INLINE COMMENTS > katesearchbar.cpp:83 > +typedef QHash > KateSearchBarPrivateHash; > +static KateSearchBarPrivateHash KateSearchBarPrivateData; > +Q_GLOBAL_STATIC(KateSearchBarPrivateHash, d_func) It's not needed, right? > katesearchbar.cpp:97-99 > +K

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar updated this revision to Diff 47891. loh.tar added a comment. - Workaround to keep binary compatibility > It's exported class you cannot... Argh! How can I see this quickly the next time? KTEXTEDITOR_EXPORT ? - Seams still to work so far, but looks not so lovely - Fixed

D17459: SearchBar: Add Cancel button to stop long running tasks

anthonyfieroni added inline comments. INLINE COMMENTS > katesearchbar.h:153 > -bool find(SearchDirection searchDirection = SearchForward, const QString > *replacement = nullptr); > -int findAll(KTextEditor::Range inputRange, const QString *replacement); > It's exported class you canno

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar edited the summary of this revision. REVISION DETAIL https://phabricator.kde.org/D17459 To: loh.tar, #ktexteditor, #vdg, cullmann Cc: brauch, cullmann, abetts, kwrite-devel, kde-frameworks-devel, #ktexteditor, hase, michaelh, ngraham, bruns, demsking, sars, dhaumann

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar updated this revision to Diff 47843. loh.tar added a comment. - 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:

D17459: SearchBar: Add Cancel button to stop long running tasks

cullmann added a comment. I am at the moment a bit busy, if nobody else has time, I will try to take a closer look after x-mas. REVISION DETAIL https://phabricator.kde.org/D17459 To: loh.tar, #ktexteditor, #vdg, cullmann Cc: brauch, cullmann, abetts, kwrite-devel, kde-frameworks-devel, #kt

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar updated this revision to Diff 47408. loh.tar added a comment. - 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 m

D17459: SearchBar: Add Cancel button to stop long running tasks

cullmann added a comment. 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 u

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar added a comment. In D17459#375429 , @cullmann wrote: > 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 to have it taking care of "edits" in-between th

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar updated this revision to Diff 47380. loh.tar added a comment. - Fix to remember loop state CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17459?vs=47379&id=47380 REVISION DETAIL https://phabricator.kde.org/D17459 AFFECTED FILES src/search/katesearchbar.cpp src/search/

D17459: SearchBar: Add Cancel button to stop long running tasks

cullmann added a comment. 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 to have it taking care of "edits" in-between the calls (even if that is n

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar updated this revision to Diff 47379. loh.tar added a comment. - Fix crash in replace mode CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17459?vs=47362&id=47379 REVISION DETAIL https://phabricator.kde.org/D17459 AFFECTED FILES src/search/katesearchbar.cpp src/search/k

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar updated this revision to Diff 47362. loh.tar edited the summary of this revision. loh.tar added a comment. - 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 p

D17459: SearchBar: Add Cancel button to stop long running tasks

cullmann added a comment. 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. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17459 To: loh.

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar added a comment. > 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. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17459

D17459: SearchBar: Add Cancel button to stop long running tasks

cullmann added a comment. 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". REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17459

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar added a comment. > 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 n

D17459: SearchBar: Add Cancel button to stop long running tasks

cullmann added a comment. 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.

D17459: SearchBar: Add Cancel button to stop long running tasks

brauch added a comment. 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? I think the right wa

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar added a comment. 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 ot

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar added a comment. > 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 appr

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar updated this revision to Diff 47275. loh.tar added a comment. - Check every 500 lines or 100 machtes, whatever is first - Fix false check due to wrong math as long no match was found REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17459?vs=47208

D17459: SearchBar: Add Cancel button to stop long running tasks

cullmann added a comment. 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). REPOSITORY R39

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar added a comment. > 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 th

D17459: SearchBar: Add Cancel button to stop long running tasks

cullmann requested changes to this revision. cullmann added a comment. This revision now requires changes to proceed. 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/windo

D17459: SearchBar: Add Cancel button to stop long running tasks

abetts added a comment. I would probably not make the button that big, but I think it is useful REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17459 To: loh.tar, #ktexteditor, #vdg Cc: abetts, kwrite-devel, kde-frameworks-devel, #ktexteditor, hase, michaelh, ngr

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar added a comment. 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? REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D17459 To: l

D17459: SearchBar: Add Cancel button to stop long running tasks

loh.tar created this revision. loh.tar added reviewers: KTextEditor, VDG. Herald added projects: Kate, Frameworks. Herald added subscribers: kde-frameworks-devel, kwrite-devel. loh.tar requested review of this revision. REVISION SUMMARY Without this patch could it happens that Kate run an almost