Use consistent key bindings in FindReplaceOverlay on MacOS #2007 #2008

HeikoKlare · 2024-06-28T13:24:16Z

Currently, the opening action for the find/replace overlay uses a key binding with the OS-dependent "modifier 1" (mapping to CTRL on Windows and CMD on MacOS), while all key bindings within the overlay (such as activating search options) use key bindings with CTRL as a modifier.

In order to unify the key bindings, this change adapts the key bindings within the overlay to also use the OS-dependent "modifier 1", such that on MacOS the CMD modifier is used. The change also adapts the tooltips to (1) reflect the changed keys and (2) represent the MacOS modifier keys via icons like at all other places.

Fixes #2007

HeikoKlare · 2024-06-28T13:26:26Z

@BeckerWdf Can you confirm that these changed key bindings make sense on MacOS?

Example

Current

Open find/replace overlay: CMD+F
Move focus to find input field within find/replace overlay: CTRL+F
Activate whole word search: CTRL+W

New

Open find/replace overlay: CMD+F
Move focus to find input field within find/replace overlay: CMD+F
Activate whole word search: CMD+W

BeckerWdf · 2024-06-28T13:44:50Z

Activate whole word search: CMD+W

With your patch it's CMD-Shift-W. But the tooltip still says CTRL-W

BeckerWdf · 2024-06-28T13:48:43Z

and btw:
in the UI CMD should be rendered with the CMD-Icon like e.g. in the menu entries:

Can these bindings be changes on the "Keys" preference page?

github-actions · 2024-06-28T14:05:01Z

Test Results

1 210 files - 605 1 210 suites - 605 1h 0m 24s ⏱️ - 32m 2s
7 668 tests ± 0 7 436 ✅ - 4 231 💤 + 3 1 ❌ +1
16 110 runs - 8 055 15 596 ✅ - 7 820 513 💤 - 236 1 ❌ +1

For more details on these failures, see this check.

Results for commit ca7b586. ± Comparison against base commit 337f4ba.

This pull request skips 3 tests.

UiTestSuite org.eclipse.ui.tests.api.ApiTestSuite org.eclipse.ui.tests.api.WorkbenchPluginTest ‑ testGetImageRegistryFromAdditionalDisplay
org.eclipse.jface.text.tests.contentassist.ContextInformationTest ‑ testContextInfo_hide_focusOut
org.eclipse.urischeme.internal.registration.TestUnitWinRegistry ‑ testWinRegistry

♻️ This comment has been updated with latest results.

HeikoKlare · 2024-06-28T14:13:10Z

You are right, the tooltip has to be adapted as well, thanks! I think we should try to adapt the tooltip to use the proper modifier icon like in the menus. I will check that.

But do you agree with me that, in general, CMD instead of CTRL should be used on MacOS for these shortcuts?

Can these bindings be changes on the "Keys" preference page?

I don't think so, they are currently hard coded.

BeckerWdf · 2024-06-28T14:18:27Z

But do you agree with me that, in general, CMD instead of CTRL should be used on MacOS for these shortcuts?

Yes.

I don't think so, they are currently hard coded.

I think this should also be changed.

Wittmaxi · 2024-07-01T11:51:39Z

@BeckerWdf I am not sure if that is sensible - the keybindings are not actions like the actions which are targetted by the settings of the "keys" page. Maybe the keybindings should be editable somewhere else?

BeckerWdf · 2024-07-01T11:56:40Z

@BeckerWdf I am not sure if that is sensible - the keybindings are not actions like the actions which are targetted by the settings of the "keys" page.
Is it a must to be an action to be on the "keys" pref page?

Maybe the keybindings should be editable somewhere else?

Pls. don't a second possibility to configure shortcuts.

Wittmaxi · 2024-07-01T12:02:10Z

Pls. don't a second possibility to configure shortcuts.

I understand! Then let's use the "keys" page.

Is it a must to be an action to be on the "keys" pref page?

