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

Rename "Control" key to "Ctrl" and add "_pressed" suffix to all InputEventWithModifiers properties/methods #48168

Merged
merged 1 commit into from
May 17, 2021

Conversation

AaronRecord
Copy link
Contributor

@AaronRecord AaronRecord commented Apr 24, 2021

See #16863 (comment)

if (event->get_control()) {
...
}

becomes

if (event->is_ctrl_pressed()) {
...
}

Renames:

  • KEY_CONTROL to KEY_CTRL
  • MOD_CONTROL to MOD_CTRL
  • InputEventWithModifiers:
    • Affixed _pressed to all modifiers
    • control to ctrl(_pressed)
    • renamed meta(_pressed)'s setters and getters to match the property name ([...]_metakey_pressed() -> [...]_meta_pressed())
    • get_ to is_ for modifiers' getters

On MacBook keyboards, Ctrl is often control, but I believe Ctrl is more common (the documentation literally calls it the Ctrl modifier, and most comments in the code say Ctrl) and more importantly won't get confused with Control nodes.

@AaronRecord AaronRecord requested review from a team as code owners April 24, 2021 21:05
@AaronRecord AaronRecord marked this pull request as draft April 24, 2021 21:07
@AaronRecord AaronRecord force-pushed the control-to-ctrl-4.0 branch 4 times, most recently from 77c672e to 1ff236a Compare April 24, 2021 22:29
@AaronRecord AaronRecord marked this pull request as ready for review April 24, 2021 22:33
@Calinou Calinou added this to the 4.0 milestone Apr 24, 2021
@AaronRecord AaronRecord force-pushed the control-to-ctrl-4.0 branch from 1ff236a to 3678501 Compare April 24, 2021 23:45
Copy link
Contributor

@madmiraal madmiraal 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. Just one small nitpick below.

@AaronRecord AaronRecord force-pushed the control-to-ctrl-4.0 branch from 3678501 to ec0d6d2 Compare April 25, 2021 16:40
@akien-mga akien-mga removed request for a team May 7, 2021 11:49
@akien-mga akien-mga changed the title Rename "control [key]" to "ctrl" Rename "Control" key to "Ctrl" and add "_pressed" suffix to all InputEventWithModifiers properties/methods May 7, 2021
@akien-mga
Copy link
Member

akien-mga commented May 7, 2021

It would have been better for review purposes to split those changes into two PRs, the rename of Control to Ctrl is just one aspect of this PR which changes a lot of the InputEventWithModifiers API.

That being said, now that the work is done let's keep it this way.

I don't have a strong opinion on rename Control to Ctrl, but I'd just raise a comment that the name of keys doesn't necessarily match what's printed on the key caps... My laptop has "pg up" / "pg dn", yet we have KEY_PAGEUP, KEY_PAGEDOWN. A German keyboard has "Strg" for the control key. We also have KEY_ESCAPE when most keycaps have "esc" or "Esc" printed on them.

I do not think that we should rename KEY_ESCAPE so I just want to make it clear that the aim is not to have a 1:1 correspondence between key names and what's printed on key caps. Following a standard might be a better way to decide what should be used.

Now, if most are in favor of the rename I don't mind either way.


On the other proposed changes to InputEventWithModifiers, I haven't seen much discussion. I'm not sure it's worth it to add a _pressed suffix to all properties, isn't it clearly inferred by the description of InputEventWithModifiers? It's an InputEvent and it has modifiers, which can be Alt, Shift, Ctrl, etc.

I don't feel strongly either about this but someone needs to raise a comment to get at least some discussion going on the merits of the change ;)

@AaronRecord
Copy link
Contributor Author

AaronRecord commented May 7, 2021

On the other proposed changes to InputEventWithModifiers, I haven't seen much discussion. I'm not sure it's worth it to add a _pressed suffix to all properties, isn't it clearly inferred by the description of InputEventWithModifiers? It's an InputEvent and it has modifiers, which can be Alt, Shift, Ctrl, etc.

My reasoning for this change was mostly because I thought adding _pressed would help the code read easier.

For example:

if (event->is_meta() || event->is_command()) {
    // ...
}

makes less sense to me than:

if (event->is_meta_pressed() || event->is_command_pressed()) {
    // ...
}

The first one sounds like it's asking if the event is meta or if the event is a command (whatever that'd mean), which might be a little confusing to someone new to the codebase/unfamiliar with the InputEventWithModifiers class, the _pressed makes it clear it's referring to the meta/command keys.

Also I think it's more consistent with Input.is_key_pressed(), which could be called Input.is_key() or Input.get_key() but I think Input.is_key_pressed is the most clear and easy to read.

@AaronRecord AaronRecord force-pushed the control-to-ctrl-4.0 branch 4 times, most recently from e5a8cb2 to 9f60d26 Compare May 7, 2021 19:35
@AaronRecord AaronRecord force-pushed the control-to-ctrl-4.0 branch from 9f60d26 to 97fecd1 Compare May 7, 2021 20:01
@akien-mga akien-mga merged commit 6c367f8 into godotengine:master May 17, 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.

7 participants