Fix incorrect `KEY_MODIFIER_MASK` value #98441

Yelloween10 · 2024-10-22T23:21:14Z

This pull request fixes an incorrect value assignment for MODIFIER_MASK in keyboard.h and @GlobalScope.xml.

Updated the MODIFIER_MASK to 0x7E << 22 to isolate modifier keys correctly, ensuring that it does not interfere with the Key.SPECIAL value.

I tested the solution by checking that the result of the AND operation between MODIFIER_MASK and CODE_MASK returned 0, confirming that the modifier mask is now correctly configured.

(Sorry for the confusion with the previous PR; I'm new to this and accidentally included others' changes, so I created this new one for clarity.)

Fixes #89775

Sauermann

This PR changes the API which requires additional steps. You can find relevant information in the docs:
https://docs.godotengine.org/en/latest/contributing/development/handling_compatibility_breakages.html

Sauermann · 2024-10-23T05:52:44Z

core/os/keyboard.h

@@ -249,7 +249,7 @@ enum class Key {

 enum class KeyModifierMask {
 	CODE_MASK = ((1 << 23) - 1), ///< Apply this mask to any keycode to remove modifiers.
-	MODIFIER_MASK = (0x7F << 22), ///< Apply this mask to isolate modifiers.
+	MODIFIER_MASK = (0x7E << 22), ///< Apply this mask to isolate modifiers.


Suggested change

MODIFIER_MASK = (0x7E << 22), ///< Apply this mask to isolate modifiers.

MODIFIER_MASK = (0x1FF << 23), ///< Apply this mask to isolate modifiers.

Currently the upper 9 bits are reserved for modifiers (see #98447).
If we want to break API compatibility with this change (which I believe, we should in order to fix this API-bug), then we should include the complete range of valid modifiers, so that we don't need to change it again.

Currently the upper 9 bits are reserved for modifiers

Not sure if KPAD and GROUP_SWITCH should be considered modifiers.

But both seems to be unused:

GROUP_SWITCH flag seems to be completely unused (I have no idea what it was intended for).

KPAD flag is parsed by macOS native menu code, but it must an oversight, since it's never set by any code.

I would probably mask it like this:

0-20 Unicode value |1| 21 Reserved |1| CODE_MASK 22 Special key flag |1| 23 Reserved |2| Keep reserved in case we want to add some other special flag. 24 CTRL or CMD flag |3| 25 SHIFT flag |3| 26 ALT flag |3| 27 META flag |3| MODIFIER_MASK 28 CTRL flag |3| 29 (KPAD) Reserved |3| 30 (GROUP_SWITCH) Reserved |3| 31 Reserved |4| Sign bit, keep clean to avoid negative values if it's used with int32_t API.

Sounds good, so that would boil down to:

Suggested change

MODIFIER_MASK = (0x7E << 22), ///< Apply this mask to isolate modifiers.

MODIFIER_MASK = (0x7F << 24), ///< Apply this mask to isolate modifiers.

@Yelloween10 Please see the above suggestion.

@Yelloween10 Please see the above suggestion.

I was just working on it at the moment. I was busy for a few weeks, I have time to do it now.

Yelloween10 · 2024-10-24T15:59:32Z

This PR changes the API which requires additional steps. You can find relevant information in the docs: https://docs.godotengine.org/en/latest/contributing/development/handling_compatibility_breakages.html

I read the docs and I'm confused about how I should continue in this situation. How should I approach this?

DarkyMago · 2024-11-15T00:00:32Z

This PR changes the API which requires additional steps. You can find relevant information in the docs: https://docs.godotengine.org/en/latest/contributing/development/handling_compatibility_breakages.html

I read the docs and I'm confused about how I should continue in this situation. How should I approach this?

you need to add expected changes to API in the correct .expected file in misc/extension_api_validation/

basically you add

GH-%pull-requestID%
-------------------
%expected validation error%

you should find the expected validation error in the failed check that has been run on your PR (this one),
or by running the misc/scripts/validation_extension_api.sh.

Resolved

Yelloween10 · 2024-12-05T17:50:08Z

Is there anything I should do?

AThousandShips · 2024-12-05T17:51:09Z

You've added unrelated changes to the api-validation files, you need to resolve that conflict

Yelloween10 · 2024-12-05T19:42:49Z

I noticed that the changes I made eventually ended up getting conflicts because of the new changes. I tried adding my original change at the end again, but it still says that there are conflicts.

AThousandShips · 2024-12-05T19:47:35Z

misc/extension_api_validation/4.3-stable.expected

+--------
+Validate extension JSON: Error: Field 'classes/TranslationServer/methods/standardize_locale/arguments': size changed value in new API, from 1 to 2.
+
+Optional argument added. Compatibility method registered.


You've added a lot here that I'm not sure where it comes from, did you copy these over from the 4.2 file?

I copied the lines from 4.3-stable.expected and put my changes at the end. I thought that it would fix the message "This branch has conflicts that must be resolved". I did that because my changes got outdated from the new lines inside of the 4.3-stable.expected file.

You need to rebase your branch as you are behind and you are making these changes on top of older code, you shouldn't copy what's in these files right now but rebase instead and just add your content

How can I do that exactly? The last time I tried it, I ended up accidentally ruining the entire PR. Sorry about the questions, I'm nervous about that.

See the interactive rebase for details

Oh, so I need to update my branch with a rebase?
I have another question, should I squash my commits after I'm done?

Yes you need to rebase your branch on the up-to-date branch, squashing would be good too but not necessary until this is approved

Yelloween10 · 2024-12-13T13:53:22Z

Is this good?

AThousandShips · 2024-12-17T23:44:37Z

Compatibility code is correct now!

Yelloween10 · 2024-12-17T23:46:16Z

Should I squash it now?

AThousandShips · 2024-12-17T23:47:03Z

You can do it now if you like, but you don't have to untill it's approved

Updated key modifier mask and documented API change Changed the old value Changed the old value inside the .expected file Resolved Conflicts Moved changes to the end

Yelloween10 · 2024-12-18T00:04:11Z

Alright, thank you.

Yelloween10 · 2024-12-21T09:47:40Z

When would it be available to review?

Sauermann

The changes make sense as discussed here: #98441 (comment)
I can't predict how many problems this breaking change causes for projects. But since the previous implementation is simply wrong, i'm in favor of merging.

Repiteo · 2024-12-23T17:22:48Z

Thanks!

Yelloween10 requested review from a team as code owners October 22, 2024 23:21

clayjohn added the bug label Oct 22, 2024

clayjohn requested a review from a team October 22, 2024 23:22

clayjohn added the topic:input label Oct 22, 2024

clayjohn added this to the 4.x milestone Oct 22, 2024

Sauermann previously requested changes Oct 23, 2024

View reviewed changes

Sauermann added the breaks compat label Oct 23, 2024

Yelloween10 force-pushed the fix-modifier-mask branch from 3c6f926 to 7c88dcb Compare November 15, 2024 10:16

akien-mga changed the title ~~Fix incorrect MODIFIER_MASK value~~ Fix incorrect KEY_MODIFIER_MASK value Nov 15, 2024

Yelloween10 requested review from a team as code owners November 15, 2024 10:56

Mickeon removed the request for review from a team November 16, 2024 14:04

AThousandShips reviewed Dec 5, 2024

View reviewed changes

Yelloween10 force-pushed the fix-modifier-mask branch 2 times, most recently from 526dba2 to 94a5b52 Compare December 12, 2024 21:10

Yelloween10 force-pushed the fix-modifier-mask branch from 94a5b52 to 44419cb Compare December 17, 2024 22:22

Yelloween10 requested a review from AThousandShips December 17, 2024 23:43

Fix incorrect MODIFIER_MASK value

0153cb8

Updated key modifier mask and documented API change Changed the old value Changed the old value inside the .expected file Resolved Conflicts Moved changes to the end

Yelloween10 force-pushed the fix-modifier-mask branch from 44419cb to 0153cb8 Compare December 18, 2024 00:02

Sauermann approved these changes Dec 21, 2024

View reviewed changes

Repiteo modified the milestones: 4.x, 4.4 Dec 21, 2024

Repiteo merged commit b97c8b3 into godotengine:master Dec 23, 2024
20 checks passed

Sauermann mentioned this pull request Jan 9, 2025

Fix comment describing the form of 32-bit keycode representation #98447

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrect `KEY_MODIFIER_MASK` value #98441

Fix incorrect `KEY_MODIFIER_MASK` value #98441

Yelloween10 commented Oct 22, 2024

Sauermann left a comment

Sauermann Oct 23, 2024

bruvzg Oct 23, 2024

Sauermann Oct 23, 2024

akien-mga Nov 15, 2024

Yelloween10 Nov 15, 2024

Yelloween10 commented Oct 24, 2024

DarkyMago commented Nov 15, 2024 •

edited

Loading

Yelloween10 commented Dec 5, 2024

AThousandShips commented Dec 5, 2024

Yelloween10 commented Dec 5, 2024

AThousandShips Dec 5, 2024

Yelloween10 Dec 5, 2024

AThousandShips Dec 5, 2024

Yelloween10 Dec 5, 2024 •

edited

Loading

AThousandShips Dec 6, 2024

Yelloween10 Dec 7, 2024

AThousandShips Dec 7, 2024

Yelloween10 commented Dec 13, 2024

AThousandShips commented Dec 17, 2024

Yelloween10 commented Dec 17, 2024

AThousandShips commented Dec 17, 2024

Yelloween10 commented Dec 18, 2024

Yelloween10 commented Dec 21, 2024

Sauermann left a comment

Repiteo commented Dec 23, 2024

	MODIFIER_MASK = (0x7E << 22), ///< Apply this mask to isolate modifiers.
	MODIFIER_MASK = (0x1FF << 23), ///< Apply this mask to isolate modifiers.

	MODIFIER_MASK = (0x7E << 22), ///< Apply this mask to isolate modifiers.
	MODIFIER_MASK = (0x7F << 24), ///< Apply this mask to isolate modifiers.

Fix incorrect KEY_MODIFIER_MASK value #98441

Fix incorrect KEY_MODIFIER_MASK value #98441

Conversation

Yelloween10 commented Oct 22, 2024

Sauermann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yelloween10 commented Oct 24, 2024

DarkyMago commented Nov 15, 2024 • edited Loading

Yelloween10 commented Dec 5, 2024

AThousandShips commented Dec 5, 2024

Yelloween10 commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yelloween10 Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yelloween10 commented Dec 13, 2024

AThousandShips commented Dec 17, 2024

Yelloween10 commented Dec 17, 2024

AThousandShips commented Dec 17, 2024

Yelloween10 commented Dec 18, 2024

Yelloween10 commented Dec 21, 2024

Sauermann left a comment

Choose a reason for hiding this comment

Repiteo commented Dec 23, 2024

Fix incorrect `KEY_MODIFIER_MASK` value #98441

Fix incorrect `KEY_MODIFIER_MASK` value #98441

DarkyMago commented Nov 15, 2024 •

edited

Loading

Yelloween10 Dec 5, 2024 •

edited

Loading