Update InputEventKey doc: Clarify behavior when setting event properties

[jsjtxietian]

Fixes #84731

Suggestions are welcomed as I'm not a native English speaker.

[timothyqiu]

I don't think this explains the problem. You're just describing the nature of all properties to be set to their default value on creation, which is true for any Variant, not just InputEventKey.

The core of #84731 is assuming setting one of keycode, physical_keycode, and unicode automatically updates other fields.

[jsjtxietian]

setting one of keycode, physical_keycode, and unicode automatically updates other fields

Or it's just the name confuses them, a little hard to clarify though.

[AThousandShips]

Or it's just the name confuses them

Not really, the confusion is that people assume they are linked and update together, and aren't separate properties

[akien-mga]

I think adding docs here is useful, as the behavior is far from obvious. It's not an oversight that only one of keycode or physical_keycode is non-zero, it's actually how InputEventKey knows whether it's a physical or not physical event.

But to fix #84731 itself, I would add documentation to OS.get_keycode_string(), to point out this difference between keycode and physical_keycode.

[bruvzg]

Event mappings should have only one of the [member keycode], [member physical_keycode], or [member unicode] set.

Seems a bit excessive, this part already describes the same behavior.

[timothyqiu]

I would add documentation to OS.get_keycode_string(), to point out this difference between keycode and physical_keycode.

OS.get_keycode_string() simply maps the Key enum into String, it has no concept of keycode and physical_keycode.

Another possible cause is that the 4.1.3 documentation for physical_keycode also says you should use keycode. This is already fixed in 4.2.

godot/doc/classes/InputEventKey.xml

Lines 80 to 83 in 2d3b2ab

	<member name="physical_keycode" type="int" setter="set_physical_keycode" getter="get_physical_keycode" enum="Key" default="0">
	Represents the physical location of a key on the 101/102-key US QWERTY keyboard, which corresponds to one of the [enum Key] constants.
	To get a human-readable representation of the [InputEventKey], use [code]OS.get_keycode_string(event.keycode)[/code] where [code]event[/code] is the [InputEventKey].
	</member>

[akien-mga]

I would add documentation to OS.get_keycode_string(), to point out this difference between keycode and physical_keycode.

OS.get_keycode_string() simply maps the Key enum into String, it has no concept of keycode and physical_keycode.

My point is that OS.get_keycode_string() is used with an int, and we don't tell users what that int should be explictly enough. If they have an event, and want the string for that event's key code, they need to use the right property based on the type of key code their event uses.

And the event is one they receive and they don't know ahead of time if it's physical or not, they basically need to reimplement the internal logic we use:

OS.get_keycode_string(event.physical_keycode if event.keycode == KEY_NONE else event.keycode)

which doesn't feel very satisfactory as API design, but at least this can be documented.

[jsjtxietian]

And the event is one they receive and they don't know ahead of time if it's physical or not, they basically need to reimplement the internal logic we use:

I do feel a little curious about this, why we have to store all three fields together instead of a tagged union or something like a variant to represent a InputKeyEvent, is there any corner cases that we need consider?

[AThousandShips]

Because we don't usually do that and it's not necessary simple or stable to do so I'd say

[timothyqiu]

And the event is one they receive and they don't know ahead of time if it's physical or not

They should know what they want to get. Physical keycode and keycode are in different domains. A physical "E" and a regular "E" are two different keys.

#84731 can only be handled reasonably in 4.2 via the new API:

OS.get_keycode_string(DisplayServer.keyboard_get_keycode_from_physical(e.physical_keycode))

And this is already documented in InputEventKey.

i.e. The issue is not a miss use of OS.get_keycode_string(), but a miss understanding of physical key and how related fields in InputEventKey are filled.

[akien-mga]

Right, see also #82092. OS.get_keycode_string() description should likely advise users to check the description of InputEventKey.physical_keycode.

[jsjtxietian]

They should know what they want to get. Physical keycode and keycode are in different domains.

If we don't need to store keycode and phyiscal keycode simultaneously, I believe this adds more weight to my initial thought of use a tagged union to store all these fields, in this way we can print some warning when user try to access the incorrect field. But I suspect it will break compatibility, the public interface doesn't have to change, but the serialized format will change.

[AThousandShips]

It would require a lot of potentially fragile code to access and update the data, I'd say it's not worth it for a little data savings (you'd save at most one 32 bit value, as the flag for which type is stored would be aligned, so you'd at most save one 32 bit value)

[jsjtxietian]

Not for data saving, but for prevent get the wrong data. Maybe add a tag will do the same, but as you said, it is more work compared to gain. Should be designed from day 1.