Make find-and-replace bar accept Ctrl/Cmd + Alt modifier to Replace All#88846
Make find-and-replace bar accept Ctrl/Cmd + Alt modifier to Replace All#88846MajorMcDoom wants to merge 2 commits intogodotengine:masterfrom
Ctrl/Cmd + Alt modifier to Replace All#88846Conversation
|
The fact that it's hardcoded and provides no way of knowing about it is a bit... "bad" in my opinion. "Replace All" is mapped to |
There are quite a few instances of this in Godot, unfortunately. I'm not sure what the best approach would be here though. I don't know if Godot has some standards for showing instructions. |
|
The least that could be done is a tooltip on the LineEdit itself, perhaps. That's the most "consistent" standard when it comes to showing additional inputs. I do agree that modifiers on a submission often feel natural and may be discoverable by trial & error, but yeah. Just... yeah... The editor isn't known for being consistent. |
|
I'd prefer to use Ctrl + Alt + Enter as a dedicated shortcut. Requiring two modifiers to be held makes it a lot harder to accidentally input it. |
MajorMcDoom
changed the title
Ctrl/Cmd modifier to Replace AllCtrl/Cmd + Alt modifier to Replace All
MajorMcDoom
force-pushed
the
find-replace-all-shortcut
branch
2 times, most recently
from
920ce52 to
be3bb26
Compare
|
Okay, I've changed it to |
MajorMcDoom
force-pushed
the
find-replace-all-shortcut
branch
from
be3bb26 to
6f0f2aa
Compare
|
|
||
| void FindReplaceBar::_replace_text_submitted(const String &p_text) { | ||
| if (selection_only->is_pressed() && text_editor->has_selection(0)) { | ||
| if ((Input::get_singleton()->is_key_pressed(Key::CMD_OR_CTRL) && Input::get_singleton()->is_key_pressed(Key::ALT)) || (selection_only->is_pressed() && text_editor->has_selection(0))) { |
There was a problem hiding this comment.
The feature makes sense, but this isn't the right way to add shortcuts.
BaseButton has an API for this already, which should be usable here, a priori (didn't test).
This lets you register a shortcut, with a property name so it can be customized by the user, and it gets added to a tooltip automatically with the proper localization. If the shortcut is pressed, the button is pressed and triggers its callback.
In the constructor:
replace_all->set_shortcut(ED_SHORTCUT("script_editor/replace_all_occurrences", TTR("Replace All Occurrences"), KeyModifierMask::CMD_OR_CTRL | KeyModifierMask::ALT | Key::ENTER);There was a problem hiding this comment.
Thanks for this! As a button on its own with a shortcut, I'd do this with no hesitation, but I do have to raise the question of what to do with the rest of the interactions that don't have associated buttons?
e.g. Shift-Enter to search / replace backwards?
And also, the plain Enter isn't a mapping for a search / replace shortcut either - it's just a submission from a TextEdit.
Is this asymmetry okay?
There was a problem hiding this comment.
The actions which don't have an associated button should rely on ui_ input actions in the InputMap, and possibly add new ones if they're not covered already.
But there are indeed still a bunch of places where we hardcode specific keys instead of relying on action, I don't know if it's an oversight (UI actions for stuff like TextEdit were added recently), or if there's a good reason for it.
CC @KoBeWi
There was a problem hiding this comment.
For the time being, should I add this shortcut as described by @akien-mga, without any further modifications to the other interactions?
There was a problem hiding this comment.
Unfortunately the shortcut only works when none of the text fields have focus. When one of them has focus (which it almost always does when this shortcut is to be used), the text submission shortcut takes priority. Any idea how to get around that?
There was a problem hiding this comment.
I think this solution is okay as it's only checking modifiers, from the LineEdit's text_submitted signal. Which should already be using the ui_text_submit shortcut.
Given it's not currently handled like a shortcut / input event as above, we'd have to do some restructuring to support this like we do in CodeTextEditor::input, and bypass the LineEdit signal.
There was a problem hiding this comment.
So for this PR, what's the best course of action? Leave it as a modifier check? Or restructuring?
7 participants
Currently there is no easy way to replace all without using the mouse, or using tab to cycle focus to the
Replace Allbutton.Since we are already using the
Shiftmodifier for searching/replacing backwards, I think it's acceptable to useCtrl/Cmdmodifier toReplace All. There is definitely existing software that uses this shortcut, though I cannot think of which off the top of my head.