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

[loh tar]

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-frameworks-devel, #ktexteditor, hase, michaelh, ngraham, bruns, demsking, 
sars

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

[Christoph Cullmann]

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?vs=49754=49928

REVISION DETAIL
  https://phabricator.kde.org/D17459

AFFECTED FILES
  src/search/katesearchbar.cpp
  src/search/katesearchbar.h
  src/search/searchbarpower.ui

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: dhaumann, anthonyfieroni, brauch, cullmann, abetts, kwrite-devel, 
kde-frameworks-devel, #ktexteditor, hase, michaelh, ngraham, bruns, demsking, 
sars

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

[Christoph Cullmann]

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/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 :=)

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-frameworks-devel, #ktexteditor, hase, michaelh, ngraham, bruns, demsking, 
sars

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

[loh tar]

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 - kateindenttest_testCppstyle (Failed)
66 - vimode_keys (Failed)
  
  Errors while running CTest

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17459?vs=48391=49754

REVISION DETAIL
  https://phabricator.kde.org/D17459

AFFECTED FILES
  src/search/katesearchbar.cpp
  src/search/katesearchbar.h
  src/search/searchbarpower.ui

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: dhaumann, anthonyfieroni, brauch, cullmann, abetts, kwrite-devel, 
kde-frameworks-devel, #ktexteditor, hase, michaelh, ngraham, bruns, demsking, 
sars

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

[Dominik Haumann]

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 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. 
:-)

REVISION DETAIL
  https://phabricator.kde.org/D17459

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: dhaumann, anthonyfieroni, brauch, cullmann, abetts, kwrite-devel, 
kde-frameworks-devel, #ktexteditor, hase, michaelh, ngraham, bruns, demsking, 
sars

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

[loh tar]

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=48391

REVISION DETAIL
  https://phabricator.kde.org/D17459

AFFECTED FILES
  src/search/katesearchbar.cpp
  src/search/katesearchbar.h
  src/search/searchbarpower.ui

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: anthonyfieroni, 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

[Christoph Cullmann]

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-frameworks-devel, #ktexteditor, hase, michaelh, ngraham, bruns, demsking, 
sars, dhaumann

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

[loh tar]

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, 
kde-frameworks-devel, #ktexteditor, hase, michaelh, ngraham, bruns, demsking, 
sars, dhaumann

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

[Christoph Cullmann]

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 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 ;=)

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17459

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: anthonyfieroni, 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

[Christoph Cullmann]

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
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17459

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: anthonyfieroni, 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]

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=47966

REVISION DETAIL
  https://phabricator.kde.org/D17459

AFFECTED FILES
  src/search/katesearchbar.cpp
  src/search/katesearchbar.h
  src/search/searchbarpower.ui

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: anthonyfieroni, 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]

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 compiler(?)

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17459

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: anthonyfieroni, 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

[Anthony Fieroni]

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 = replaceMode;
> +d(this)->m_matchCounter = 0;
> +d(this)->m_cancelFindOrReplace = false; // Ensure we have a GO!

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

  auto dd = d(this);
  dd->...

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17459

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: anthonyfieroni, 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]

loh.tar added a comment.


  I have tried to run two S jobs at the same time on the same document, seems 
to works nicely
  
  - view 1 -> S "tab" -> "-"
  - view 2 -> S "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
  F6496970: still-running-near-finish.png 
  After finish, scrolled down
  F6496972: finished-scrolled-down.png 
  
  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 (?)

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17459

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: anthonyfieroni, 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]

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 job is running
  - Fix crash when the view will closed while a S job is running
  
  These fixes seems to work so far but they do not block the user action which 
is somehow an unusual behaviour.

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17459?vs=47907=47963

REVISION DETAIL
  https://phabricator.kde.org/D17459

AFFECTED FILES
  src/search/katesearchbar.cpp
  src/search/katesearchbar.h
  src/search/searchbarpower.ui

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: anthonyfieroni, 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

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> katesearchbar.cpp:89
> +ret = new KateSearchBarPrivate;
> +d_func()->insert(foo, ret);
> +}

You can take another approach

  connect(foo, ::destroyed, d_func, [foo]() { delete 
d_func()->take(foo); });

> katesearchbar.cpp:94-97
> +static void delete_d(const KateSearchBar *foo)
> +{
> +delete d_func()->take(foo);
> +}

Remove.

REVISION DETAIL
  https://phabricator.kde.org/D17459

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: anthonyfieroni, 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]

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 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.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17459?vs=47891=47907

REVISION DETAIL
  https://phabricator.kde.org/D17459

AFFECTED FILES
  src/search/katesearchbar.cpp
  src/search/katesearchbar.h
  src/search/searchbarpower.ui

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: anthonyfieroni, 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

[Anthony Fieroni]

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
> +KateSearchBarPrivate *ret = d_func()->value(foo);
> +delete ret;
> +d_func()->remove(foo);

Use directly

  delete d_func()->take(foo);

