Find/Replace overlay: immediate replace after selected text #2011 #2052
Conversation
Test Results 1 815 files ±0 1 815 suites ±0 1h 35m 4s ⏱️ + 1m 15s For more details on these failures, see this check. Results for commit 866fba1. ± Comparison against base commit 816820a. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Functionality is sound and aligns with the FindReplaceOverlay. Test is a proper regression test. Thank you!
Just one thing: I do not understand the commit message. Where is the bug in the implementation of the IFindReplaceTargetExtension3? And if that was the case, why not fix that implementation?
|
@HeikoKlare the Bug we need to work around is the one we already encountered when implementing /**
* The TextViewer implementation of IFindReplaceLogic is misleading and not adhering to the
* Interface: IFindReplaceTargetExtension3#replaceSelection will NOT replace the selection if
* nothing was previously found. This does not generally have to be the case and thus we mock a
* target that is setup like this:
*
* The text contained is {@code ~SELECTEDTEXT~abcd} with ~ marking the boundaries of the
* selection. We perform a search for "NOTFOUND" first - the selection stays put since the
* string was not found. At this point, we do not want to perform
* {@code replaceSelection("NOTFOUND")} since an implementation adhering to the specification of
* the function would just overwrite the current selection.
*/In short: the editor will only replace a selection if it "searched" for it before, even though we would expect it to "blindly replace" the selection. As for fixing the implementation, I am not sure if we want to get into that; we would need to write tests for the Implementation of FindReplaceTargetExtension3 of the Editor, learn and debug the code and hope that no user depends on this behavior. What do you think? |
|
I see, still I find it confusing to have a commit message stating that there is a bug that we work around without stating explicitly what and where the bug is and that it is accepted to (currently) not address it. Taking a look at that bug led me to a related bug in the current implementation of the overlay: when having the regex option enabled, the string selected in the editor is not properly transformed into a regex when inserting it into the overlay (like done in the existing dialog). This PR will make the situation worse, as a find operation is executed based on that string, which can change the selection in the editor: |
|
@HeikoKlare we can fix that bug first and then rebase this PR on that PR - what do you think? |
Yes, makes sense 👍 |
|
@HeikoKlare I rebased this PR on #2056 as discussed. |
|
I have fixed the merging conflicts. |
…latform#2011 The TextViewer implementation of IFindReplaceTargetExtension3.replaceSelection() does not perform a replace operation if no preceeding find operation was called. To address this accepted deviation from the interface's specification, this change forces the overlay to search for an initializing word again so that the Editor can then later replace it if required. fixes eclipse-platform#2011
|
Failing test is unrelated: #1935 |
|
Thank you for this fix! |
Work around for a bug of the Editor's implementation of the
FindReplaceTargetExtension3; when initializing the search with an
already selected string, force the overlay to search for it again so
that the Editor can then later replace it if required.
fixes #2011