On Thu, 22 May 2025 21:43:20 GMT, Phil Race <p...@openjdk.org> wrote:
>> Magnus Ihse Bursie has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Restore MenuShortcut.java
>> - Restore LocaleDataTest.java
>
> src/java.desktop/share/classes/java/awt/MenuShortcut.java line 49:
>
>> 47: * For example, a menu shortcut for "Ctrl+cyrillic ef" is created by
>> 48: * <p>
>> 49: * <code>MenuShortcut ms = new
>> MenuShortcut(KeyEvent.getExtendedKeyCodeForChar('ф'), false);</code>
>
> This is javadoc inJava SE specification. As is the Action case below.
> I can't think of any actual harm from this change, so OK, but I am not
> seeing why it is needed.
The resulting Javadoc html files will contain the same character before and
after this fix, so there is no specification change.
I did this since I thought it increased readability of the source code, but I
can just as well revert it.
> test/jdk/java/awt/font/TextLayout/RotFontBoundsTest.java line 63:
>
>> 61:
>> 62: private static final String INSTRUCTIONS =
>> 63: "A string \u201C" + TEXT + "\u201D is drawn at eight
>> different "
>
> I really don't like these tests being changed. It isn't part of the JDK build.
> People compile these in all sorts of locales.
> Please revert all the changes in the client tests.
Just a clarification. What I did was convert curly quotes and an em dash to
normal ASCII quotes and an ASCII hyphen, just as it is in all other
`INSTRUCTIONS` in the tests. This is similar to what was done in JDK-8354273
and JDK-8354213, and should have been made as part of those PRs if I only had
noticed this place by then.
The user's locale should not really matter. When have stringent locale
requirements for building, and automatically setup the correct locale while
building. I don't think the locale will matter at runtime either, but it n the
rare case that it should matter, then pure ASCII would be prefer, wouldn't it?
Let me know if you still want me to revert it.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25228#discussion_r2106769659
PR Review Comment: https://git.openjdk.org/jdk/pull/25228#discussion_r2106791184
