On Mon, 9 Jun 2025 18:51:17 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8359053
>
> src/java.desktop/macosx/classes/sun/lwawt/macosx/CPlatformWindow.java line
> 437:
>
>> 435: styleBits = SET(styleBits, TEXTURED, false);
>> 436: styleBits = SET(styleBits, NONACTIVATING, true);
>> 437: styleBits = SET(styleBits, IS_POPUP, true);
>
> Is the code below the comment still required? It looks it was relevant to
> applets only.
Since the code isn't conditionalised on applets, I don't think it should be
changed as part of this PR
> src/java.desktop/macosx/classes/sun/lwawt/macosx/CPlatformWindow.java line
> 716:
>
>> 714: execute(ptr -> {
>> 715: if (isPopup) {
>> 716: CWrapper.NSWindow.orderFrontRegardless(ptr);
>
> Is this still relevant without applets?
Since the code isn't conditionalised on applets, I don't think it should be
changed as part of this PR
> src/java.desktop/share/classes/com/sun/java/swing/SwingUtilities3.java line
> 108:
>
>> 106: * Checks if vsync painting is requested for {@code rootContainer}
>> 107: *
>> 108: * @param rootContainer topmost container. Should be Window
>
> Suggestion:
>
> * @param rootContainer topmost container. Should be {@code Window}
This isn't public API doc, so I'm not inclined to go updating it any more than
I have to.
> src/java.desktop/share/classes/com/sun/media/sound/JavaSoundAudioClip.java
> line 116:
>
>> 114: }
>> 115:
>> 116: public static JavaSoundAudioClip create(final URL url) {
>
> Does this method need to be removed?
It was used by AudioClip. I removed it, nothing broke, so it must now be
un-used. So why not remove it ?
> src/java.desktop/share/classes/java/awt/Component.java line 3938:
>
>> 3936: /**
>> 3937: * Inner class for flipping buffers on a component. That
>> component must
>> 3938: * be a {@code Canvas} or {@code Window}
>
> Suggestion:
>
> * be a {@code Canvas} or {@code Window}.
>
> It ended with a full stop, and the full stop should be preserved.
added back
> src/java.desktop/share/classes/java/awt/Component.java line 3988:
>
>> 3986: /**
>> 3987: * Creates a new flipping buffer strategy for this component.
>> 3988: * The component must be a {@code Canvas} or {@code Window}
>
> Suggestion:
>
> * The component must be a {@code Canvas} or {@code Window}.
full stop added.
> src/java.desktop/share/classes/java/awt/Container.java line 1561:
>
>> 1559: * <p>
>> 1560: * The {@code Window} class is the validate root in AWT.
>> 1561: * Swing introduces more validate roots.
>
> Suggestion:
>
> * Swing introduces more validate roots.
space removed
> src/java.desktop/share/classes/java/awt/Dialog.java line 174:
>
>> 172: * from the same toolkit except those from its own child
>> hierarchy.
>> 173: */
>> 174: TOOLKIT_MODAL
>
> Should we add a note that `APPLICATION_MODAL` and `TOOLKIT_MODAL` mean the
> same thing in the absence of applets or even remove one of them?
Yes, It didn't escape me that they mean essentially the same thing, even more
so than they did before
Theoretically there can be more than one toolkit, although it is not really
possible.
Addressing the nuances of that is outside the scope of this change, so I made
the minimum doc change and removing applet isn't the right time to add advice
about which to use. So future work there.
And very unlikely to remove either of them as I suspect the similarities mean
that developers probably are 50:50 in which they used, so we'd just break
(more) apps if we did.
> src/java.desktop/share/classes/java/awt/Toolkit.java line 1353:
>
>> 1351:
>> 1352: /**
>> 1353: * {@return the EventQueue for this application}
>
> Suggestion:
>
> * {@return the {@code EventQueue} for this application}
ok
> src/java.desktop/share/classes/java/awt/Toolkit.java line 1360:
>
>> 1358:
>> 1359: /**
>> 1360: * A method used by toolkit subclasses to get the EventQueue.
>
> Suggestion:
>
> * A method used by toolkit subclasses to get the {@code EventQueue}.
ok
> src/java.desktop/share/classes/java/awt/Toolkit.java line 1362:
>
>> 1360: * A method used by toolkit subclasses to get the EventQueue.
>> 1361: * This may be more direct or more efficient than calling
>> 1362: * {@code getSystemEventQueue()}
>
> Suggestion:
>
> * {@code getSystemEventQueue()}.
full stop added
> src/java.desktop/share/classes/javax/swing/KeyboardManager.java line 135:
>
>> 133:
>> 134: /**
>> 135: * Find the top focusable Window, or InternalFrame
>
> Suggestion:
>
> * Find the top focusable Window, or JInternalFrame
>
> Update to the real type used in the condition?
a private method in an internal class and the same the same is elsewhere in
this file. Not worth changing.
> src/java.desktop/share/classes/javax/swing/RepaintManager.java line 55:
>
>> 53: * As of 1.6 <code>RepaintManager</code> handles repaint requests
>> 54: * for Swing's top level components (
>> 55: * <code>JWindow</code>, <code>JFrame</code> and <code>JDialog</code>).
>
> Suggestion:
>
> * for Swing's top level components
> * (<code>JWindow</code>, <code>JFrame</code> and <code>JDialog</code>).
>
> Avoid a space after the opening parenthesis.
ok
> src/java.desktop/share/classes/javax/swing/SwingUtilities.java line 2196:
>
>> 2194: * <p>
>> 2195: * The component hierarchy must be displayable up to the toplevel
>> component
>> 2196: * (a {@code Frame}) Otherwise this method returns {@code null}.
>
> Suggestion:
>
> * (a {@code Frame}). Otherwise this method returns {@code null}.
full stop added.
> src/java.desktop/share/classes/javax/swing/UIManager.java line 1457:
>
>> 1455: * This method is called before any code that depends on the
>> 1456: * <code>AppContext</code> specific LAFState object runs.
>> 1457: In some AppContext cases it's possible for this method
>
> Suggestion:
>
> * <code>AppContext</code> specific LAFState object runs.
> * In some AppContext cases, it's possible for this method
fixed
> src/java.desktop/share/classes/sun/awt/AppContext.java line 110:
>
>> 108: * wants to peek all of the key events on the EventQueue to listen for
>> 109: * passwords; if separate EventQueues are used for each ThreadGroup
>> 110: * using AppContexts, the only key events that code will be able to
>
> Suggestion:
>
> * using AppContexts, the only key events that the code will be able to
>
> Definite article?
not here.
> src/java.desktop/share/classes/sun/awt/AppContext.java line 114:
>
>> 112: * change the Swing default look-and-feel; with that default stored in
>> 113: * an AppContext, the look-and-feel will change without
>> 114: * disrupting other contexts.<p>
>
> Suggestion:
>
> * disrupting other contexts.
>
> The empty paragraph can be dropped.
ok
> src/java.desktop/share/classes/sun/awt/EmbeddedFrame.java line 51:
>
>> 49:
>> 50: /**
>> 51: * A generic container used for embedding Java components,
>
> Suggestion:
>
> * A generic container used for embedding Java components.
ok
> src/java.desktop/share/classes/sun/font/SunFontManager.java line 2484:
>
>> 2482: * that code is already written to be able to perform properly if
>> called
>> 2483: * to duplicate work. The main difference is that if we detect we
>> are
>> 2484: * An AppContext environment these new fonts
>
> Suggestion:
>
> * in an AppContext environment these new fonts
fixed
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25698#discussion_r2136487935
PR Review Comment: https://git.openjdk.org/jdk/pull/25698#discussion_r2136488391
PR Review Comment: https://git.openjdk.org/jdk/pull/25698#discussion_r2136490304
PR Review Comment: https://git.openjdk.org/jdk/pull/25698#discussion_r2136492907
PR Review Comment: https://git.openjdk.org/jdk/pull/25698#discussion_r2136494512
PR Review Comment: https://git.openjdk.org/jdk/pull/25698#discussion_r2136495544
PR Review Comment: https://git.openjdk.org/jdk/pull/25698#discussion_r2136496455
PR Review Comment: https://git.openjdk.org/jdk/pull/25698#discussion_r2136509376
PR Review Comment: https://git.openjdk.org/jdk/pull/25698#discussion_r2136544801
PR Review Comment: https://git.openjdk.org/jdk/pull/25698#discussion_r2136545330
PR Review Comment: https://git.openjdk.org/jdk/pull/25698#discussion_r2136546411
PR Review Comment: https://git.openjdk.org/jdk/pull/25698#discussion_r2136515250
PR Review Comment: https://git.openjdk.org/jdk/pull/25698#discussion_r2136548505
PR Review Comment: https://git.openjdk.org/jdk/pull/25698#discussion_r2136517418
PR Review Comment: https://git.openjdk.org/jdk/pull/25698#discussion_r2136526160
PR Review Comment: https://git.openjdk.org/jdk/pull/25698#discussion_r2136526991
PR Review Comment: https://git.openjdk.org/jdk/pull/25698#discussion_r2136549954
PR Review Comment: https://git.openjdk.org/jdk/pull/25698#discussion_r2136550460
PR Review Comment: https://git.openjdk.org/jdk/pull/25698#discussion_r2136551402
