Find/Replace overlay: add a search- and replace- history #1990

Wittmaxi · 2024-06-22T12:45:11Z

Adds a history feature for the find and replace bar of the overlay.
The history is displayed using a menu filled with entries from the
HistoryStore.

fixes #1907

Wittmaxi · 2024-06-22T12:48:09Z

@jukzi Can you please review this feature and tell me what you think of it? :)

jukzi · 2024-06-22T12:55:20Z

next week

Wittmaxi · 2024-06-22T13:07:05Z

@jukzi yes, no rush :)

github-actions · 2024-06-22T13:23:27Z

Test Results

1 815 files + 605 1 815 suites +605 1h 33m 1s ⏱️ + 30m 48s
7 677 tests ± 0 7 447 ✅ + 1 228 💤 - 3 2 ❌ +2
24 192 runs +8 064 23 440 ✅ +7 825 749 💤 +236 3 ❌ +3

For more details on these failures, see this check.

Results for commit b6e473d. ± Comparison against base commit be68f2f.

♻️ This comment has been updated with latest results.

iloveeclipse · 2024-06-22T14:46:17Z

Instead of the menu (or additionally to it), could we have ctrl+space for the list of proposals, filled with the history?

Wittmaxi · 2024-06-22T15:54:45Z

@iloveeclipse I think that could be a good idea! I was also thinking about allowing moving up/down the history using the arrow keys like vscode does. Ctrl+Space fits the "content assist" idiom and is also a very good idea!

HannesWell · 2024-06-22T19:50:39Z

Thank you for this. The screenshot already look promising, 🚀

Instead of the menu (or additionally to it), could we have ctrl+space for the list of proposals, filled with the history?

That's also an interesting proposal. But in order to make users aware of it, it should for example be mentioned in the empty-block background text (similar to the screenshot of VS code in #1907).
I'm not 100% sure if a content-assist behavior fits as trigger to show a search history, but maybe that's just based on what I'm used to, but in the end there is no right or wrong.

What also came into my mind when I read Andrey's proposal: The history could automatically drop-down as soon as the text-block gets the focus. Additionally the already typed text could then be used as 'filter' for the history and remove not matching items.
But I'm not sure if this is too much noise if one already knows what to type?

iloveeclipse · 2024-06-22T20:00:06Z

order to make users aware of it, it should for example be mentioned in the empty-block background text

Not necessarily, Eclipse has a concept of "light bulb" field decoration if there is something that provides extra functionality.

Wittmaxi · 2024-06-24T06:08:05Z

@HannesWell @iloveeclipse @jukzi before coming up with many new features, I would like to understand the problem at hand a little bit better. I never use the search history myself (even in the Dialog) and I couldn't come up with a workflow where the search history could be "really" useful. Can you please tell me a bit about how the search history is really used?

The history could automatically drop-down as soon as the text-block gets the focus.

I never use the search history, for me, this would just be (very) annoying.

jukzi · 2024-06-24T06:22:50Z

Can you please tell me a bit about how the search history is really used

example: i search a method in a .java file, but i am too lazy to type each letter of the method name. Therefore i copy paste a Stacktrace line "p.C.method ()" from VisualVm . Since the pasted line contains more then only the method name i have to delete the classname prefix "p.C" and the " ()" suffix within the search dialog. As soon as i have done that i will find a method and all its calls in the active editor. Now i continue with editing, searching other things and so on. Later i want to search the same method again - but still i don't know its name. I used to find its name in the search history. Now i only find it in the copy/paste history from windows but there it still contains prefix and suffix.

jukzi · 2024-06-24T07:02:55Z

I have tested it. Optically it is not beautiful but at least it works. Here are my nitpicks:

i expect that drop down to be below and aligned to the text field - as it was in the old search dialog:

