[Modern Find/Replace] Implemented Find-Replace Overlay

[Wittmaxi]

Modern Find-Replace Overlay

newly introduced Overlay in dark theme

newly introduced Overlay on the bottom of the screen, used to perform a replace-operation.

at the top of the screen, with some search-options enabled

What are we addressing (copied from #1090)

The current solution opens a Modal on keypress Control + F - prompting a user to enter a string for finding
and optionally a string to replace the currently found string with.

The status quo also has multiple options for searching which are available by selecting
the appropriate checkbox.

• The modal can be disturbing while programming
  • The modal requires a switch to a new window
  • The modal can hide important controls while open
  • It is not clear which View the Find/Replace-Dialog is bound to. This becomes an issue, when two editors are opened side-by-side.
• In many cases, the user "just want's to search", not needing too many options. I personally really like using vim's search feature for it's simplicity.
• Searching forward and backwards requires to select the correct radio button and then pressing "search" - which is not intuitive and hindering in many workflows

Showcase

2023-10-05_11-21-40.mp4

[github-actions]

Test Results

 1 812 files  +  604   1 812 suites  +604   1h 32m 26s ⏱️ + 39m 34s
 7 629 tests +   15   7 400 ✅ +   14  228 💤 ±  0  1 ❌ +1 
24 039 runs  +8 043  23 289 ✅ +7 762  749 💤 +280  1 ❌ +1 

For more details on these failures, see this check.

Results for commit c7f973c. ± Comparison against base commit 6990a01.

♻️ This comment has been updated with latest results.

[jukzi]

I like the idea!
But the implementation does not feel perfect yet:

1. The Minus Symbol is not at the same location as the Plus symbol. so expand/collapse does not feel right
2. The text "classic find/Replace" takes too much space. I would prefer a small button
3. i can't move the Popup around. It may hide some text in the editor. Should be either a) moveable (mouse drag) or b) outside the Editor content area. I would prefer to have it at the Bottom left - not as an overlay but like the statusbar.
4. i am missing the last searched/replaced words in a dropdown. I am using that a lot and its a real show stopper for me to not have access to the last searches.
5. The Match-Case Symbol does not feel intuitive. Better a capital letter next to an lower letter.
6. The colors feel a bit odd. i don't know what exactly is wrong. But most likely the grey border around the search input. Especially letters like "j","g" that have some drawing below the writing line do touch the border below, while Capital letters do not touch the upper border
   
7. after resizing the input field is sometimes distorted / text drawn to much left
   
8. it currently doe not work well in console view (wrong location, too small, +/- not working):
   
9. it should replace the search in the Javadoc search
10. error "Widget is disposed"

org.eclipse.swt.SWTException: Widget is disposed
	at org.eclipse.swt.SWT.error(SWT.java:4918)
	at org.eclipse.swt.SWT.error(SWT.java:4833)
	at org.eclipse.swt.SWT.error(SWT.java:4804)
	at org.eclipse.swt.widgets.Widget.error(Widget.java:450)
	at org.eclipse.swt.widgets.Widget.checkWidget(Widget.java:369)
	at org.eclipse.swt.widgets.Control.isFocusControl(Control.java:1899)
	at org.eclipse.ui.texteditor.FindReplaceOverlay$1.performEnterAction(FindReplaceOverlay.java:128)
	at org.eclipse.ui.texteditor.FindReplaceOverlay$1.keyPressed(FindReplaceOverlay.java:159)
	at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:171)

[Wittmaxi]

Thank you for your feedback, @jukzi.

In particular, 9) is related to this issue.

[jukzi]

Optically i like the "filter" bar in the Error Log View much more - below the editor it would be nice for searching:

[vogella]

Did we make any progress here? Would be a shame not to get this beautiful new UI into Eclipse.

[HeikoKlare]

@vogella Maximilian is still working on this. The first to merge is the refactoring in #1132. And then I hope that we can merge a first version of this new UI (probably to be optionally activated at that point in time) for 4.31 M1.

[Wittmaxi]

The layouting now happens in three stages:

1. Maximal width:
   Will stop growing at a certain width, such that it does not become too big (especially on large screens)
   

2. Compromise width
   The Toolbars are removed, making space for the inputs. Since all tools are still available through keybindings, we don't "lose" functionality per se, and you can still "quickly search" in your small editor. I made the assumption that users will not want to power-use the feature in an editor that was cast to the side and that users who want to perform harder operations that require the toolbar to be visible would also open the editor to long enough to display the entire find/replace-bar.
   
   

3. Worst-case compromise
   Every tool-button is discarded and the Dialog is resized to take up 90% of the width of the parent.
   You can still perform all actions and enable/disable the "Replace"-Feature using Ctrl+R
   
   

[vogella]

@Wittmaxi can you continue with this enhancement now that the refactoring is done?

