On Mon, 10 Apr 2023 20:04:45 GMT, Martin Fox <d...@openjdk.org> wrote:
>> modules/javafx.graphics/src/main/native-glass/mac/GlassKey.m line 195:
>>
>>> 193: }
>>> 194:
>>> 195: if (keyCode >= 0x00 && keyCode <= 0x32)
>>
>> Add a comment? like `// Null to [Space]`
>
> This are Mac virtual key code constants, not characters, so this covers the
> keyboard from 'A' to 'grave'. I added a comment clarifying this as well as
> the 0x5D to 0x5F range (which are JIS keys). I could change the numbers from
> hex to the symbolic constants defined in Events.h but keeping them in hex
> makes it easier to compare to the table.
Ok, good that you clarify this.
>> modules/javafx.graphics/src/main/native-glass/mac/GlassKey.m line 195:
>>
>>> 193: }
>>> 194:
>>> 195: if (keyCode >= 0x00 && keyCode <= 0x32)
>>
>> out of curiosity, why not?
>> ` if ((keyCode >= 0x00 && keyCode <= 0x32) || (keyCode >= 0x5D && keyCode <=
>> 0x5F))`
>
> No real reason, it was probably just easier to write it like this when I was
> originally going through the table and sorting out the ranges. I do prefer
> separating out conditionals when I can since it makes it easier to set debug
> breakpoints (not that that matters here).
okay, no problem
>> tests/manual/events/KeyboardTest.java line 458:
>>
>>> 456: GERMAN("German", KeyListBuilder.germanKeys()),
>>> 457: LATIN("Latin", KeyListBuilder.latinKeys()),
>>> 458: NON_LATIN("non-Latin", KeyListBuilder.nonLatinKeys());
>>
>> How hard would be adding another languages to this test, like Spanish? I see
>> that you already commented that the robot can't access dead keys (so no
>> possible test for `ñ` for instance...)?
>
> I added Spanish and found a bug in this PR, I wasn't handling the inverted
> exclamation mark. I've added it and Euro to match the Windows code.
>
> I can't test the `ñ` key on the Spanish layout since there's no corresponding
> KeyCode. This applies to most (all?) characters that include a diacritic.
>
> Most dead keys do have KeyCodes so in theory a Robot could generate key press
> events for them. This might even work on Windows and Linux though I haven't
> tested it. I've prototyped the Mac code but I think that would be a separate
> PR.
Thanks for adding Spanish, I've tested it successfully.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/425#discussion_r1164303135
PR Review Comment: https://git.openjdk.org/jfx/pull/425#discussion_r1164304142
PR Review Comment: https://git.openjdk.org/jfx/pull/425#discussion_r1164309229
