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

Add missing YEN, SECTION and OPENURL names to keycode mappings #81054

Merged
merged 1 commit into from
Aug 28, 2023
Merged

Add missing YEN, SECTION and OPENURL names to keycode mappings #81054

merged 1 commit into from
Aug 28, 2023

Conversation

AttackButton
Copy link
Contributor

Probably these three were the last ones left.

@AttackButton
Copy link
Contributor Author

AttackButton commented Aug 27, 2023

Hey, @Calinou. Could you explain why to postpone? This is not an enhancement. They are missing and not working in gdscript.

@AThousandShips AThousandShips changed the title Add YEN, SECTION and OPENURL to core/os/keyboard.cpp Add missing YEN, SECTION and OPENURL strings to core/os/keyboard.cpp Aug 27, 2023
@AThousandShips
Copy link
Member

The title of your PR was very misleading and really sounded like an enhancement as opposed to a fix, I've updated it to be more descriptive

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Makes sense, don't believe there was any specific reason to skip these, if so it should be commented

@AThousandShips AThousandShips modified the milestones: 4.x, 4.2 Aug 27, 2023
@akien-mga
Copy link
Member

akien-mga commented Aug 27, 2023

I haven't compared the two lists to find it but I think we might still be missing one.

In enum class Key, I count 194 entries (searched for = in that section, after removing the macOS duplicate for CMD_OR_CTRL.

In _keycodes, I count 193 entries in this PR (searched for Key::, likewise after removing the macOS/Windows duplicates).

Edit: Didn't have to search long, the difference is Key::SPECIAL, which is an arbitrary offset. It likely shouldn't be in _keycodes indeed, so it's fine.

Could you add a comment in the header?

diff --git a/core/os/keyboard.h b/core/os/keyboard.h
index cf276dc49f..a25f314411 100644
--- a/core/os/keyboard.h
+++ b/core/os/keyboard.h
@@ -33,6 +33,7 @@
 
 #include "core/string/ustring.h"
 
+// Keep the values in this enum in sync with `_keycodes` in `keyboard.cpp`.
 enum class Key {
 	NONE = 0,
 	// Special key: The strategy here is similar to the one used by toolkits,

@AThousandShips
Copy link
Member

AThousandShips commented Aug 27, 2023

Unfortunately, unlike for example the variant types these aren't sequential so a static check isn't possible

Might be worth to check the matchup between core/core_constants.cpp as well and add cross references to the notes

@Calinou Calinou added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 27, 2023
@akien-mga akien-mga changed the title Add missing YEN, SECTION and OPENURL strings to core/os/keyboard.cpp Add missing YEN, SECTION and OPENURL names to keycode mappings Aug 27, 2023
@akien-mga
Copy link
Member

akien-mga commented Aug 27, 2023

Might be worth to check the matchup between core/core_constants.cpp as well and add cross references to the notes

Good call, I reviewed the list, it was already correct but with some bindings in a different order to the enum.

I suggest applying this patch to this PR:

diff --git a/core/core_constants.cpp b/core/core_constants.cpp
index 2332bc235b..33b3271495 100644
--- a/core/core_constants.cpp
+++ b/core/core_constants.cpp
@@ -420,6 +420,10 @@ void register_global_constants() {
 	BIND_CORE_ENUM_CLASS_CONSTANT(Key, KEY, LAUNCHD);
 	BIND_CORE_ENUM_CLASS_CONSTANT(Key, KEY, LAUNCHE);
 	BIND_CORE_ENUM_CLASS_CONSTANT(Key, KEY, LAUNCHF);
+	BIND_CORE_ENUM_CLASS_CONSTANT(Key, KEY, GLOBE);
+	BIND_CORE_ENUM_CLASS_CONSTANT(Key, KEY, KEYBOARD);
+	BIND_CORE_ENUM_CLASS_CONSTANT(Key, KEY, JIS_EISU);
+	BIND_CORE_ENUM_CLASS_CONSTANT(Key, KEY, JIS_KANA);
 	BIND_CORE_ENUM_CLASS_CONSTANT(Key, KEY, UNKNOWN);
 	BIND_CORE_ENUM_CLASS_CONSTANT(Key, KEY, SPACE);
 	BIND_CORE_ENUM_CLASS_CONSTANT(Key, KEY, EXCLAM);
@@ -492,10 +496,6 @@ void register_global_constants() {
 	BIND_CORE_ENUM_CLASS_CONSTANT(Key, KEY, ASCIITILDE);
 	BIND_CORE_ENUM_CLASS_CONSTANT(Key, KEY, YEN);
 	BIND_CORE_ENUM_CLASS_CONSTANT(Key, KEY, SECTION);
-	BIND_CORE_ENUM_CLASS_CONSTANT(Key, KEY, GLOBE);
-	BIND_CORE_ENUM_CLASS_CONSTANT(Key, KEY, KEYBOARD);
-	BIND_CORE_ENUM_CLASS_CONSTANT(Key, KEY, JIS_EISU);
-	BIND_CORE_ENUM_CLASS_CONSTANT(Key, KEY, JIS_KANA);
 
 	BIND_CORE_BITFIELD_CLASS_FLAG_CUSTOM(KeyModifierMask, KEY_CODE_MASK, CODE_MASK);
 	BIND_CORE_BITFIELD_CLASS_FLAG_CUSTOM(KeyModifierMask, KEY_MODIFIER_MASK, MODIFIER_MASK);
diff --git a/core/os/keyboard.h b/core/os/keyboard.h
index cf276dc49f..785972d31d 100644
--- a/core/os/keyboard.h
+++ b/core/os/keyboard.h
@@ -33,6 +33,8 @@
 
 #include "core/string/ustring.h"
 
+// Keep the values in this enum in sync with `_keycodes` in `keyboard.cpp`,
+// and the bindings in `core_constants.cpp`.
 enum class Key {
 	NONE = 0,
 	// Special key: The strategy here is similar to the one used by toolkits,
diff --git a/doc/classes/@GlobalScope.xml b/doc/classes/@GlobalScope.xml
index e44fdd9776..defe9427aa 100644
--- a/doc/classes/@GlobalScope.xml
+++ b/doc/classes/@GlobalScope.xml
@@ -2035,6 +2035,18 @@
 		<constant name="KEY_LAUNCHF" value="4194415" enum="Key">
 			Launch Shortcut F key.
 		</constant>
+		<constant name="KEY_GLOBE" value="4194416" enum="Key">
+			"Globe" key on Mac / iPad keyboard.
+		</constant>
+		<constant name="KEY_KEYBOARD" value="4194417" enum="Key">
+			"On-screen keyboard" key on iPad keyboard.
+		</constant>
+		<constant name="KEY_JIS_EISU" value="4194418" enum="Key">
+			英数 key on Mac keyboard.
+		</constant>
+		<constant name="KEY_JIS_KANA" value="4194419" enum="Key">
+			かな key on Mac keyboard.
+		</constant>
 		<constant name="KEY_UNKNOWN" value="8388607" enum="Key">
 			Unknown key.
 		</constant>
@@ -2251,18 +2263,6 @@
 		<constant name="KEY_SECTION" value="167" enum="Key">
 			§ key.
 		</constant>
-		<constant name="KEY_GLOBE" value="4194416" enum="Key">
-			"Globe" key on Mac / iPad keyboard.
-		</constant>
-		<constant name="KEY_KEYBOARD" value="4194417" enum="Key">
-			"On-screen keyboard" key iPad keyboard.
-		</constant>
-		<constant name="KEY_JIS_EISU" value="4194418" enum="Key">
-			英数 key on Mac keyboard.
-		</constant>
-		<constant name="KEY_JIS_KANA" value="4194419" enum="Key">
-			かな key on Mac keyboard.
-		</constant>
 		<constant name="KEY_CODE_MASK" value="8388607" enum="KeyModifierMask" is_bitfield="true">
 			Key Code mask.
 		</constant>

Note: Please make modifications by amending the commit, and force pushing the changes, so that it stays as a single commit (see PR workflow).

@AThousandShips
Copy link
Member

Was actually just gonna suggest reordering it to make it easier to track!

@AttackButton AttackButton requested a review from a team as a code owner August 27, 2023 21:50
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks great!

doc/classes/@GlobalScope.xml Outdated Show resolved Hide resolved
@akien-mga akien-mga merged commit cd5c007 into godotengine:master Aug 28, 2023
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@AttackButton AttackButton deleted the core-os-keyboard_cpp branch August 28, 2023 15:15
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 24, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

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