Find/Replace Overlay: Restore cursor position when search input is cleared #3379

deepika-u · 2025-10-07T13:45:49Z

Restores the editor caret to its original position when the Find/Replace overlay search field is cleared.

Fixes #1944

This change introduces logic to:

Store the current editor selection (caret position) when the user starts typing a new search pattern (transition from empty → non-empty).
Restore the stored selection when the search field becomes empty again, using IFindReplaceTargetExtension#setSelection(int, int).

Also to test the new changes, test cases are added.

Caret Position Capture:
When a user begins a new search (transition from empty → non-empty search string), the current editor selection is stored as the base location.
Caret Restoration:
When the search string becomes empty again, the stored selection is restored using IFindReplaceTargetExtension#setSelection(int, int).
Incremental Base Location Reset:
Manual updates to the caret position followed by resetIncrementalBaseLocation() ensure that subsequent incremental searches start from the updated location.
Session-aware Restoration:
The restore logic correctly refreshes between multiple search sessions, ensuring that the caret is not restored to stale positions.

Accordingly the below test cases are updated/added.

testResetIncrementalBaseLocation() - Verifies that manual base location resets affect incremental search results and that caret restoration works after clearing the search.
testSelectionRestoredAfterClearingSearch() - Ensures that the caret returns to its original position after clearing the search input.
testCaretRestoredWhenSearchCleared() - Validates that the caret is restored to the correct position after a search and clearing the input.
testCaretRestoredBetweenSearchSessions() - Confirms that the restore logic refreshes correctly between multiple search sessions and does not reuse stale positions.

@HeikoKlare :
Can you have a look into this when you get some time please.

HeikoKlare

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

github-actions · 2025-10-07T14:37:48Z

Test Results

3 018 files ±0 3 018 suites ±0 2h 24m 1s ⏱️ + 4m 46s
8 236 tests +3 7 987 ✅ +3 249 💤 ±0 0 ❌ ±0
23 628 runs +9 22 834 ✅ +9 794 💤 ±0 0 ❌ ±0

Results for commit 1364ad4. ± Comparison against base commit ce17c9e.

♻️ This comment has been updated with latest results.

deepika-u · 2025-10-07T15:13:17Z

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

Let me check on them and also test failures. Thanks for your time.

deepika-u · 2025-10-09T06:43:55Z

@HeikoKlare
As suggested added and updated junit test cases for the same. Have a look when you get some time please.

HeikoKlare

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

...kbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogicTest.java

HeikoKlare · 2025-10-10T18:53:04Z

...kbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogicTest.java

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


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.

The logic to capture restoreBaseLocation only triggers when transitioning from an empty to a non-empty find string (setFindString("") → setFindString("non-empty")).
In your case, you moved the caret after the search was already active, so the transition logic didn’t capture the new caret position.
In summary, also updated the test case. Your scenario fails because the caret must be moved before starting a new search string to capture the new base location. This is the behavior as per present design, based on the implementation in setFindString().

...pse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogic.java

HeikoKlare · 2025-10-10T19:19:24Z

...pse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogic.java

+			if (target != null)
+				restoreBaseLocation = target.getSelection();


Please always put expression blocks into braces (applies in the same way also to other places in the PR).

Suggested change

if (target != null)

restoreBaseLocation = target.getSelection();

if (target != null) {

restoreBaseLocation = target.getSelection();

}

Taken care, thanks.

deepika-u · 2025-10-22T05:05:54Z

Sorry, i couldnt reply on your comments as i had some workspace issues. Now all set, will check and reply on this.

deepika-u · 2025-10-24T13:14:39Z

@HeikoKlare
Can you recheck now when you get some time please? Mac failures are unrelated as i have seen. Thanks alot for your time.

a new search pattern.

deepika-u · 2025-10-28T07:00:42Z

Mac failures cleared now. But seeing this issue now "05:42:31.581 [ERROR] Failed to execute goal org.eclipse.tycho:tycho-source-plugin:5.0.1-SNAPSHOT:plugin-source (plugin-source) on project org.eclipse.ui.workbench: Error creating source archive: Problem creating jar: Execution exception: Java heap space -> [Help 1]