old:
i expect that active UI components i.e. the drop down to have the highest possible contrast i.e. Black on white - as it was in the old search dialog
i expect that up and down keys can be used to scroll through the list - as it was in the old search dialog
i expect that i can close the list when i click on on the arrow (that is used to open the list) a second time - as it was in the old search dialog
compare to the old search dialog the list wastes much space in the front of the items

In summary i expected that the drop down would be exactly as in the old dialog, because that worked well and i dont know how it could be made better.

iloveeclipse · 2024-06-24T07:08:58Z

I never use the search history myself (even in the Dialog) and I couldn't come up with a workflow where the search history could be "really" useful. Can you please tell me a bit about how the search history is really used?

Trying to fix ~2000 test fails in not my own code.
Searching / replacing different constants / code with other constants / code, also using regex.
Mass search/replace doesn't work as code to replace relies on context
Gazillion of files opened, same patterns with different variations need to be replaced with other patterns with different variations.
Having search and replace field history allows me to search in same file for different occurrences from the history and replace it with different occurrences from the history.
Search (and replace) patterns are not trivial text and just typing it would be a constant pain, so history entries for both is essential.

Wittmaxi · 2024-06-24T07:19:32Z

@iloveeclipse @jukzi thank you very much for your user-stories! That helps me a lot understanding your concerns for the usability of the history.

It is now possible to navigate through the history using the arrow keys (arrow up/arrow down)

I will move on to address @jukzi s concerns.
I have implemented the search history using a menu, but it might be better to implement it as a shell overlaid on the Find/Replace overlay - what do you think?
In particular, I cannot use a Combo (like Find/Replace Dialog does) because it does not fit into the Overlay aesthetically and does also not support all required Operations (for example: it has no concept of a "message" which is the preview text shown when the text bar is empty)

Wittmaxi · 2024-06-24T09:48:47Z

@jukzi what do you think of this version?

jukzi · 2024-06-24T10:41:45Z

it looks good!

up/down arrows still not working
error: when i press down button again eclipse is send to background!

jukzi · 2024-06-24T10:50:55Z

another minor: in the bottom left of the eclipse window the progress shortly says "String xyz not found"

The word "String" is something a user does not need to understand. Better: "'xyz' not found"
"xyz NOT FOUND" is a status not a progress, that should not disappear within a tens of a second

--
up/down key work if and only if the replace field is visible.

jukzi · 2024-06-24T10:55:31Z

error loged:

org.eclipse.swt.SWTException: Widget is disposed
	at org.eclipse.swt.SWT.error(SWT.java:4922)
	at org.eclipse.swt.SWT.error(SWT.java:4837)
	at org.eclipse.swt.SWT.error(SWT.java:4808)
	at org.eclipse.swt.widgets.Widget.error(Widget.java:500)
	at org.eclipse.swt.widgets.Widget.checkWidget(Widget.java:419)
	at org.eclipse.swt.widgets.Control.isFocusControl(Control.java:1859)
	at org.eclipse.ui.texteditor.FindReplaceOverlay.lambda$0(FindReplaceOverlay.java:195)
	at org.eclipse.swt.events.KeyListener$1.keyPressed(KeyListener.java:66)
	at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:184)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:91)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4300)

Wittmaxi · 2024-06-24T11:22:07Z

Thank you for the report, I fixed the error here: 0b1b622

Can you please tell me what exactly you do to send eclipse to the background? I could not reproduce that.

Changing the status text is out of scope for this issue. I don't really think it's worth changing (and it is the "old" behavior), but if you think it is, we can create an Issue for it.

jukzi · 2024-06-24T11:42:08Z

Can you please tell me what exactly you do to send eclipse to the background? I could not reproduce that.

i am on WIndows. i have multiple programs open. I have replace line closed. I click on down-> history opens. I click on down again .> another program comes in front (feels like closing to the dropdown sends eclipse to background).

Wittmaxi · 2024-06-24T11:53:55Z

@jukzi yes, I can reproduce this! Thanks!
I will look into this

