Add `Key::CTRL_OR_CMD` and use it to fix shortcut for tilemap painting tools on macOS #64021

ajreckof · 2022-08-06T23:41:59Z

first add CMD as a Key the same way CMD as been added to KeyModifiersMask
Then replace all KEY::CTRL by KEY::CMD

mhilbrunner · 2022-11-29T11:48:23Z

EricEzaM · 2023-01-13T12:52:58Z

cc @bruvzg as they did some recent working on keyboard/input stuff. Not sure if there is a better way of doing this. On the Input singleton maybe it would be nice to have is_modifier_key_pressed?

bruvzg · 2023-01-13T12:59:58Z

APPLE_STYLE_KEYS was removed, and there's CMD_OR_CTRL mask to specify shortcuts that are Cmd+ on macOS and Ctrl+ on other systems.

bruvzg · 2023-01-13T13:05:16Z

Not sure if there is a better way of doing this. On the Input singleton maybe it would be nice to have is_modifier_key_pressed?

What would be the difference from is_key_pressed?

PR seems fine, but I would rename CMD to CMD_OR_CTRL for consistency with current masks, and #ifdef APPLE_STYLE_KEYS should be changed to #ifdef MACOS_ENABLED.

EricEzaM · 2023-01-13T13:08:15Z

What would be the difference from is_key_pressed?

Ah I forgot that we don't distinguish between left/right modifier keys. I also thought it might be helpful as a way to check if CMD_OR_CTRL is pressed as that is not currently possible. But yeah, just adding CMD_OR_CTRL to the key list like you suggest works well.

ajreckof · 2023-01-14T10:26:39Z

Just made the changed based on what was discussed here without changing what was done on the bottom. Changed the name of the CMD for CMD_OR_CTRL and changed the APPLE_STYLE_KEYS by MACOS_ENABLED.
It would have been cool to be able to use modifiers but at some point we need to know if the modifier is pressed or no for preview even if there is no input to check modifiers from. This why we need to have it as a key and not just a key modifier

core/os/keyboard.cpp

akien-mga

Looks good! There might be more areas in the engine using Input::get_singleton()->is_key_pressed(Key::CTRL) which may benefit from this change.

ajreckof · 2023-01-15T18:00:47Z

I'll look into it. Do you think i should create a new pull request or should i add to this one and rename it ?

ajreckof · 2023-01-16T03:20:56Z

I have already found at least (still going through all occurences of calling is_key_pressed(Key::CTRL)) two other places where using Control instead of command prevents you from using the functionality just as in his case because it needs a left click and using ctrl makes a right click. I think they should also be included in this PR.

I think after i am done with the is_key_pressed(Key::CTRL) i'll look into the is_ctrl_pressed as they might also have the same bugs just because people didn't test on mac and didn't think of using is_command_or_control_pressed() (or the code was developed before the function was implemented).

…RL prevented the use of the functionality on mac on Mac CTRL + Left click = Right click therefore when a functionality needs CTRL + Left click it is not possible on mac. It then forcibly needs to be CMD on mac

akien-mga · 2023-01-16T07:37:09Z

editor/plugins/canvas_item_editor_plugin.cpp