05:42:31.581 [ERROR] ".
@HeikoKlare - Thanks alot for your time.

HeikoKlare

Thank you for the update. With the simplification, the PR is much more streamlined. I still have some comments and I wonder if it cannot be further simplified:

The restore behavior is currently applied only depending on the find string state. However, this should probably only be a feature used when incremental mode is active. For example, what happens is that if you are using the find/replace dialog (without incremental model) and press ENTER when the find field is empty, it unexpectedly resets the cursor location to (0,0). So I think the behavior should be limited to incremental search and that should also be reflected in tests (either for the logic or maybe as system tests for the FindReplaceOverlay/FindReplaceDialog.
Why do we need to store another location at all? We already have the incrementalBasePosition, which is the position where incremental search starts. It will be reset when you perform an actual search (clicking the search button). After that, every incremental search starts from that search results. So that's also the position the user has in mind when typing and results become highlighted. Why should we restore to an even different position if we clear the input field? That both adds technical complexity and also more reference points for the user to consider, making the functionality less comprehensible (in my opinion).

So I propose to try to completely remove the new base location and only use the incremental base position to restore to when clearing the input field. In addition, this should only be done incremental mode is active (or at least no unexpected behavior should occur in case incremental mode is deactivated).

HeikoKlare · 2025-10-29T12:08:33Z

...pse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogic.java

 	private IFindReplaceTarget target;
 	private Point incrementalBaseLocation;
-
+	private Point restoreBaseLocation = new Point(0, 0);


Why initialize with (0,0) and then later check for null (which it can never be)? This leads to setting the cursor location to (0,0) when performing a search while no find string was entered.

HeikoKlare · 2025-10-29T12:09:08Z

...pse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogic.java

+		if (restoreBaseLocation == null)
+			return;


As noted in #3379 (comment): please always put expression blocks into braces (applies in the same way also to other places in the PR).

deepika-u force-pushed the 1944_cursor_reset branch from 72bb8b1 to 7a05734 Compare October 7, 2025 13:46

HeikoKlare reviewed Oct 7, 2025

View reviewed changes

deepika-u force-pushed the 1944_cursor_reset branch 6 times, most recently from 0088837 to 22bbb44 Compare October 9, 2025 06:33

deepika-u force-pushed the 1944_cursor_reset branch from 22bbb44 to 46003dd Compare October 10, 2025 06:01

HeikoKlare reviewed Oct 10, 2025

View reviewed changes

deepika-u force-pushed the 1944_cursor_reset branch 6 times, most recently from 8fa82e1 to c01ff0b Compare October 24, 2025 11:51

This pr stores the current editor caret position when user starts typing

1364ad4

a new search pattern.

deepika-u force-pushed the 1944_cursor_reset branch from c01ff0b to 1364ad4 Compare October 27, 2025 05:22

HeikoKlare reviewed Oct 29, 2025

View reviewed changes

		if (target != null)
		restoreBaseLocation = target.getSelection();

Uh oh!

Find/Replace Overlay: Restore cursor position when search input is cleared #3379

Are you sure you want to change the base?

Find/Replace Overlay: Restore cursor position when search input is cleared #3379

Conversation

deepika-u commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HeikoKlare left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

deepika-u commented Oct 7, 2025

Uh oh!

deepika-u commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HeikoKlare left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

HeikoKlare Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

deepika-u Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HeikoKlare Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

deepika-u Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

deepika-u commented Oct 22, 2025

Uh oh!

deepika-u commented Oct 24, 2025

Uh oh!

deepika-u commented Oct 28, 2025

Uh oh!

HeikoKlare left a comment

Choose a reason for hiding this comment

Uh oh!

HeikoKlare Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

HeikoKlare Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

deepika-u commented Oct 7, 2025 •

edited

Loading

github-actions bot commented Oct 7, 2025 •

edited

Loading

deepika-u commented Oct 9, 2025 •

edited

Loading