Skip to content

Add support for parsing "Command/Ctrl" in OS::find_keycode_from_string #108034

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

OzelotVanilla
Copy link
Contributor

Let the function directly parse the shortcut such like "Command/Ctrl+S" to reduce the boilerplate code.

@OzelotVanilla OzelotVanilla requested a review from a team as a code owner June 27, 2025 02:20
@OzelotVanilla OzelotVanilla force-pushed the enhance_find_keycode_for_cmd_or_ctrl_string branch from 64cd57c to 94223ef Compare June 27, 2025 02:39
@timothyqiu
Copy link
Member

Note that regex is an optional module.

It's probably not a good idea to make the core dependent on this module or to change the behavior depending on whether the module is enabled.

@OzelotVanilla
Copy link
Contributor Author

Note that regex is an optional module.

It's probably not a good idea to make the core dependent on this module or to change the behavior depending on whether the module is enabled.

Got that, thank you. Let me try a different approach.

@OzelotVanilla
Copy link
Contributor Author

I fixed it and now it is OK for review.

@@ -435,6 +435,12 @@ Key find_keycode(const String &p_codestr) {
keycode |= KeyModifierMask::META;
} else if (code_part.nocasecmp_to(find_keycode_name(Key::ALT)) == 0) {
keycode |= KeyModifierMask::ALT;
} else if (code_part.nocasecmp_to("command/ctrl") == 0 || code_part.nocasecmp_to("ctrl/command") == 0) {
if (OS::get_singleton()->has_feature("macos") || OS::get_singleton()->has_feature("web_macos") || OS::get_singleton()->has_feature("web_ios")) {
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 this supports web iOS but not native iOS support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied from somewhere else, and I did not realised that. Will fix later, thank you for pointing it out !

Copy link
Contributor Author

@OzelotVanilla OzelotVanilla Jun 27, 2025

Choose a reason for hiding this comment

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

Currently I found that OS::get_name() or OS::get_identifier for lower case, returns the OS information, and I guess comparing them with "macos", "ios", "visionos", plus the has_feature compare of "web_macos" and "web_ios" (and also "web_visionos" ?), solves the problem.

Do you think adding a method in OS like is_apple_platform is better (worth making a PR) ?

  • If so, I will rebase this PR after is_apple_platform is merged to master 😀.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I found the doc doc/classes/OS.xml, lacks the "visionOS" one.

godot/doc/classes/OS.xml

Lines 372 to 383 in 9a39760

<method name="get_name" qualifiers="const">
<return type="String" />
<description>
Returns the name of the host platform.
- On Windows, this is [code]"Windows"[/code].
- On macOS, this is [code]"macOS"[/code].
- On Linux-based operating systems, this is [code]"Linux"[/code].
- On BSD-based operating systems, this is [code]"FreeBSD"[/code], [code]"NetBSD"[/code], [code]"OpenBSD"[/code], or [code]"BSD"[/code] as a fallback.
- On Android, this is [code]"Android"[/code].
- On iOS, this is [code]"iOS"[/code].
- On Web, this is [code]"Web"[/code].
[b]Note:[/b] Custom builds of the engine may support additional platforms, such as consoles, possibly returning other names.

which is implemented already:
String OS_VisionOS::get_name() const {
return "visionOS";
}

Can I ask if it is by design, or ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And also the code I copied is from here:

if (OS::get_singleton()->has_feature("macos") || OS::get_singleton()->has_feature("web_macos") || OS::get_singleton()->has_feature("web_ios")) {

Should it also be changed to the apple-platform detection logic mentioned above ?

Looking forward to your reply 😀.

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.

3 participants