jukzi · 2024-06-25T08:41:04Z

now eclispe does not move to background anymore but again drop down can not be closed by second click.

jukzi · 2024-06-25T09:03:52Z

i can not scroll up/down the drop down with the keys when i use TAB+ENTER to open the drop down (instead the drop down closes)
right click on an entry in the drop down opens the context menu of the editor in the background (i was looking if i could remove a search entry from history)

Wittmaxi · 2024-07-01T09:54:35Z

@jukzi I have addressed all your concerns.
Allowing "right click" to remove the menu item is an interesting proposal but out of scope for now. If you want that option added, please create an issue for it :)

Wittmaxi · 2024-07-08T16:16:28Z

@jukzi @HeikoKlare I believe that we should discuss the "unused fields" later and prioritize getting feedback on the search history. I have created this issue for the unused fields #2060 and I have reverted the changes that made them unused.

I have also fixed the unit tests (at least locally) and removed a file which I forgot to remove while merging

HeikoKlare

In general, the code changes look good and sound. When testing, the UI also looks good and works quite well. I have rather nitpicky comments, but at least one issue (exception) that needs to be addressed:

Clicking "down" key when the history is empty results in an exception:

java.lang.ArithmeticException: / by zero
	at org.eclipse.ui.internal.findandreplace.overlay.HistoryTextWrapper.navigateInHistory(HistoryTextWrapper.java:119)
	at org.eclipse.ui.internal.findandreplace.overlay.HistoryTextWrapper.lambda$1(HistoryTextWrapper.java:75)

There seems to be an issue with the background color of a separator:

...kbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlay.java

...kbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/HistoryTextWrapper.java

Wittmaxi · 2024-07-15T10:23:17Z

I have fixed the divide by zero and the coloring of the separators now works correctly

HeikoKlare

Thank you for addressing my concerns! Regarding the code, I only have a minor comment left. For the images, we also need to have a PR with the svg to the platform.images repo.

I have two finding with respect to the UI / layout:

The text boxes are not properly centered (vertically) anymore. They are off to the top.

Before this PR:

After this PR:

On MacOS, the drop down menu is misplaced (off to the top):

HeikoKlare · 2024-07-15T16:38:40Z

...rkbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/SearchHistoryMenu.java

+	private TableColumn column;
+
+	public SearchHistoryMenu(Shell parent, HistoryStore history,
+			HistoryItemSelectedListener menuItemSelectionListener) {


It would be sufficient to use a general Consumer<String> instead of introducing a new (functional) interface here. If not, the listener should at least be declared as a functional interface, so that a lambda instead of an anonymous class can be passed as an argument to this constructor.

HeikoKlare · 2024-07-15T19:58:20Z

...kbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/HistoryTextWrapper.java

+		});
+
+		Point barPosition = textBar.toDisplay(0, 0);
+		menu.setPosition(barPosition.x, barPosition.y + dropDown.getWidth(), textBar.getSize().x + dropDown.getWidth());


I have investigated the MacOS placement issue and found the reason here:
This calculation does not work on MacOS, because the drop down's width returns a default value rather than the actual width. I am not sure whether that implementation is correct/expected, but since the implementation has always been like this, it cannot be changed anyway. Since the bounds contain the correct values, we can adapt the code as follows:

Rectangle dropDownBounds = dropDown.getBounds(); menu.setPosition(barPosition.x, barPosition.y + dropDownBounds.height, textBar.getSize().x + dropDownBounds.width);

Without the change, it looks like this:

With the change, it looks like this:

HeikoKlare · 2024-07-16T10:58:10Z

@Wittmaxi I have pushed my proposed changed onto your branch: https://github.com/eclipse-platform/eclipse.platform.ui/compare/c7042b7e8f9f3aec27f4d038fa10befb832d8214..9ff464d8c7f049d28d2fd8e12ff026051b2420f1