I am not really sure - from my understanding yes, but my understanding is extremely limited! Maybe somebody can shed light on this?

HeikoKlare · 2024-07-08T16:49:50Z

We have extracted the custom binding of keys into a separate issue: #2015.

I have adapted this PR so that tooltips show the OS-dependent shortcuts and also use modifier icons on MacOS to be consistent with the presentation of shortcuts at other places (in particular, menus).

BeckerWdf · 2024-07-09T10:32:22Z

I have some improvement suggestion regarding the formatting of the keybindings. It's up to you if you want to consider this or not.

HeikoKlare · 2024-07-09T11:40:55Z

I have some improvement suggestion regarding the formatting of the keybindings. It's up to you if you want to consider this or not.

Improvement suggestions are always appreciated 🙂 I will gladly adopt them.
@BeckerWdf Can you share those suggestions?

BeckerWdf · 2024-07-09T10:31:32Z

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

@@ -70,6 +74,8 @@
 * @since 3.17
 */
 public class FindReplaceOverlay extends Dialog {
+	private static final String MOD1_KEY_STRING = OS.isMac() ? "⌘" : "Ctrl+"; //$NON-NLS-1$//$NON-NLS-2$


There's a util that does that for you:
See: org.eclipse.jface.bindings.key.KeySequence#org.eclipse.jface.bindings.key

This is also used for formatting they shortcuts in the empty text editor's onboarding implementation.
See: ModeledPageLayout#addEditorOnboardingCommandId

That looks great, thank you! I will try to adapt this PR using that functionality.

BeckerWdf · 2024-07-09T11:42:24Z

@BeckerWdf Can you share those suggestions?

Ups....

BeckerWdf

Works fine and looks good

Wittmaxi

Very Sensible way of extracting keyboard shortcuts from the handler code and at the same time address the formatting of the shortcuts! Thanks 😀

Wittmaxi · 2024-07-11T12:17:56Z

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

 		searchUpButton = new AccessibleToolItemBuilder(searchTools).withStyleBits(SWT.PUSH)
 				.withImage(FindReplaceOverlayImages.get(FindReplaceOverlayImages.KEY_FIND_PREV))
-				.withToolTipText(FindReplaceMessages.FindReplaceOverlay_upSearchButton_toolTip)
-				.withSelectionListener(SelectionListener.widgetSelectedAdapter(e -> performSearch(false))).build();
+				.withToolTipText(MessageFormat.format(FindReplaceMessages.FindReplaceOverlay_upSearchButton_toolTip,


Nit: How about extracting the formatting of tooltips into a "addShortcutToToolTipText" method?

Maybe even add a "withShortcutHint" method to the ToolItemBuilder, which takes a shortcut and adds it to the ToolTip?

Good point! I have added a helper method in ca7b586.
Integrating the shortcut into the tool item (or its builder) would be even better. I am already working on a follow-up cleanup that better integrates shortcuts with operations, as well as the operations assigned to shortcuts and "usual" item clicks, which will cover that proposal in some way. Currently, there is quite some manual effort to initialize everything consistently. But that should not prevent us from having this improvement merged and is also easier to review in separation.

…atform#2007 Currently, the opening action for the find/replace overlay uses a key binding with the OS-dependent "modifier 1" (mapping to CTRL on Windows/Linux and CMD on MacOS), while all key bindings within the overlay (such as activating search options) use key bindings with CTRL as a modifier. In order to unify the key bindings, this change adapts the key bindings within the overlay to also use the OS-dependent "modifier 1", such that on MacOS the CMD modifier is used. The change also adapts the shortcuts to be represented as KeyStroke instances and uses them to match actual key combinations as well as to produce the string representation of the shortcuts for the tooltips. Fixes eclipse-platform#2007

Wittmaxi mentioned this pull request Jul 1, 2024

Find/Replace Overlay: Allow rebinding shortcuts #2015

Open

HeikoKlare force-pushed the issue-2007 branch 3 times, most recently from d2a5ac3 to 66e7f9d Compare July 8, 2024 10:49

HeikoKlare marked this pull request as ready for review July 8, 2024 16:49

BeckerWdf reviewed Jul 9, 2024

View reviewed changes

HeikoKlare marked this pull request as draft July 9, 2024 11:44

HeikoKlare force-pushed the issue-2007 branch 4 times, most recently from b2c0218 to 2445c9f Compare July 9, 2024 19:19

HeikoKlare marked this pull request as ready for review July 9, 2024 20:08

BeckerWdf approved these changes Jul 10, 2024

View reviewed changes

HeikoKlare force-pushed the issue-2007 branch from 2445c9f to 3648bbf Compare July 11, 2024 11:54

Wittmaxi approved these changes Jul 11, 2024

View reviewed changes

HeikoKlare force-pushed the issue-2007 branch from 3648bbf to ca7b586 Compare July 12, 2024 06:09

HeikoKlare merged commit 4888a8c into eclipse-platform:master Jul 12, 2024
15 of 16 checks passed

HeikoKlare deleted the issue-2007 branch July 12, 2024 08:02

Wittmaxi approved these changes Jul 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use consistent key bindings in FindReplaceOverlay on MacOS #2007 #2008

Use consistent key bindings in FindReplaceOverlay on MacOS #2007 #2008

HeikoKlare commented Jun 28, 2024 •

edited

Loading

HeikoKlare commented Jun 28, 2024

BeckerWdf commented Jun 28, 2024

BeckerWdf commented Jun 28, 2024

github-actions bot commented Jun 28, 2024 •

edited

Loading

HeikoKlare commented Jun 28, 2024

BeckerWdf commented Jun 28, 2024

Wittmaxi commented Jul 1, 2024

BeckerWdf commented Jul 1, 2024

Wittmaxi commented Jul 1, 2024 •

edited

Loading

HeikoKlare commented Jul 8, 2024

BeckerWdf commented Jul 9, 2024

HeikoKlare commented Jul 9, 2024

BeckerWdf Jul 9, 2024

HeikoKlare Jul 9, 2024

BeckerWdf commented Jul 9, 2024

BeckerWdf left a comment

Wittmaxi left a comment •

edited

Loading

Wittmaxi Jul 11, 2024

Wittmaxi Jul 11, 2024

HeikoKlare Jul 12, 2024

Use consistent key bindings in FindReplaceOverlay on MacOS #2007 #2008

Use consistent key bindings in FindReplaceOverlay on MacOS #2007 #2008

Conversation

HeikoKlare commented Jun 28, 2024 • edited Loading

HeikoKlare commented Jun 28, 2024

Example

Current

New

BeckerWdf commented Jun 28, 2024

BeckerWdf commented Jun 28, 2024

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

Test Results

HeikoKlare commented Jun 28, 2024

BeckerWdf commented Jun 28, 2024

Wittmaxi commented Jul 1, 2024

BeckerWdf commented Jul 1, 2024

Wittmaxi commented Jul 1, 2024 • edited Loading

HeikoKlare commented Jul 8, 2024

BeckerWdf commented Jul 9, 2024

HeikoKlare commented Jul 9, 2024

BeckerWdf Jul 9, 2024

Choose a reason for hiding this comment

HeikoKlare Jul 9, 2024

Choose a reason for hiding this comment

BeckerWdf commented Jul 9, 2024

BeckerWdf left a comment

Choose a reason for hiding this comment

Wittmaxi left a comment • edited Loading

Choose a reason for hiding this comment

Wittmaxi Jul 11, 2024

Choose a reason for hiding this comment

Wittmaxi Jul 11, 2024

Choose a reason for hiding this comment

HeikoKlare Jul 12, 2024

Choose a reason for hiding this comment

HeikoKlare commented Jun 28, 2024 •

edited

Loading

github-actions bot commented Jun 28, 2024 •

edited

Loading

Wittmaxi commented Jul 1, 2024 •

edited

Loading

Wittmaxi left a comment •

edited

Loading