Skip to content

Conversation

@Wittmaxi
Copy link

@Wittmaxi Wittmaxi commented Sep 2, 2024

In order to preserve the workflow of users who use the Mnemonics of the Find/Replace Dialog a lot, add support to these key combinations as well

fixes #2080

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2024

Test Results

0 files   -  1 815  0 suites   - 1 815   0s ⏱️ - 1h 37m 11s
0 tests  -  7 696  0 ✅  -  7 468  0 💤  - 228  0 ❌ ±0 
0 runs   - 24 249  0 ✅  - 23 500  0 💤  - 749  0 ❌ ±0 

Results for commit 7c69e19. ± Comparison against base commit b96413b.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, it's great to have the existing mnemonics of the dialog supported in the overlay as well. My only concern is that they derivate from existing behavior in the sense that for the search options/action they are only available when the search input field has focus, while the replace actions are only available when the replace input field has focus. So when the focus is anywhere else (any toolbar item or the other input field), the shortcuts are not available. This was already a limitation before, but it somehow becomes worse when readding the existing mnemonics, as they were dialog-unique and accessible from everywhere in the dialog. Maybe we should extend the design to also support that case?

private static final List<KeyStroke> SEARCH_FORWARD = List.of( //
KeyStroke.getInstance(SWT.CR), KeyStroke.getInstance(SWT.KEYPAD_CR));
KeyStroke.getInstance(SWT.CR), KeyStroke.getInstance(SWT.KEYPAD_CR),
KeyStroke.getInstance(SWT.ALT, 'n'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the dialog, all the added bindings also work when having the shift key pressed. Maybe an additional KeyStroke instance should be added for that combination as well?

private static final List<KeyStroke> OPTION_REGEX = List.of( //
KeyStroke.getInstance(SWT.MOD1 | SWT.SHIFT, 'P'), KeyStroke.getInstance(SWT.MOD1 | SWT.SHIFT, 'p'));
KeyStroke.getInstance(SWT.MOD1 | SWT.SHIFT, 'P'), KeyStroke.getInstance(SWT.MOD1 | SWT.SHIFT, 'p'),
KeyStroke.getInstance(SWT.ALT, 'X'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be 'x' instead of 'X'.

@Wittmaxi
Copy link
Author

After merging into the FindReplaceOverlay which is drawn as a composite inside of the page, some of the "alt+..." keybindings don't work anymore, and trigger a global action instead (e.g. alt+s opens "source" from the menu bar at the top).
Some mnemonics still work.

I believe this is a reason to drop this feature all together.

  1. we don't communicate to any user which mnemonics exists, this is merely a help to old users who are used to the old mnemonics
  2. choosing new mnemonics would not help in our case since we don't communicate to the user which mnemonics they can expect to work

This is the table of mnemonics which work:

Action Has Mnemonic? Mnemonic Key(s)
SEARCH_FORWARD No -
SEARCH_BACKWARD No -
SEARCH_ALL Yes ALT + S
REPLACE_ALL No -
OPTION_CASE_SENSITIVE Yes ALT + C
OPTION_WHOLE_WORD No -
OPTION_REGEX No -
OPTION_SEARCH_IN_SELECTION Yes ALT + L
CLOSE No -
TOGGLE_REPLACE No -

I have updated my PR with a subset of all the mnemonics - every registered mnemonic still works.

In order to preserve the workflow of users who use the Mnemonics of the
Find/Replace Dialog a lot, add support to these key combinations as well

fixes eclipse-platform#2080
@HeikoKlare
Copy link
Contributor

I believe this is a reason to drop this feature all together.

I agree with this. In particular, now the overlay is not a separate dialog with its completely independent shortcut processing anymore. Shortcuts defined for the editor scope will apply, such that depending on the individual configuration different shortcuts may be conflicting. E.g., configuring some editor action for shortcut ALT+C would lead to a conflict with the support for the find/replace dialog mnemonic to toggle case-sensitive search. This also demands for making even the current shortcuts for the overlay configurable so that conflicting shortcuts are detected/avoided.
In summary, I propose to not provide any implicit shortcuts just for "backwards compatibliity" but to only support the explicitly defined shortcuts that are also mentioned in the tooltips (and make them configurable).

@Wittmaxi
Copy link
Author

@HeikoKlare in this case, I am now closing this PR

@Wittmaxi Wittmaxi closed this Oct 16, 2024
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.

Users of the Find/Replace dialog want to use Alt + A to perform "Search all"

2 participants