Maybe you can have a look whether that is fine for you, because I would like to merge this PR soon, so that we finally have that feature available and more easily find potential problems in productive use. If the changes are not okay, just let me know and I will revert them.

Wittmaxi · 2024-07-16T11:00:46Z

@HeikoKlare the changes look sound, especially the use of Consumer is much better than a custom object. I can't comment on the mac changes. If they work, that's fine.

thank you :)

HeikoKlare · 2024-07-16T11:08:43Z

Thank you for the fast feedback! :-)
With the latest change, the layout on MacOS is as expected (i.e., just like on Windows and Linux). Here are screenshots of the final version from all three OS:

Windows

MacOS

Linux

Add a search history for the Find/Replace overlay, displayed as a dropdown below the find/replace inputs. fixes eclipse-platform#1907

HeikoKlare · 2024-07-16T12:24:55Z

Test failures are unrelated: #294 #1941

Let's merge this and see how the histories behave in productive use. Thank you for contributing this, @Wittmaxi!

HeikoKlare · 2024-07-16T12:29:17Z

Can you please share the icon sources in the images repo, @Wittmaxi? It's not urgent, we should just not forget about that. I have created a tracking issue for that: eclipse-platform/eclipse.platform.images#83

And I just saw that the "scrolling hints" you added to the search and replace bar hints were removed in some of the recent commits. What that intended or by accident, so that we should apply that change in a follow-up?

FindReplaceOverlay_searchBar_message=Find (\u2195 for history)
FindReplaceOverlay_replaceBar_message=Replace (\u2195 for history)

Wittmaxi · 2024-07-16T12:35:58Z

@HeikoKlare That's not intended and should be re-added. Thanks for the catch :)

I can add an issue for that, I am not at the computer currently

HeikoKlare · 2024-07-16T13:34:07Z

I can add an issue for that, I am not at the computer currently

I have just created a quick follow-up PR: #2091
No need to have an issue for that.

HannesWell · 2024-07-26T16:56:50Z

Thank you for the adding the history to the new overlays. In general this works like charm.

The only thing that made me wonder when using this is that I have to press the UP key to navigate downwards the list of elements in the history. I found it confusing because when I open the full list its also a drop-down menu where the older elements are further down the list.
Also when I open the overlay and want to get the last search back, then I have to press the UP key.
Actually I expected the reverse key-binding: Use DOWN to get to older elements and UP to get newer ones. And also that I have to press DOWN to get my last search back in.

HeikoKlare · 2024-07-29T06:16:53Z

Thank you for the feedback, @HannesWell! Great to hear that, in general, the functionality works as expected.

I fully agree with your expectation regarding behavior of pressing the up/down keys. I am not sure whether this was ever intended. The only potential reason I found is the behavior of VS code, which moves "downwards" in the history by pressing the "up" button. In this case, I think we should deviate from the VS code behavior because (1) we also have a drop-down menu with an inverse order making the behavior inconsistent and (2) Eclipse users are used to the navigation direction because of the existing find/replace dialog. I have created a PR to invert the navigation direction: #2136

Wittmaxi force-pushed the MW_search_history branch 2 times, most recently from 074a7fd to 3b71076 Compare June 24, 2024 09:48

Wittmaxi force-pushed the MW_search_history branch from 3b71076 to 0b1b622 Compare June 24, 2024 11:20

Wittmaxi force-pushed the MW_search_history branch 2 times, most recently from b7caa6b to d1f08a9 Compare June 24, 2024 14:58

jukzi mentioned this pull request Jun 27, 2024

Find/Replace overlay: "String xyz not found" message #1993

Closed

Wittmaxi force-pushed the MW_search_history branch from d1f08a9 to 7032d6c Compare July 1, 2024 09:50

Wittmaxi force-pushed the MW_search_history branch from 4b6bd53 to 7c21528 Compare July 8, 2024 16:16