REVISION DETAIL
  https://phabricator.kde.org/D17459

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: anthonyfieroni, 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]

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 (hopefully) as described there 
https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B#Trouble_shooting
  - Some functions have now no implementation, is that a problem?
  - Some new function could be avoided and an old one used, but that would 
reduce the readability

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17459?vs=47843=47891

REVISION DETAIL
  https://phabricator.kde.org/D17459

AFFECTED FILES
  src/search/katesearchbar.cpp
  src/search/katesearchbar.h
  src/search/searchbarpower.ui

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: anthonyfieroni, 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

[Anthony Fieroni]

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 cannot remove a function, it breaks the ABI

> katesearchbar.h:172
>  // Helpers
> -bool find(SearchDirection searchDirection = SearchForward, const QString 
> *replacement = nullptr);
> -int findAll(KTextEditor::Range inputRange, const QString *replacement);
> +bool find(SearchDirection searchDirection = SearchForward) { return 
> findOrReplace(searchDirection, nullptr); };
> +bool findOrReplace(SearchDirection searchDirection, const QString 
> *replacement);

Do not change.

> katesearchbar.h:174
>  
> -void showInfoMessage(const QString );
>  

Same here.

> katesearchbar.h:221-227
> +KTextEditor::MovingRange *m_workingRange = nullptr;
> +KTextEditor::Range m_inputRange;
> +QString m_replacement;
> +uint m_matchCounter = 0;
> +bool m_replaceMode = false;
> +bool m_cancelFindOrReplace = false;
> +std::vector m_highlightRanges;

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.

REVISION DETAIL
  https://phabricator.kde.org/D17459

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: anthonyfieroni, 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]

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]

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 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

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17459?vs=47408=47843

REVISION DETAIL
  https://phabricator.kde.org/D17459

AFFECTED FILES
  src/search/katesearchbar.cpp
  src/search/katesearchbar.h
  src/search/searchbarpower.ui

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

[Christoph Cullmann]

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, #ktexteditor, 
hase, michaelh, ngraham, bruns, demsking, sars, dhaumann

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

[loh tar]

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 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() tasks. 
Is it a feature? :-)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17459?vs=47380=47408

REVISION DETAIL
  https://phabricator.kde.org/D17459

AFFECTED FILES
  src/search/katesearchbar.cpp
  src/search/katesearchbar.h
  src/search/searchbarpower.ui

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

[Christoph Cullmann]

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 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.
  
===

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]

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 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 of the highlight looks a little 
strage(?) Could it currently not reproduce

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]

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=47380

REVISION DETAIL
  https://phabricator.kde.org/D17459

AFFECTED FILES
  src/search/katesearchbar.cpp
  src/search/katesearchbar.h
  src/search/searchbarpower.ui

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

[Christoph Cullmann]

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 no good idea, but that could avoid some 
issues, and saves the re-creation of the range in each call)

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]

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=47379

REVISION DETAIL
  https://phabricator.kde.org/D17459

AFFECTED FILES
  src/search/katesearchbar.cpp
  src/search/katesearchbar.h
  src/search/searchbarpower.ui

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]

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 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

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17459?vs=47275=47362

REVISION DETAIL
  https://phabricator.kde.org/D17459

AFFECTED FILES
  src/search/katesearchbar.cpp
  src/search/katesearchbar.h
  src/search/searchbarpower.ui

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

[Christoph Cullmann]

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.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]

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

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

[Christoph Cullmann]

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

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

[Christoph Cullmann]

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.
  
  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.

REPOSITORY
  R39 KTextEditor

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

[Sven Brauch]

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 way to implement this would be something like, copy the 
required data, run the expensive S with QtConcurrent, and replace it back 
into the UI when done.

REPOSITORY
  R39 KTextEditor

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]

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 
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 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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17459

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: 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]

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 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 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 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?
  
  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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17459

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: 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]

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=47275

REVISION DETAIL
  https://phabricator.kde.org/D17459

AFFECTED FILES
  src/search/katesearchbar.cpp
  src/search/katesearchbar.h
  src/search/searchbarpower.ui

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: 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

[Christoph Cullmann]

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 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17459

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: 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]

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 
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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17459

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: 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

[Christoph Cullmann]

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/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

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17459

To: loh.tar, #ktexteditor, #vdg, cullmann
Cc: 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

[Andres Betts]

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, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

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

[loh tar]

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: loh.tar, #ktexteditor, #vdg
Cc: kwrite-devel, kde-frameworks-devel, #ktexteditor, hase, michaelh, ngraham, 
bruns, demsking, cullmann, sars, dhaumann

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

[loh tar]

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 endless S
  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

TEST PLAN
  Before
  F6466371: 154434.png 
  
  New waiting for action
  F6466378: 1544380395.png 
  
  New while running job
  F6466380: 1544381064.png 

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17459

AFFECTED FILES
  src/search/katesearchbar.cpp
  src/search/katesearchbar.h
  src/search/searchbarpower.ui

To: loh.tar, #ktexteditor, #vdg
Cc: kwrite-devel, kde-frameworks-devel, #ktexteditor, hase, michaelh, ngraham, 
bruns, demsking, cullmann, sars, dhaumann