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 shortcuts to have any number of input events assigned to them. #51273

Merged

Conversation

EricEzaM
Copy link
Contributor

@EricEzaM EricEzaM commented Aug 5, 2021

Closes godotengine/godot-proposals#2875
Supersedes / Closes #48009

This is based on discussion on Rocket chat as well as feedback in the above proposal and PR.

Summary:

  • One Shortcut can now have many InputEvents assigned to it (related; Shortcut: Rename shortcut property to event #51215)
  • Relevant methods and properties changed, as well as the way Editor shortcuts are serialised as it is now an array.
  • The EditorSettings Shortcuts editor finally has the same user interface for Built-In actions and other editor actions! 🎉🎉 (see below). A lot of the changes were to do with the changes made to the editor settings dialog.
  • I had to made one minor modification to drag & drop logic, where the drag and drop forward target can now just be a Node rather than being a Control. This is because EditorSettingsDialog needed to be the node which the Tree drag was forwarded to, but as Window is not a Control, I had to change this to Node.

The the editor settings shows up to 2 shortcuts in-line, and the additional amount in parentheses. The next tree level then shows all shortcuts, where they can be edited or removed. The user can drag and drop shortcuts to determine which one is the "primary" one. The primary shortcut is what is displayed in the UI, for example in PopupMenus with items that have shortcuts, or in Button tooltips, as shown in the video. In the video, any shortcut (E, Ctrl+R, Ctrl+Shit+T) could be used to select the rotate mode.
image

shortcut_changing.mp4

For users, the shortcuts resource now includes an array, which accepts any InputEvent derived resource. Please note when editing this in the inspector, this is affected by the new "inspector refresh" system, as described here: #49817. To test this PR properly and be able to drop down the array item selector, change your inspector refresh interval in your settings to a high value (e.g. 5 seconds). P.S. yes, the button in the video should say "press S", its just a typo :)

shortcuts.showcase.mp4

And for serialization of shortcut editor settings, a bit simpler now:

shortcuts = [{
"name": "canvas_item_editor/rotate_mode",
"shortcuts": [SubResource( "InputEventKey_5ftl2" ), SubResource( "InputEventKey_nudrv" ), SubResource( "InputEventKey_ffuxw" )]
}, {
"name": "tiles_editor/paint_tool",
"shortcuts": [SubResource( "InputEventKey_nlk4p" )]
}]

@EricEzaM EricEzaM requested review from a team as code owners August 5, 2021 10:46
@EricEzaM
Copy link
Contributor Author

EricEzaM commented Aug 5, 2021

I am not sure if #51215 should be merged before or after this... Probably before?

@akien-mga
Copy link
Member

I am not sure if #51215 should be merged before or after this... Probably before?

Either way works for me, but I'd need someone to approve it to merge ;)

@EricEzaM EricEzaM force-pushed the multiple-events-per-shortcut-take2 branch 7 times, most recently from 41617e6 to a42c556 Compare August 7, 2021 03:15
@akien-mga
Copy link
Member

Needs a rebase.

@EricEzaM EricEzaM force-pushed the multiple-events-per-shortcut-take2 branch from a42c556 to 46ca88a Compare September 21, 2021 01:28
@EricEzaM EricEzaM requested a review from a team as a code owner September 21, 2021 01:28
@EricEzaM
Copy link
Contributor Author

EricEzaM commented Sep 21, 2021

Rebased.

Also had to make some changes to fix the inspector refresh bug. I follow what was done in #52082, which is basically just making the backing field and Array instead of List<Ref<InputEvent>> *. This kind of sucks since we lose the typing but... otherwise it is completely unusable in resources/inspector, so it needs to be done. Not sure if Vector<> can be used for this, or TypedArray<>, but the PR I mentioned above just uses Array so that's what I went with, and it's fine. The positive is that we can remove the auxiliary methods which just set/get the list as an array, so there is compatibility with the properties and functions system.

Anyway, it all appears to be working as expected.

@EricEzaM EricEzaM force-pushed the multiple-events-per-shortcut-take2 branch 5 times, most recently from df23d3d to 6961f40 Compare September 22, 2021 00:35
Copy link
Member

@Paulb23 Paulb23 left a comment

Choose a reason for hiding this comment

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

Looks good to me, couple small nitpicks but nothing major.

core/input/shortcut.cpp Outdated Show resolved Hide resolved
core/input/shortcut.cpp Outdated Show resolved Hide resolved
editor/editor_settings.cpp Outdated Show resolved Hide resolved
editor/editor_settings.cpp Outdated Show resolved Hide resolved
core/input/shortcut.cpp Outdated Show resolved Hide resolved
@EricEzaM EricEzaM force-pushed the multiple-events-per-shortcut-take2 branch from 6961f40 to b7974b4 Compare September 26, 2021 23:23
@EricEzaM EricEzaM force-pushed the multiple-events-per-shortcut-take2 branch from b7974b4 to ad30b0a Compare October 1, 2021 08:04
@akien-mga akien-mga merged commit 769691a into godotengine:master Oct 1, 2021
@akien-mga
Copy link
Member

Thanks!

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.

Allow shortcuts to have more than one input event assigned to them
3 participants