@@ -1897,7 +1897,7 @@ bool CanvasItemEditor::_gui_input_scale(const Ref<InputEvent> &p_event) {
 			Transform2D simple_xform = (viewport->get_transform() * unscaled_transform).affine_inverse() * transform;

 			bool uniform = m->is_shift_pressed();
-			bool is_ctrl = Input::get_singleton()->is_key_pressed(Key::CTRL);
+			bool is_ctrl = m->is_ctrl_pressed();


Should it be CTRL or CMD_OR_CTRL here?

ctrl works fine here as you are already dragging when pushing the control button. I just changed it to the event key modifier as it was more in sync with the other declaration just above and there was no reason to call the Input Singleton.

It works, but is it what makes sense semantically? Usually Ctrl being used as modifier on Windows/Linux is intended to be Command on macOS, no?

Most of the time tes but not always more over something have been Cyril for a long time on Godot so people might have already learned it is Godot so I think for now if it is not causing a bug we should not address it right and open an issue to discuss we want to get rid and of all of them / most of them but keep some edge case.
I think also we might want to discuss wether we would like to add a warning when is_ctrl_pressed is used for people not on a mac to understand it is most likely not what they want to use (if there is a way to disable warning for a line)

I think I will be opening an issue that details everything that I found when I'm done looking through all the is_ctrl_pressed. There are around 50 of them so it might take me a week or so. As I can see it most use cases can be summarised in a few groups. I think those groups should be moved out from ctrl to command but some edge case might better to keep on ctrl.

akien-mga · 2023-01-16T12:14:49Z

Thanks!

ajreckof requested a review from a team as a code owner August 6, 2022 23:42

Calinou added bug platform:macos topic:editor topic:2d labels Aug 7, 2022

Calinou added this to the 4.0 milestone Aug 7, 2022

ajreckof force-pushed the change-shortcut-for-rect-tool-tilemap branch from d1a3ef5 to 6b124e2 Compare August 18, 2022 19:09

mhilbrunner requested a review from groud August 18, 2022 19:20

ajreckof force-pushed the change-shortcut-for-rect-tool-tilemap branch from 6b124e2 to d994f77 Compare January 14, 2023 09:52

bruvzg approved these changes Jan 14, 2023

View reviewed changes

akien-mga reviewed Jan 15, 2023

View reviewed changes

core/os/keyboard.cpp Outdated Show resolved Hide resolved

add CMD_OR_CTRL as a Key and not just a key modifier

4309dff

ajreckof force-pushed the change-shortcut-for-rect-tool-tilemap branch from d994f77 to 7383f36 Compare January 15, 2023 17:51

akien-mga approved these changes Jan 15, 2023

View reviewed changes

ajreckof force-pushed the change-shortcut-for-rect-tool-tilemap branch from 7383f36 to 44af893 Compare January 16, 2023 04:14

replace the use of CTRL by CMD_OR_CTRL in three places where using CT…

fa528b8

…RL prevented the use of the functionality on mac on Mac CTRL + Left click = Right click therefore when a functionality needs CTRL + Left click it is not possible on mac. It then forcibly needs to be CMD on mac

ajreckof force-pushed the change-shortcut-for-rect-tool-tilemap branch from 44af893 to fa528b8 Compare January 16, 2023 05:07

ajreckof requested review from akien-mga and bruvzg and removed request for groud, akien-mga and bruvzg January 16, 2023 05:32

ajreckof requested a review from akien-mga January 16, 2023 05:32

akien-mga reviewed Jan 16, 2023

View reviewed changes

akien-mga changed the title ~~Change shortcut for tilemap painting tools~~ Add Key::CTRL_OR_CMD and use it to fix shortcut for tilemap painting tools on macOS Jan 16, 2023

akien-mga merged commit d94a46a into godotengine:master Jan 16, 2023

akien-mga mentioned this pull request Jan 16, 2023

Make keyboard shortcuts in tile data editors consistent with tilemap editor. #71517

Merged

ajreckof mentioned this pull request Jan 23, 2023

Replace Ctrl in editor shortcuts with Cmd or Ctrl depending on platform #71905

Merged

2 tasks

ajreckof deleted the change-shortcut-for-rect-tool-tilemap branch January 23, 2023 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Key::CTRL_OR_CMD` and use it to fix shortcut for tilemap painting tools on macOS #64021

Add `Key::CTRL_OR_CMD` and use it to fix shortcut for tilemap painting tools on macOS #64021

ajreckof commented Aug 6, 2022

mhilbrunner commented Nov 29, 2022

EricEzaM commented Jan 13, 2023

bruvzg commented Jan 13, 2023

bruvzg commented Jan 13, 2023

EricEzaM commented Jan 13, 2023 •

edited

Loading

ajreckof commented Jan 14, 2023 •

edited

Loading

akien-mga left a comment

ajreckof commented Jan 15, 2023 •

edited

Loading

ajreckof commented Jan 16, 2023 •

edited

Loading

akien-mga Jan 16, 2023

ajreckof Jan 16, 2023

akien-mga Jan 16, 2023

ajreckof Jan 16, 2023

akien-mga commented Jan 16, 2023

Add Key::CTRL_OR_CMD and use it to fix shortcut for tilemap painting tools on macOS #64021

Add Key::CTRL_OR_CMD and use it to fix shortcut for tilemap painting tools on macOS #64021

Conversation

ajreckof commented Aug 6, 2022

mhilbrunner commented Nov 29, 2022

EricEzaM commented Jan 13, 2023

bruvzg commented Jan 13, 2023

bruvzg commented Jan 13, 2023

EricEzaM commented Jan 13, 2023 • edited Loading

ajreckof commented Jan 14, 2023 • edited Loading

akien-mga left a comment

Choose a reason for hiding this comment

ajreckof commented Jan 15, 2023 • edited Loading

ajreckof commented Jan 16, 2023 • edited Loading

akien-mga Jan 16, 2023

Choose a reason for hiding this comment

ajreckof Jan 16, 2023

Choose a reason for hiding this comment

akien-mga Jan 16, 2023

Choose a reason for hiding this comment

ajreckof Jan 16, 2023

Choose a reason for hiding this comment

akien-mga commented Jan 16, 2023

Add `Key::CTRL_OR_CMD` and use it to fix shortcut for tilemap painting tools on macOS #64021

Add `Key::CTRL_OR_CMD` and use it to fix shortcut for tilemap painting tools on macOS #64021

EricEzaM commented Jan 13, 2023 •

edited

Loading

ajreckof commented Jan 14, 2023 •

edited

Loading

ajreckof commented Jan 15, 2023 •

edited

Loading

ajreckof commented Jan 16, 2023 •

edited

Loading