[HeikoKlare]

can you continue with this enhancement now that the refactoring is done?

He is working on it, but it may still take some time due to limited time budget. We optimistically aim for a contribtion as soon as possible after 2024-03 freeze, so we have quite some time for evaluation in the development cycle for 2024-06.
We at least have to do these things for an initial contribution:

• Provide a configuration option via property (to select whether old dialog or new overlay shall be used)
• Fix a bug with overlay placement in different kinds of editors (current issue with the console view)
• Select / design proper icons and finalize the look&feel
• Conduct some intensive testing

[Wittmaxi]

I expect at least one Test to fail: The FindReplaceDialog-Test currently cannot test the default behavior since the "new Overlay" is opened by default. If this persists after I correctly parametrise the new Overlay, I might have overlooked a problem while merging.

[Wittmaxi]

@HeikoKlare @vogella very happy news!

I could fix another final milestone: the Overlay now binds to the constraints of Console's when opened on one (not yet perfectly...):

Here is a screenshot of the parameter used for selecting between Find/Replace-Methods:

The Find/Replace-Overlay is off by default (for now).

I have one doubt and need to check this:
What happens when I add a Listener to a Control in a PageBookView and the current Page changes? How can I retrieve that listener? Is that even necessary or will it just be destroyed? Do I risk some nullpointer-errors?

This is the relevant code:

	private void unbindListeners() {
		if (targetPart != null && targetPart instanceof StatusTextEditor textEditor) {
			Control targetWidget = textEditor.getSourceViewer().getTextWidget();
			if (targetWidget != null) {
				targetWidget.getShell().removeControlListener(shellMovementListener);
				targetWidget.removePaintListener(widgetMovementListener);
				targetPart.getSite().getPage().removePartListener(partListener);
			}
		}
		if (targetPart != null && targetPart instanceof PageBookView consoleView) {
			Control targetWidget = consoleView.getCurrentPage().getControl();
			if (targetWidget != null) {
				targetWidget.getShell().removeControlListener(shellMovementListener);
				targetWidget.removePaintListener(widgetMovementListener);
				targetPart.getSite().getPage().removePartListener(partListener);
			}
		}
	}

	private void bindListeners() {
		if (targetPart instanceof StatusTextEditor textEditor) {
			Control targetWidget = textEditor.getSourceViewer().getTextWidget();

			targetWidget.getShell().addControlListener(shellMovementListener);
			targetWidget.addPaintListener(widgetMovementListener);
			targetPart.getSite().getPage().addPartListener(partListener);
		}

		if (targetPart != null && targetPart instanceof PageBookView consoleView) {
			Control targetWidget = consoleView.getCurrentPage().getControl();

			targetWidget.getShell().addControlListener(shellMovementListener);
			targetWidget.addPaintListener(widgetMovementListener);
			targetPart.getSite().getPage().addPartListener(partListener);
		}
	}

[HeikoKlare]

I have some minor points left that were introduced by recent changes/rebases.

In addition, there is still the issue with the incremental base position I reported in my last review:

• The incremental search base location does only seem to be initialized when the overlay is first created with current cursor position. If you go to a different position in the document and press CTRL+F, the search will start at the original position again and not at the cursor location.

While other known issues regarding usability/accessibility can be addressed incrementally after merging this initial PR, this renders the find functionality rather useless, because you need to reopen the editor to start searching from a different position.

Here is a demonstration of the issue:

[Wittmaxi]

I have fixed the bug with the incremental base location not updating. This is the code where this happens:

// in searchBar.addFocusListener(new FocusListener..., ll. 670
			@Override
			public void focusGained(FocusEvent e) { 
				// we want to update the base-location of where we start incremental search
				// to the currently selected position in the target
				// when coming back into the dialog
				findReplaceLogic.deactivate(SearchOptions.INCREMENTAL);
				findReplaceLogic.activate(SearchOptions.INCREMENTAL);
			}

I had removed the call to "deactivate" because purely from the activation states it is redundant. For updating base-locations it is not.
This hints at a bigger problem: FindReplaceLogic should expose a method for updating an incremental base location instead of relying on this trick. We should look at this once this PR is through.

[HeikoKlare]

From my side, this one is almost ready to merge. Since it is now too late for 2024-06, we should merge this once the 2024-09 development stream opens. We should then switch back to having the feature enabled by default to ensure that many people will test it from I-Builds or latest from the M1 build.

When testing again, I asked myself one (or two) things regarding the expansion of the replace bar, in particular also considering that we had some feedback from people searching for the replace functionality as they were not able to identify the expansion button:

1. Instead of using an image for the expand/collapse button, we could also use UTF-8 text icons like ⯆ and ⯅. Benefits using text would be
   • less dependencies on code-external resources and
   • better themeing as fonts are properly colored so that we do not have to rely on a gray icon that fits for both light and dark themes but is not perfectly visible in both cases.
2. Instead of having the expand/collapse button on the left, we could put it below the search bar to make it easier to identify that clicking this will do some expansion.

Here is how it looks like with the mentioned text icons and font size set to 0.8. It would even be better when having smaller padding within the button, but I could not figure out yet how to achieve that.

For comparison, this is how it looks now:

[vogella]

1. Instead of using an image for the expand/collapse button, we could also use UTF-8 text icons like ⯆ and ⯅. Benefits using text would be

I like that one.

• less dependencies on code-external resources and
• better themeing as fonts are properly colored so that we do not have to rely on a gray icon that fits for both light and dark themes but is not perfectly visible in both cases.

2. Instead of having the expand/collapse button on the left, we could put it below the search bar to make it easier to identify that clicking this will do some expansion.

Here is how it looks like with the mentioned text icons and font size set to 0.8. It would even be better when having smaller padding within the button, but I could not figure out yet how to achieve that.

For comparison, this is how it looks now:

IMHO the existing display looks way better, the new very big up and down button look broken or like an UI glitch to me. So +1 for keeping the expand icon on the left, this also aligns with Vscode and as I said before multiple times, MS most likely has more UX experts working on VScode than we have in Eclipse.

[Wittmaxi]

@HeikoKlare @vogella

I have changed the button to be a StyledText with according Listeners instead.

I agree that the button looks a bit clumsy, but without the borders and the big area taking up space, I think this looks much cleaner than vscode. Most importantly, it makes the "replace"-feature even more obvious and stand out. This was a relevant problem that multiple testers had (they didn't even notice my overlay had a replace-feature!)

[vogella]

@HeikoKlare @vogella

I have changed the button to be a StyledText with according Listeners instead.

I agree that the button looks a bit clumsy, but without the borders and the big area taking up space, I think this looks much cleaner than vscode. Most importantly, it makes the "replace"-feature even more obvious and stand out. This was a relevant problem that multiple testers had (they didn't even notice my overlay had a replace-feature!)

I still find the new proposed style ugly / not good looking. Obviously this will be very visible to the user (hence they will easily discover the feature) as it is really dominant in the UI.

In case you want to inform the users I think a notification with a "Got it" button would be better which shows an animation how to open up the second line.

[vogella]

And for some background of my usage in Vscode. First time I wanted to use Replace in Vscode, I also had to search for it, I think it took me maybe 20 seconds to find the expand button using the tooltip of all the UI elements. But I would not like it if VScode would provide a big and always present replace button all the time just to save me a Google search or 20 seconds to look through the button tooltips.

[Wittmaxi]

@HeikoKlare @vogella
One more proposal: what about even adding this (or some other) text?

I am not sure if eclipse has enough documentation for a user to quickly find this feature - especially in the beginning.
(Although I intend on writing some public documentation)

[HeikoKlare]

I am sorry for disturbing this almost-ready-to-merge PR with my proposal. This was not meant as a kind of requirement from my side but just as an idea since there were concerns mentioned for the current expand/collapse realization. The answer "we won't/shouldn't do that" is totally fine for me. Some of my conclusions on this:

• I don't see relevant benefits from changing the way the expand/collapse is provided or people who are in favor of it because of other reasons, so we should just stick to existing solution which everyone has could have had a look at for quite some time and (implicitly or explicitly) agreed on.
• I would still be in favor of replacing the used icons with UTF-8 symbols for the mentioned reasons regarding theming and maintenance.
• I am not in favor of adding text to the open/collapse bar. I had also tried that but this either leads to text that is unreadably small or to a space-consuming expand bar to make the text readable.
• While I totally agree that MS probably has more UX experts than the Eclipse development has and even though I also think that they designed the find/replace bar in a good way, this is not an argument that I would generally agree on because:
  1. Just because one solution is good does not mean that another may not be equally good. I agree that in this case it makes sense that equal tools look and behave equally, but there might be other cases were it could be good to have some distinction.
  2. UX is not (only) a "local" topic w.r.t. to a single UI component/composite but an overall concept of an application, so just because a specific realization fits well for one tool does not mean that is also fits for another if the overall UX concepts of the application differ.

Don't get me wrong: None of the latter two arguments applies in this case, they are just rather general comments. On the contrary, having an in-editor search functionality realization that behaves equal across different tools (such as VS code and the Eclipse platform / IDE) is a good thing as both are good tools that can be used side-by-side and equal appearance and behavior of such a central functionality makes it easier for user to switch between the two. That's why I agree with @vogella that we should stick with the existing solution.

[Wittmaxi]

@vogella @HeikoKlare

I agree with you that his is not an issue big enough for us to get held up by it!
I have reverted it and changed the icons for an UTF8-Icon