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

Allow using Ctrl + Y to redo actions in the editor #47929

Closed
wants to merge 1 commit into from

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Apr 15, 2021

This is already possible in TextEdit nodes (including the script editor), but this change allows using Ctrl + Y in the 2D and 3D editors as well.

The existing Ctrl + Shift + Z shortcut is still available. Both shortcuts will remain supported to avoid breaking user expectations.

I've done some quick testing and script editor redo still works as expected. Feel free to test this further locally 🙂

Before cherry-picking this to the 3.x branch, remember that it uses a different shortcut handling system. I'd make sure to test this thoroughly on the 3.x branch before cherry-picking.

This is already possible in TextEdit nodes (including the script editor),
but this change allows using Ctrl + Y in the 2D and 3D editors as well.

The existing Ctrl + Shift + Z shortcut is still available.
Both shortcuts will remain supported to avoid breaking user expectations.
@Calinou Calinou added cherrypick:3.x Considered for cherry-picking into a future 3.x release enhancement topic:editor usability labels Apr 15, 2021
@Calinou Calinou added this to the 4.0 milestone Apr 15, 2021
@Riteo
Copy link
Contributor

Riteo commented Apr 16, 2021

I don't know, wouldn't it be redundant to have both? I mean, I have never seen a program having both. IMO we should choose either one or the other and then make whatever uses the old shortcut use the new one.

@Calinou
Copy link
Member Author

Calinou commented Apr 16, 2021

I don't know, wouldn't it be redundant to have both?

Technically, it's redundant, but here, I think the principle of least astonishment matters more. In 2021, applications still haven't settled on using a single redo shortcut. When an application supports only Ctrl + Y or Ctrl + Shift + Z, it'll break user expectations sooner or later.

@Riteo
Copy link
Contributor

Riteo commented Apr 17, 2021

Technically, it's redundant, but here, I think the principle of least astonishment matters more.

Ehhh... Honestly, for such a split shortcut astonishment wouldn't be that bad either, after all other applications have this issue too. I feel like anybody making games is kind of used to this. Look at blender, krita and vscode for example: they use ctrl + shift + z while other programs such as gimp and llms use ctrl + y. Maybe we could use the most common one by looking at what's used most in programs used in game development?

In 2021, applications still haven't settled on using a single redo shortcut.

By that logic, I don't think choosing both is going to help with this issue.

I'm still uncertain whether this PR is the right choice to make. I'll think about it. For now though, IMO this could be avoided, as pretty much the only affected users would be people completely inexperienced with computers if we go for the (less common in windows, but IMO superior) ctrl + shift + z route, but a quick read of the manual could fix this. Also, if the rebindable editor shortcuts PR got merged (has it?) one could choose by themselves which one to use, eliminating the problem altogheter.

@Calinou
Copy link
Member Author

Calinou commented Apr 17, 2021

@Riteo Since Ctrl + Y isn't used for anything in the editor right now, I see no downside to supporting both shortcuts.

@@ -409,6 +409,12 @@ void EditorNode::_unhandled_input(const Ref<InputEvent> &p_event) {
if (ED_IS_SHORTCUT("editor/filter_files", p_event)) {
filesystem_dock->focus_on_filter();
}
if (k->get_command() && k->get_keycode() == KEY_Y) {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why we can't use if (k->is_action("ui_redo", true)) { instead?

Copy link
Member Author

@Calinou Calinou Apr 17, 2021

Choose a reason for hiding this comment

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

@Paulb23 Wouldn't this cause the redo action to apply twice when pressing Ctrl + Shift + Z since there's also the ui_redo shortcut that is being listened to?

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't do as the popup menu will call accept_event, though might need to also call that here.

@Duroxxigar
Copy link
Contributor

@Riteo Since Ctrl + Y isn't used for anything in the editor right now, I see no downside to supporting both shortcuts.

This is assuming people haven't set shortcuts on their own. I know I have to reset some shortcuts every time Godot decides to change a default shortcut.

@akien-mga
Copy link
Member

akien-mga commented Jun 17, 2021

I don't understand why this PR would be needed? ui_redo is already defined with both Ctrl+Shift+Z and Ctrl+Y, so both should already work, like in the script editor, no? CC @EricEzaM

@EricEzaM
Copy link
Contributor

EricEzaM commented Jun 17, 2021

@akien-mga Yes, text edit and such get multiple bindings for actions because they directly use the input map. Other parts of the editor use the Shortcut class, which only has one binding allowed. That is why I did #48009. Maybe we could make that support any amount of shortcut bindings? That would also allow us to unify the shortcut editor instead of having two different sections.

@Riteo
Copy link
Contributor

Riteo commented Jan 17, 2023

Can this be closed? Just tested the editor and I can redo with both Ctrl + Shift + Z and Ctrl + Y. I think this PR has been long superceded by #51273.

@akien-mga
Copy link
Member

Superseded by #51273.

@akien-mga akien-mga closed this Jan 17, 2023
@akien-mga akien-mga added archived and removed cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants