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

Reply via email to