Find/Replace Overlay: Restore cursor position when search input is cleared #3379
Conversation
deepika-u
force-pushed
the
1944_cursor_reset
branch
from
72bb8b1 to
7a05734
Compare
There was a problem hiding this comment.
@deepika-u thank you for working on this!
We already have a bunch of tests for the FindReplaceLogic as well as for the dialog and overlay UIs (in FindReplaceUITest). Could you please extend them with test methods to cover the use cases you address? That would help to verify the behavior (now and against regressions in the future), and it would also make it easier to understand how the additions are supposed to behave when one can just read the use cases in the form of tests.
Test Results 3 018 files 3 018 suites 2h 33m 8s ⏱️ For more details on these failures and errors, see this check. Results for commit 46003dd. ♻️ This comment has been updated with latest results. |
Let me check on them and also test failures. Thanks for your time. |
deepika-u
force-pushed
the
1944_cursor_reset
branch
6 times, most recently
from
0088837 to
22bbb44
Compare
|
@HeikoKlare |
a new search pattern.
deepika-u
force-pushed
the
1944_cursor_reset
branch
from
22bbb44 to
46003dd
Compare
There was a problem hiding this comment.
Thank you for the contribution and the addition of tests!
I tested the improvement a bit and it generally it works quite fine for me. I found one use case that behaves different than what I would expect (see the comments for it). And currently the change looks more complex to me than it needs to be, but maybe I miss something when trying to understand the purpose of that code (also see one of the comments for this).
| // Initially empty find string | ||
| logic.setFindString(""); | ||
| // Now start a new search (transition empty -> non-empty) | ||
| logic.setFindString("beta"); |
There was a problem hiding this comment.
Wouldn`t it be better to use, e.g., "gamma" here, as (6,0) will now be both the position of the current incremental match as well as the restoreBaseLocation?
| // Now start a new search (transition empty -> non-empty) | ||
| logic.setFindString("beta"); | ||
| // Expect the stored restore location to match the caret before search started | ||
| assertThat(((FindReplaceLogic) logic).getRestoreBaseLocation(), is(new Point(6, 0))); |
There was a problem hiding this comment.
It seems like this cast and exposing getRestoreBaseLocation() is only necessary for testing purposes. Since these tests are supposed to test the API, wouldn't it be better to test use cases of the API? E.g., a user is not interested in what the internal restoreBaseLocation value is (that's an implementation detail) but they are interested in the text viewer selection restoring to the original position after doing some search and then removing the find string again.
As an example, a test flow could be like this:
- Open document "alpha beta gamma"
- Set lection position in document to 6 (before "beta")
- Search for "gamma"
- Remove search input
- Expectation: selection position is 6 (whlie before implementing this feature it was 11)
| textViewer.setSelectedRange(6, 0); | ||
| logic.setFindString("beta"); | ||
| // Verify that restoreBaseLocation updated to the new caret position | ||
| assertThat(((FindReplaceLogic) logic).getRestoreBaseLocation(), is(new Point(6, 0))); |
There was a problem hiding this comment.
I tested a similar scenario which does not currently not work, and I am not sure if it's intended or not:
- Open document "alpha beta gamma"
- Set selection in document to 0 (before "alpha")
- Search for "gamma"
- Set selection in document to 6 (before "beta")
- Remove the search input
I expect the cursor to move to the last selected position 6 (before "beta"), but I get the cursor at position 0, the one when find string was empty for the last time.
| if (isSetRestoreBaseLocation) { | ||
| restoreBaseLocation = incrementalBaseLocation; | ||
| isSetRestoreBaseLocation = false; | ||
| } |
There was a problem hiding this comment.
Can you explain the purpose of this code?
isSetRestoreBaseLocation seems to be initalized with true and is then set to false here, thus it's a one-time execution in the lifecycle of an instance of FindReplaceLogic. But to the best of my knowledge such an instance will live as long as an editor lives, which means that this code is just executed on first opening of a find/replace UI in that editor, and then it will never be executed again. And this first time happens right on initialization, as resetting the incremental base location happens when activating the INCREMENTAL search option. The restoreBaseLocation will only become relevant when a search string was entered and that will in every case happen after the incremental base position was set for the first time.
So I wonder if this code and the isSetRestoreBaseLocation field is necessary at all?
| if (target != null) | ||
| restoreBaseLocation = target.getSelection(); |
There was a problem hiding this comment.
Please always put expression blocks into braces (applies in the same way also to other places in the PR).
| if (target != null) | |
| restoreBaseLocation = target.getSelection(); | |
| if (target != null) { | |
| restoreBaseLocation = target.getSelection(); | |
| } |
2 participants
Restores the editor caret to its original position when the Find/Replace overlay search field is cleared.
Fixes #1944
This change introduces logic to:
Also to test the new changes, test cases are added.
1. It validates that:
- The initial incremental search starts correctly.
- Resetting the base location manually affects subsequent performSearch() calls.
- The selection updates to expected “match start + length” positions.
2. Caret position capture on new search start (setFindString() change).
3. Caret restore when find string becomes empty (performSearch() + restoreSelectionIfEmpty() change).
4. Proper restore behavior across multiple search sessions (ensure it doesn’t reuse an old restoreBaseLocation).
Accordingly the below test cases are updated/added.
@HeikoKlare :
Can you have a look into this when you get some time please.