On Thu, 26 Jun 2025 12:17:52 GMT, Prasanta Sadhukhan <psadhuk...@openjdk.org>
wrote:
>> When JRadioButtonMenuItem is called with imageIcon, then only imageIcon is
>> shown without radiobutton in WIndowsLookAndFeel as there was no provision of
>> drawing the radiobutton alongside icon.
>> If icon is not there, the radiobutton is drawn. Added provision of drawing
>> the radiobutton windows Skin even when imageIcon is present.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> MenuItem with icon fix
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/com/sun/java/swing/SwingUtilities3.java line 77:
> 75: private static Color disabledForeground;
> 76: private static Color acceleratorSelectionForeground;
> 77: private static Color acceleratorForeground;
Why are these static? I'm pretty sure the colors can be menu item specific,
although more commonly they would be L&F specific.
src/java.desktop/share/classes/com/sun/java/swing/SwingUtilities3.java line 167:
> 165: }
> 166:
> 167: public static void paintCheckIcon(Graphics g, MenuItemLayoutHelper
> lh,
I suggest creating a new `sun.swing.MenuItemRenderHelper` class similar to
`sun.swing.MenuItemLayoutHelper` to keep both layout and renderer closer
together.
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicMenuItemUI.java line
723:
> 721: acceleratorSelectionForeground);
> 722: SwingUtilities3.setAcceleratorForeground(acceleratorForeground);
> 723: SwingUtilities3.paintAccText(g, lh, lr);
This is a really weird way… Pass the colors explicitly as parameters to the
`SwingUtilities3.paintAccText` method.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsCheckBoxMenuItemUI.java
line 81:
> 79: * Paint MenuItem.
> 80: */
> 81: protected void paintMenuItem(Graphics g, JComponent c,
This javadoc doesn't add anything on top of what's in the overridden method,
remove the javadoc.
Add `@Override` annotation.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
line 891:
> 889: // beside imageicon so this check is necessary to
> differentiate
> 890: boolean isWindows11OrLater =
> Integer.parseInt(System.getProperty("os.name")
> 891:
> .replaceAll("[^0-9]", "")) >= 11;
This value can be cached in a static field rather than evaluating it each time
`WindowsIconFactory.VistaMenuItemCheckIconFactory.VistaMenuItemCheckIcon.paintIcon`
is called.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
line 927:
> 925: skin.paintSkin(g, x - 2 * OFFSET, y,
> 926: getIconWidth(), getIconHeight(),
> backgroundState);
> 927: skinWidth = getIconWidth();
To achieve a better layout, the icon size, specifically its width, should
increase whenever we detect a situation where both check mark / bullet and
associated icon need to be painted together so that
`VistaMenuItemCheckIconFactory` returns an instance of `VistaMenuItemCheckIcon`
or `Windows11MenuItemCheckIcon` which reserves the space for both check mark /
bullet and icon as well as a gap between them. This way, the exiting menu
layout code would position the elements correctly.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java
line 73:
> 71: private static Color disabledForeground;
> 72: private static Color acceleratorSelectionForeground;
> 73: private static Color acceleratorForeground;
Why are these needed?
`BasicMenuItemUI` has fields for all three colors: `disabledForeground`,
`acceleratorSelectionForeground`, `acceleratorForeground`. Why do you need to
create three *new static* fields instead?
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java
line 157:
> 155: UIManager.getColor(prefix + ".disabledForeground");
> 156: }
> 157: }
I'm not convinced `installDefaults` needs overriding, `BasicMenuItemUI` already
provides all these colors via its `BasicMenuItemUI.installDefaults` method and
protected fields.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java
line 184:
> 182: MenuItemLayoutHelper.LayoutResult lr, Color
> holdc) {
> 183: SwingUtilities3.paintIcon(g, lh, lr, holdc);
> 184: }
Why are these static methods needed? You can statically import these methods
from `SwingUtilities3` (or wherever they end up) and use them as if they're
declared locally.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java
line 193:
> 191: SwingUtilities3.setAcceleratorForeground(acceleratorForeground);
> 192: SwingUtilities3.paintAccText(g, lh, lr);
> 193: }
Pass the colors explicitly as parameters to the SwingUtilities3.paintAccText
method.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java
line 201:
> 199: }
> 200:
> 201: protected void paintMenuItem(Graphics g, JComponent c,
Suggestion:
@Override
protected void paintMenuItem(Graphics g, JComponent c,
Add `@Override` annotation.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java
line 234:
> 232: }
> 233: return iconPresent;
> 234: }
This should be really simple:
private static boolean checkIfImageIconPresent(JMenuItem mi) {
return (mi instanceof JCheckBoxMenuItem
|| mi instanceof JRadioButtonMenuItem)
&& mi.getIcon() != null;
}
That is
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java
line 262:
> 260: acceleratorFont,
> MenuItemLayoutHelper.useCheckAndArrow(menuItem),
> 261: prefix);
> 262: MenuItemLayoutHelper.LayoutResult lr = lh.layoutMenuItem();
Can `acceleratorDelimiter`, `acceleratorFont` be fetched from any `MenuItemUI`
classes?
Do I understand correctly that the original menu layout that was already
performed for a class gets thrown away and is being re-evaluated here?
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java
line 269:
> 267:
> 268: if ((Integer.parseInt(System.getProperty("os.name")
> 269: .replaceAll("[^0-9]", "")) >= 11)
You already have this code to detect whether it's Windows 11 or 10, put that
code somewhere for re-use instead of duplicating it.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuItemUI.java
line 306:
> 304: g.setColor(holdc);
> 305: g.setFont(holdf);
> 306: return;
Suggestion:
`return` as the last statement in a method is redundant.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsMenuUI.java
line 136:
> 134: * Paint MenuItem.
> 135: */
> 136: protected void paintMenuItem(Graphics g, JComponent c,
Suggestion:
@Override
protected void paintMenuItem(Graphics g, JComponent c,
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsRadioButtonMenuItemUI.java
line 81:
> 79: * Paint MenuItem.
> 80: */
> 81: protected void paintMenuItem(Graphics g, JComponent c,
Suggestion:
@Override
protected void paintMenuItem(Graphics g, JComponent c,
-------------
PR Review: https://git.openjdk.org/jdk/pull/23324#pullrequestreview-2961758453
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2168941423
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169334895
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169199694
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169272729
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169282066
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169331667
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169217699
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169267877
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169263009
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169219765
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169285512
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169258704
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169310299
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169294403
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169296194
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169298130
PR Review Comment: https://git.openjdk.org/jdk/pull/23324#discussion_r2169302893