HeikoKlare reviewed Jul 8, 2024

View reviewed changes

Wittmaxi force-pushed the MW_search_history branch 5 times, most recently from c8049b4 to c7042b7 Compare July 15, 2024 10:20

Wittmaxi requested a review from HeikoKlare July 15, 2024 10:23

HeikoKlare reviewed Jul 15, 2024

View reviewed changes

HeikoKlare force-pushed the MW_search_history branch 2 times, most recently from 9ff464d to 02faa47 Compare July 16, 2024 10:56

Find/Replace Overlay: Add a search history

b6e473d

Add a search history for the Find/Replace overlay, displayed as a dropdown below the find/replace inputs. fixes eclipse-platform#1907

HeikoKlare force-pushed the MW_search_history branch from 02faa47 to b6e473d Compare July 16, 2024 11:21

HeikoKlare merged commit a162c72 into eclipse-platform:master Jul 16, 2024
13 of 16 checks passed

HeikoKlare mentioned this pull request Jul 16, 2024

Add images for FindReplaceOverlay history drop downs eclipse-platform/eclipse.platform.images#83

Open

This was referenced Jul 16, 2024

Add hints for history navigation to FindReplaceOverlay #2091

Merged

Find/Replace overlay: search and replace input fields not vertically centered #2092

Closed

HeikoKlare mentioned this pull request Jul 29, 2024

Find/replace overlay: invert keyboard navigation direction for history #2136

Merged

Find/Replace overlay: add a search- and replace- history #1990

Find/Replace overlay: add a search- and replace- history #1990

Conversation

Wittmaxi commented Jun 22, 2024 • edited Loading

Wittmaxi commented Jun 22, 2024

jukzi commented Jun 22, 2024

Wittmaxi commented Jun 22, 2024

github-actions bot commented Jun 22, 2024 • edited Loading

Test Results

iloveeclipse commented Jun 22, 2024

Wittmaxi commented Jun 22, 2024

HannesWell commented Jun 22, 2024

iloveeclipse commented Jun 22, 2024

Wittmaxi commented Jun 24, 2024

jukzi commented Jun 24, 2024

jukzi commented Jun 24, 2024

iloveeclipse commented Jun 24, 2024

Wittmaxi commented Jun 24, 2024

Wittmaxi commented Jun 24, 2024

jukzi commented Jun 24, 2024

jukzi commented Jun 24, 2024 • edited Loading

jukzi commented Jun 24, 2024

Wittmaxi commented Jun 24, 2024

jukzi commented Jun 24, 2024

Wittmaxi commented Jun 24, 2024

jukzi commented Jun 25, 2024

jukzi commented Jun 25, 2024

Wittmaxi commented Jul 1, 2024

Wittmaxi commented Jul 8, 2024

HeikoKlare left a comment

Choose a reason for hiding this comment

Wittmaxi commented Jul 15, 2024

HeikoKlare left a comment • edited Loading

Choose a reason for hiding this comment

HeikoKlare Jul 15, 2024

Choose a reason for hiding this comment

HeikoKlare Jul 15, 2024

Choose a reason for hiding this comment

HeikoKlare commented Jul 16, 2024

Wittmaxi commented Jul 16, 2024

HeikoKlare commented Jul 16, 2024 • edited Loading

Windows

MacOS

Linux

HeikoKlare commented Jul 16, 2024

HeikoKlare commented Jul 16, 2024

Wittmaxi commented Jul 16, 2024

HeikoKlare commented Jul 16, 2024

HannesWell commented Jul 26, 2024

HeikoKlare commented Jul 29, 2024

Wittmaxi commented Jun 22, 2024 •

edited

Loading

github-actions bot commented Jun 22, 2024 •

edited

Loading

jukzi commented Jun 24, 2024 •

edited

Loading

HeikoKlare left a comment •

edited

Loading

HeikoKlare commented Jul 16, 2024 •

edited

Loading