Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Modern Find/Replace] Implemented Find-Replace Overlay #1192

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

Wittmaxi
Copy link

@Wittmaxi Wittmaxi commented Oct 5, 2023

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

@Wittmaxi Wittmaxi changed the title Implemented Find-Replace Overlay. [Modern Find/Replace] Implemented Find-Replace Overlay Oct 5, 2023
@Wittmaxi Wittmaxi marked this pull request as draft October 5, 2023 09:45
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

Test Results

 1 812 files   1 812 suites   1h 35m 0s ⏱️
 7 634 tests  7 405 ✅ 228 💤 1 ❌
24 063 runs  23 313 ✅ 749 💤 1 ❌

For more details on these failures, see this check.

Results for commit 3a2e413.

♻️ This comment has been updated with latest results.

@Wittmaxi Wittmaxi force-pushed the MW_FindReplace_Overlay branch from b5cd48f to 659b102 Compare October 5, 2023 10:52
@Wittmaxi Wittmaxi marked this pull request as ready for review October 12, 2023 07:54
@jukzi
Copy link
Contributor

jukzi commented Oct 24, 2023

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
    image
  7. after resizing the input field is sometimes distorted / text drawn to much left
    image
  8. it currently doe not work well in console view (wrong location, too small, +/- not working):
    image
  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
Copy link
Author

Thank you for your feedback, @jukzi.

In particular, 9) is related to this issue.

@jukzi
Copy link
Contributor

jukzi commented Oct 24, 2023

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

@vogella
Copy link
Contributor

vogella commented Nov 7, 2023

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

@HeikoKlare
Copy link
Contributor

@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 Wittmaxi force-pushed the MW_FindReplace_Overlay branch from 659b102 to 0455efe Compare November 7, 2023 11:12
@Wittmaxi Wittmaxi force-pushed the MW_FindReplace_Overlay branch from 0455efe to fae805d Compare December 8, 2023 14:35
@Wittmaxi
Copy link
Author

Wittmaxi commented Dec 8, 2023

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

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

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

@vogella
Copy link
Contributor

vogella commented Jan 18, 2024

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

@HeikoKlare
Copy link
Contributor

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 Wittmaxi force-pushed the MW_FindReplace_Overlay branch from 0e9bd7c to 948e57f Compare January 23, 2024 16:27
@Wittmaxi
Copy link
Author

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 Wittmaxi force-pushed the MW_FindReplace_Overlay branch 2 times, most recently from 4e2da58 to 6fb672b Compare January 23, 2024 17:16
@Wittmaxi
Copy link
Author

Wittmaxi commented Jan 23, 2024

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

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

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

@Wittmaxi Wittmaxi force-pushed the MW_FindReplace_Overlay branch 5 times, most recently from dc416a8 to 8910b98 Compare January 30, 2024 10:52
@vogella vogella force-pushed the MW_FindReplace_Overlay branch from 21f43d3 to 3a2e413 Compare June 7, 2024 10:07
@vogella
Copy link
Contributor

vogella commented Jun 7, 2024

Rebased onto the next version bump. Fingers crossed again...

@vogella
Copy link
Contributor

vogella commented Jun 7, 2024

@Wittmaxi org.eclipse.search.tests has test errors under Windows. Can you have a look?

@Wittmaxi
Copy link
Author

Wittmaxi commented Jun 7, 2024

@vogella I didn't write or modify this test, it is not related to my PR and I am very confident that the fail isn't a side-effect of my pr.

However, I could not find it in the issues - I have documented the random fail here: #1941

I cannot access my computer today, thank you for helping (finally!) merge this PR 😀

@vogella
Copy link
Contributor

vogella commented Jun 7, 2024

Thanks @Wittmaxi for documenting the failing test #1941

I merge this change now. Thanks for all your work @Wittmaxi and also thanks to @HeikoKlare and all others for reviewing this change very, very carefully.

@vogella vogella merged commit 6d6465b into eclipse-platform:master Jun 7, 2024
14 of 16 checks passed
@merks
Copy link
Contributor

merks commented Jun 7, 2024

It's cool! And EMF's use of the find dialog still works too:

image

image

So cool new stuff works and old stuff also still works. Well done!!

@HannesWell
Copy link
Member

Tried this out yesterday for the first time and it's really nice.

Can you please create a new and noteworthy for this new feature in https://github.com/eclipse-platform/www.eclipse.org-eclipse ?

Wittmaxi pushed a commit to Wittmaxi/www.eclipse.org-eclipse that referenced this pull request Jun 17, 2024
Wittmaxi pushed a commit to Wittmaxi/eclipse.platform.images that referenced this pull request Jun 22, 2024
Add the images used in the editor overlay used to perform find/replace.
The images are only added in svg-format whereas they are used in
png-format in the Overlay.

In particular, a few new images were created and/or adapted from
www.iconbolt.com.

This PR acommodates
eclipse-platform/eclipse.platform.ui#1192
Wittmaxi pushed a commit to Wittmaxi/eclipse.platform.images that referenced this pull request Jun 22, 2024
Add the images used in the editor overlay used to perform find/replace.
The images are only added in svg-format whereas they are used in
png-format in the Overlay.

In particular, a few new images were created and/or adapted from
www.iconbolt.com.

This PR acommodates
eclipse-platform/eclipse.platform.ui#1192
Wittmaxi pushed a commit to Wittmaxi/www.eclipse.org-eclipse that referenced this pull request Jun 22, 2024
Wittmaxi pushed a commit to Wittmaxi/www.eclipse.org-eclipse that referenced this pull request Jun 22, 2024
BeckerWdf pushed a commit to eclipse-platform/eclipse.platform.images that referenced this pull request Jun 22, 2024
Add the images used in the editor overlay used to perform find/replace.
The images are only added in svg-format whereas they are used in
png-format in the Overlay.

In particular, a few new images were created and/or adapted from
www.iconbolt.com.

This PR acommodates
eclipse-platform/eclipse.platform.ui#1192
Wittmaxi pushed a commit to Wittmaxi/www.eclipse.org-eclipse that referenced this pull request Jul 1, 2024
HeikoKlare pushed a commit to Wittmaxi/www.eclipse.org-eclipse that referenced this pull request Aug 6, 2024
HeikoKlare pushed a commit to Wittmaxi/www.eclipse.org-eclipse that referenced this pull request Aug 6, 2024
HeikoKlare pushed a commit to Wittmaxi/www.eclipse.org-eclipse that referenced this pull request Aug 7, 2024
HeikoKlare pushed a commit to eclipse-platform/www.eclipse.org-eclipse that referenced this pull request Aug 7, 2024
@jukzi
Copy link
Contributor

jukzi commented Aug 13, 2024

@Wittmaxi that message box looks somehow wrong. I guess it's the missing border and white bar at the bottom that makes it look ugly (win, Light default theme):
image

@HeikoKlare
Copy link
Contributor

For the record: reported issue is addressed via #2188

@grendel151
Copy link

Not sure if this belongs here, or somewhere else.

The big problem I have with the replacement of the find modal dialog is finding things in other editors. I note that a reason for the change is that it wasn't always clear what the find dialog was bound to find in, but I used that a lot.

Maybe I'm one data point, but my anecdote of workflow of what I lost is that when working on two files at the same time side by side that are related, I will often highlight a method/variable/something I want to find in the other editor and hit ctrl-f. This opens the find dialog with the desired words already in the dialog. I then click in the other window and hit enter, and it finds it.

Now I think I have to copy the text to the clip board, go to the other editor, ctrl-f, ctrl-v to try to find in the other window. I also actually find it awkward that there are two dialogs open, one for each window and that they can be finding multiple things?

The more I think about it, the finding multiple things might be more useful than my workflow? I should try the new options more I guess.

Maybe an actual suggestion would be an option for "find in visible editors" ?

A second comment. Another reason posted for the change I see is that the find dialog could pop up over things and hide them. Yes, it could. And being a dialog, you can move it. And I moved it frequently. I can't move the overlay. If it's covering something up, I have fewer options for doing something about it.

I don't know. I'm on the fence about it. I'm back to the old dialog for now. I haven't liked the removal of words from the windows right click dialog, and this feels like more of the same along that vein.

@HeikoKlare
Copy link
Contributor

Thank you for your feedback, @grendel151! You are absolutely right that the described workflow (searching for same/related things in different editors, side by side) is something that is far better supported with the existing dialog than (currently) with the overlay. On the contrary, the overlay better supports document-specific search settings. Obviously, none of the workflows is "better" than the other, only some people may face one of them more often than the other.

Note that the overlay is not "finished" but still actively worked on, as in-document searching is an essential task during developing, so all relevant workflows should be supported in an efficient way. This is the according umbrella issue:

There is also a specific issue covering the sharing search settings across multiple documents, which is highly related to your concern:

So we definitely think about ways of providing better support for the described workflow, such as some explicit option to enable a "sharing mode" in which search settings are always propagated to all open editors. Feel free to also post your needs/ideas to those specific issues, so that we can properly address them.

@jukzi
Copy link
Contributor

jukzi commented Oct 3, 2024

Chrome browser for example reuses the search field of another tab when openend in a new tab.

@Gamebuster19901
Copy link

How do I revert back to the old search box? I just don't like the new search box. I have a 4k monitor and this thing is tiny. It's also not clear to me how I am even supposed to use it anyway.

image

@Gamebuster19901
Copy link

Figured it out. If anyone else wants to know how to disable it:

On the top of the window, go to [Window] -> [Preferences]
Then on the left sidebar, navigate to [Text Editors]
image

Then uncheck [Use find/replace overlay]

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.