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