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

FindReplaceOverlay: Add "skip replace" button #2507

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Wittmaxi
Copy link

@Wittmaxi Wittmaxi commented Nov 8, 2024

support the workflow in which a user selectively wants to replace multiple occurrences but needs to check first which to replace. Adds a button associated to a shortcut which - when triggered - simply jumps to the next occurrence of the search term.

This PR is a test to see whether this is something that adds user value. Adding this button is slightly awkward since it does exactly the same thing as "find next" - except that it can be reached by shortcut (Ctrl+Shift+O) from the Replace bar.

If this PR is approved (help wanted!), these are the next steps:

  • agree on an icon
  • publish the icon on the images repo
  • write a unit test

support the workflow in which a user selectively wants to replace
multiple occurrences but needs to check first which to replace.
Adds a button associated to a shortcut which - when triggered - simply
jumps to the next occurrence of the search term.
@Wittmaxi Wittmaxi added help wanted Extra attention is needed usability labels Nov 8, 2024
@Wittmaxi
Copy link
Author

Wittmaxi commented Nov 8, 2024

Screencast.from.2024-11-08.15-06-50.webm

I don't like the icon yet. Any suggestions?image

@Wittmaxi
Copy link
Author

Wittmaxi commented Nov 8, 2024

@stephan-herrmann you proposed this in #2473.
Is this something that addresses your concern?

Copy link
Contributor

github-actions bot commented Nov 8, 2024

Test Results

   907 files   -   607     907 suites   - 607   31m 44s ⏱️ - 1h 3m 15s
 7 725 tests ±    0   7 496 ✅ +    1  229 💤 ±  0  0 ❌  - 1 
14 378 runs   - 8 112  14 110 ✅  - 7 831  268 💤  - 280  0 ❌  - 1 

Results for commit 8a48723. ± Comparison against base commit 10d96b5.

@HeikoKlare
Copy link
Contributor

I have to admit that I am not in favor of the proposed change. This is because the solution does not address the essence of the actual issue. The solution adds another button (to a user interface that is/was supposed to be lean), while there is already a button doing exactly the same thing. If I understand correctly, it is just there to provide an additional keyboard shortcut.

Would it, e.g., be better to adapt the shortcuts assigned to the existing buttons depending on which of the input fields has foucs? I am not completely convinced that that's a perfect solution (as I see changing shortcuts as conflicting to good UX in terms of expectability), but since the meaning of shortcuts is context dependent already now ("Enter" does different things depending on whether the find or replace input field has focus), that might be a reasonable alternative.
So concretely: the find/replace buttons have the current shortcuts assigned whenever the according input field has focus (which is exactly the behavior we have right now). When the other input field has focus, the shortcuts are changed, e.g., by adding some modifier, such that MODIFIED+Enter executes a search when the replace input field has focus, or executes a replace when the find input field has focus.

Note that the shortcut currently assigned to the new button has no effect on Windows (at least in my development SDK).

@stephan-herrmann
Copy link
Contributor

@stephan-herrmann you proposed this in #2473. Is this something that addresses your concern?

Sorry for late reply. Rethinking, I agree with @HeikoKlare that not a new button is needed, but some change to keybindings.

@HeikoKlare you mentioned Ctrl+K for find next, since which version is this implemented? (doesn't work for me in 2024-09, as long as the overlay is shown). Actually without consulting any "manual" I would expect one of these: F3 (like in several other applications, but already used for "Find Declaration") or repeated Ctrl+F (already used for hiding the overlay, but that's redundant with Esc, right?).

@Wittmaxi
Copy link
Author

@HeikoKlare I agree with you that adding this button is "awkward". I feel like changing keybindings when the input field is switched might be confusing (this retakes us to the "which bar is active right now?" question)

I tend to gravitate towards @stephan-herrmann 's new idea of having one "power-button" which always performs a search, no matter where you are in the find/replace dialog. Ctrl+F is the logical choice. I will implement a demo when I have slightly more time

@HeikoKlare
Copy link
Contributor

Having a general button performing a search sounds reasonable to me. In my opinion, the natural choice for this would be whatever shortcut is bound to the "Find Next" action. Usually, that is bound to CTRL+K. @stephan-herrmann that action has been there for quite a while, but currently it is not accessible from within the overlay. An option could be to just provide this action from within the overlay. Currently it is automatically deactivated when opening the overlay together with many other shortcuts that you do not want to trigger for the editor from within the overlay (such as deleting a line).

I feel like changing keybindings when the input field is switched might be confusing (this retakes us to the "which bar is active right now?" question)

Note that this is kind of already given right now: the shortcuts ENTER and CTRL+ENTER are bound to different actions, depending on whether the find or replace input field has focus. So that would actually not be different.

having one "power-button" which always performs a search, no matter where you are in the find/replace dialog. Ctrl+F is the logical choice.

Please do not use the CTRL+F shortcut for this. That shortcut is already bound in every situation: it opens the overlay, when the find input field has focus, it closes the overlay, and when anything else has focus (in particular the replace input field) it moves focus to the find input field. So using it to search when the replace input field has focus does not work without breaking existing (and in my opinion reasonable) behavior. In addition, it would further overload that shortcut, making it hard to comprehend how it behaves in different situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed usability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants