Skip to content

8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel #23324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

prsadhuk
Copy link
Contributor

@prsadhuk prsadhuk commented Jan 28, 2025

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.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel (Bug - P3)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23324/head:pull/23324
$ git checkout pull/23324

Update a local copy of the PR:
$ git checkout pull/23324
$ git pull https://git.openjdk.org/jdk.git pull/23324/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23324

View PR using the GUI difftool:
$ git pr show -t 23324

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23324.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 28, 2025

👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Jan 28, 2025

@prsadhuk This change is no longer ready for integration - check the PR body for details.

@openjdk
Copy link

openjdk bot commented Jan 28, 2025

@prsadhuk The following label will be automatically applied to this pull request:

  • client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the rfr Pull request is ready for review label Jan 28, 2025
@mlbridge
Copy link

mlbridge bot commented Jan 28, 2025

@kumarabhi006
Copy link
Contributor

@prsadhuk Unable to see the duke image along with RadioButtonMenuItem. Is it correct behaviour?

RadioButtonMenuItem

@prsadhuk
Copy link
Contributor Author

@prsadhuk Unable to see the duke image along with RadioButtonMenuItem. Is it correct behaviour?

RadioButtonMenuItem

did you download the duke file from the PR?

@kumarabhi006
Copy link
Contributor

@prsadhuk Unable to see the duke image along with RadioButtonMenuItem. Is it correct behaviour?
RadioButtonMenuItem

did you download the duke file from the PR?

Yeah, it's inside "javax\swing\JMenuItem" folder.

@kumarabhi006
Copy link
Contributor

@prsadhuk After image path correction, duke image icon is visible.

Copy link
Contributor

@kumarabhi006 kumarabhi006 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jan 30, 2025
Copy link
Member

@aivanov-jdk aivanov-jdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am also for generating an ImageIcon on the fly to avoid adding a binary file.

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Jan 30, 2025
@aivanov-jdk
Copy link
Member

This does not look right at all:

The screenshot of the newly added test case TestImageIconWithJRadioButtonMenuItem.java on Windows 10: it renders an unselected radio button control to the left of menu item which has an icon in addition to text

You shouldn't render an unselected radio button in menu.

(Yes, I modified your test case to display the menu items in a popup menu as they're supposed to be.)

The position of the image also seems off. On Windows 10, the selected state has a background which you can see on the screenshot. Without the fix, the duke is centered in this selected background; but with the fix, it's off.

If we add support for painting both an ImageIcon and the bullet / check-mark, the layout of the menu item has to change.

@prsadhuk
Copy link
Contributor Author

You shouldn't render an unselected radio button in menu.

Please check updated PR and share your testcase

@aivanov-jdk
Copy link
Member

You shouldn't render an unselected radio button in menu.

Please check updated PR…

I saw your update. I wrote this comment before you removed rendering the unselected radio button.

…share your testcase

Here's the latest version of modified TestImageIconWithJRadioButtonMenuItem.java.

The diff between yours and mine.

@aivanov-jdk
Copy link
Member

Based on the discussion above in relation to JDK-6432667 where the current behaviour/rendering was implemented, this quirk may be a feature of Windows L&F rather than a bug.

Windows 10 - red square item selected

Windows 10 has a highlight around the selected icon. Yet there's no such highlight in Windows 11, therefore it's hard to distinguish whether a menu item has a selected state of not.

@prrace
Copy link
Contributor

prrace commented May 7, 2025

I refined the example program and submitted a support case to Microsoft.
I'll attach to the JBS issue that version of the test case (rbm.cpp) and then
one (rbma.cpp) showing where we've got to.

First there are several elements to understand about menu items
https://learn.microsoft.com/en-us/windows/win32/api/winuser/ns-winuser-menuiteminfoa

(1) The menu item can have a string as the "item" (dwTypeData)
(2) The menu item can have a bitmap as the "item" (hbmpItem)

  • Historically you could only have ONE of these because the field used to specify them is the same. Now by using the new MIIM* values you can have both. And - I don't quite understand this - using the old MFT_STRING didn't seem to work at all for me I had to use MIIM_STRING.

As well as those you can specify the images to use for the check state

  • hbmpChecked - if the item is selected
  • hbmpUnchecked - if it is not selected.

Some questions and some answers with Microsoft

Q. Why is the highlighting background gone in Windows 11 ?

A. That background was part of the “Reveal” lighting effect used by the old Fluent-Design menus in Windows 10. As of WinUI 2.6 (the style set shipped with Windows 11) the UI team removed that effect across context-menus and other pop-ups to achieve a flatter look that meets new contrast guidelines. So it’s an intentional theme change, not a GDI regression.

Q. Why does the item image draw over the default radio button. How can this be anything except a bug ?

A. The Win32 menu engine draws the default bullet/checkmark at X = 0 unless you reserve a check-mark column by supplying hbmpChecked (and optionally hbmpUnchecked).
When an hbmpItem is also present, both bitmaps occupy the same origin, so the icon ends up covering the bullet. This rule has been in place since the Windows 9x/NT days—older SDK samples illustrate the same overlap if you omit the custom check bitmaps. Because the behaviour is longstanding and documented indirectly (see the remarks under SetMenuItemBitmaps and the SM_CXMENUCHECK metric), the shell team classifies it as by design rather than a bug.

Q. Why isn't space automatically left for the default rendering of the checkmark ? Another bug ?

A. For historical reasons the menu layout engine only allocates the check-mark column when it has a concrete size for that column, i.e. when hbmpChecked is non-NULL. If you ask for the default glyph while also providing an item icon, Windows assumes you are handling layout yourself. The docs don’t call this out explicitly (agreed, that would be helpful), but the behaviour is consistent and the same on every supported version of Windows, so it is not tracked as a GDI defect.

So .. in summary, two things that have been a bug so long they are considered a feature are at the root of this and the visual "cue" we've relied on for in Windows 10 was just an accident of some theming that is now gone.

So GDI really does make it hard to have all 3 of these. And maybe we should just not support it ?

But if (as GDI programmers) we want an item string and bitmap, and expect feedback that an item is selected, we seem to be forced into needing to specify bitmaps for the checked and un-checked state. But then you lose the default rendering of the bullet that Windows provides !

There's some hoops we can jump through to get close

  • Use a transparent bitmap for hbmpUnchecked
  • Use a GDI API - DrawFrameControl - to render a bullet image into a bitmap we use for hbmpChecked. Prasanta previously noted that AWT uses this API already)

rbma.cpp illustrates this.

This still isn't perfect - the bullet is too large.

I'll leave it to others to figure out how we make use of this in the Swing rendering case.
Are there any unanswered questions ? This is a long PR so perhaps ..

@prsadhuk
Copy link
Contributor Author

I checked rbma.cpp output where I think we can only support the 4th item in the list (which is this)
image

This is because we have JRadioButtonMenuItem(String text, Icon icon, boolean selected)
API which only supports one icon image (so checked/unchecked icon image in addition to item icon as below image is not possible in my opinion in Java swing)

image

The current state of PR supports this 4th item.
Only thing different is when there is no imageicon and menuitem is selected, then the text seems to be in same vertical line as the other items (in native rbma output)
image

but current PR the text is shifted to left (when there is no imageicon ie 3rd item in list) and occupies icon space, as below
image

This is because in most cases (and in SwingSet2) the radiomenu items are drawn without imageicon and there is no extra space between radio bullet and menu text

image

so if we cater to text shifting to same vertical line, there will be difference in behavior w.r.t existing one and user will notice a shift in text to accomodate non-existing image icon)

but it is fixable,
so if we are in consensus about supporting the 4th item in rbma.cpp output in Java swing with text in same vertical offset, then let mw know..Anything else in addition needs discussion whether we can support...

@prrace
Copy link
Contributor

prrace commented May 27, 2025

I checked rbma.cpp output where I think we can only support the 4th item in the list (which is this) image

This is because we have JRadioButtonMenuItem(String text, Icon icon, boolean selected) API which only supports one icon image (so checked/unchecked icon image in addition to item icon as below image is not possible in my opinion in Java swing)

Not sure I know what you mean by 4th item, but clearly we aren't going to add an API to supply checked/unchecked images.
But we should support rendering like you show above.

  • app supplies icon image and text, WE supply the radio button.

image

The current state of PR supports this 4th item.

What is "this 4th item" ? You mean the case of image + text from app, and WE display a radio button checked ?

Only thing different is when there is no imageicon and menuitem is selected, then the text seems to be in same vertical line as the other items (in native rbma output) image

but current PR the text is shifted to left (when there is no imageicon ie 3rd item in list) and occupies icon space, as below !

If you mean windows always lines up the start of the text, that seems to be true.
So we should do the same.

image

This is because in most cases (and in SwingSet2) the radiomenu items are drawn without imageicon and there is no extra space between radio bullet and menu text

it seems to me that if there's an image in ANY menu item, Windows reserves the space in all items, hence getting the text aligned.
It just does the funky thing of moving where it displays the item image if there's no checked image.

image

so if we cater to text shifting to same vertical line, there will be difference in behavior w.r.t existing one and user will notice a shift in text to accomodate non-existing image icon)

You mean user will notice text is now aligned ? I think that's fine.

but it is fixable, so if we are in consensus about supporting the 4th item in rbma.cpp output in Java swing with text in same vertical offset, then let mw know..Anything else in addition needs discussion whether we can support...

I hope (!) we are on the same page. Let me try and say it so we are ..

  • WE will always draw a check (tick) or radio button to show the selected item
  • WE will always reserve space for that tick
  • WE will never over-write the check/tick with the icon image.
  • Icon images will never move (unlike the funky windows case which is the root of all of this) because we will always reserve space for it.
  • If ANY item has an icon image, ALL items reserve the space, even if not used, this is needed for the next item :
  • Text strings will all align. Always

@prsadhuk
Copy link
Contributor Author

I hope (!) we are on the same page. Let me try and say it so we are ..

* WE will always draw a check (tick)  or radio button to show the selected item

Yes, current PR does this..

* WE will always reserve space for that tick

Yes, current PR does this.

* WE will never over-write the check/tick with the icon image.

Yes, current PR does this.

* Icon images will never move  (unlike the funky windows case which is the root of all of this)  because we will always reserve space for it.

Yes, icon image has its own space in current PR

* If ANY item has an icon image, ALL items reserve the space, even if not used, this is needed for the next item :

* Text strings will all align. Always

Current PR doesnt have this..Text string is not aligned for item that doesn't have iconimage with item that does have iconimage. I will fix this.

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label May 28, 2025
@prsadhuk
Copy link
Contributor Author

Text alignment fixed .. Current PR now looks like this

image

In SwingSet2, now there will be gap between bullet and text to accomodate non-existing imageicon

image

@prrace
Copy link
Contributor

prrace commented Jun 18, 2025

In SwingSet2, now there will be gap between bullet and text to accomodate non-existing imageicon

That gap should only be there if at least one menu item has an image icon. It shoudn't be there if none have an icon.

@prsadhuk
Copy link
Contributor Author

prsadhuk commented Jun 19, 2025

In SwingSet2, now there will be gap between bullet and text to accomodate non-existing imageicon

That gap should only be there if at least one menu item has an image icon. It shoudn't be there if none have an icon.

That will require to find out if current menuItem is part of ButtonGroup and then reiterate each menuitems of the ButtonGroup and see if anyone has an imageicon..It does not seem possible to find out from current menuItem if it part of ButtonGroup as that info is not passed to each MenuItemUI (ButtonGroup info is only known to application/user)

Also, assuming if we do find a way to get if this menuItem is part of a ButtonGroup, then iterating everytime to find out if any menu item has imageicon will affect performance.
Probably it's better to reserve space for imageicon even if not present..

@aivanov-jdk
Copy link
Member

In SwingSet2, now there will be gap between bullet and text to accomodate non-existing imageicon

That gap should only be there if at least one menu item has an image icon. It shoudn't be there if none have an icon.

That will require to find out if current menuItem is part of ButtonGroup and then reiterate each menuitems of the ButtonGroup and see if anyone has an imageicon..It does not seem possible to find out from current menuItem if it part of ButtonGroup as that info is not passed to each MenuItemUI (ButtonGroup info is only known to application/user)

Why? This depends on two factors only:

  1. Are there any instances of JCheckBoxMenuItem or JRadioButtonMenuItem, and
  2. Does any of them contain an icon?

If both are true, you add two columns for the mark and for the icon; in all other cases, the layout remains as it is now with one column where icons and marks are rendered.

The above is done in Metal L&F (run the test case in the default L&F), why can't it be done in Windows L&F?

Also, assuming if we do find a way to get if this menuItem is part of a ButtonGroup, then iterating everytime to find out if any menu item has imageicon will affect performance. Probably it's better to reserve space for imageicon even if not present..

No, I don't think it's a good approach. Look at how it's done in Metal, ideally re-use the layout code.

@prsadhuk
Copy link
Contributor Author

In SwingSet2, now there will be gap between bullet and text to accomodate non-existing imageicon

That gap should only be there if at least one menu item has an image icon. It shoudn't be there if none have an icon.

@prrace Fixed now in latest iteration

image

SwingSet2 there's no gap for RBMI since there are no imageicons

image

@aivanov-jdk
Copy link
Member

The current layout doesn't match the layout of menus in Windows.

The check mark or the bullet has to be painted where it was always painted. The menu in Windows 10 looks broken now:

The position of the check mark isn't in the centre of the highlight

This applies to the radio bullet JRadioButtonMenuItem3 which has no icon.

In Windows 11, I expect the check mark and the bullet render at the same location as it was before the fix, but it's not the case.

Below is a portion of the menu rendered with the current fix:

Comparison of JMenuItem on Windows11 with the fix vs. without the fix

The part on the image in an orange rectangle is a portion of the same test case running on JDK that has no fix.

  1. The width of the popup menu with the two columns—for check marks or bullets and icons—is the same, how is it possible? If a second column for the icon is added, the width of the popup menu has to increase.
  2. The text of JCheckBoxMenuItem 3 is not aligned to other menu items text.
    This applies to any other item added to the menu: the text of all items must align.
  3. The location of the check mark and the bullet is wrong, you moved the check mark / bullet to the left but now it doesn't match the menu layout of a native Win32 application.
  4. If I add another regular menu item with an icon, the icon is rendered below the menu item text.

@aivanov-jdk
Copy link
Member

We have to support two cases:

  1. One column is reserved as it is currently done;
  2. Two columns are reserved if there's JCheckBoxMenuItem and/or JRadioButtonMenuItem that has an icon.

To reiterate Phil's suggestion,

  • One-column layout (nothing changes):
    • No menu item has icons, check marks or bullets;
    • There are JCheckBoxMenuItem and/or JRadioButtonMenuItem but they have no icons, and no other menu items have icons either;
    • There are JCheckBoxMenuItem and/or JRadioButtonMenuItem but none of them has an icon, yet there are other menu items with icons: the check marks / bullets and the icons share the same reserved column.
  • Two-column layout (new layout):
    • There's a least one JCheckBoxMenuItem and/or JRadioButtonMenuItem that has an icon.

In the two-column layout, the first column (which is always present) is reserved for check marks and bullets of JCheckBoxMenuItem and JRadioButtonMenuItem correspondingly, and the second column is reserved for rendering any icons that menu items have.

@prsadhuk
Copy link
Contributor Author

4. If I add another regular menu item with an icon, the icon is rendered below the menu item text.

I couldn't reproduce this

image

@aivanov-jdk
Copy link
Member

This is how it looks for me:

Radio- and check mark menus with additional menu item with an icon with the proposed fix and 200% scale

I haven't pulled your recent changes that you pushed 7 hours ago.

@aivanov-jdk
Copy link
Member

The layout for Windows 11 is still wrong and suffers from the same problems that I raised yesterday. The entire popup menu looks the same way that I posted about an hour ago.

The layout for Windows 10 is correct now.

@aivanov-jdk
Copy link
Member

aivanov-jdk commented Jun 26, 2025

Your latest change has fixed the position of the icon for a regular JMenuItem.

  • Text strings will all align. Always

Current PR doesnt have this..Text string is not aligned for item that doesn't have iconimage with item that does have iconimage. I will fix this.

This still remain unresolved: the text of all menu items should be aligned.

@prsadhuk
Copy link
Contributor Author

Current PR doesnt have this..Text string is not aligned for item that doesn't have iconimage with item that does have iconimage. I will fix this.

This still remain unresolved: the text of all menu items should be aligned.

It is fixed for RBMIs as they are part of ButtonGroup (where selecting one will deselect other) so we can find out if any of the RBMIs in that group has imageicon by obtaining ButtonGroup object.
CBMIs are not part of group and can be selected/deselected individually so it doesn't seem possible to know from one CBMI whether another CBMI used by application have imageicon or not (similar for regular MIs) so in their case text are aligned individually depending on presence of imageicon for that CBMI (or regular MI)

@aivanov-jdk
Copy link
Member

Current PR doesnt have this..Text string is not aligned for item that doesn't have iconimage with item that does have iconimage. I will fix this.

This still remain unresolved: the text of all menu items should be aligned.

It is fixed for RBMIs as they are part of ButtonGroup (where selecting one will deselect other) so we can find out if any of the RBMIs in that group has imageicon by obtaining ButtonGroup object.

Why is it important?

CBMIs are not part of group and can be selected/deselected individually so it doesn't seem possible to know from one CBMI whether another CBMI used by application have imageicon or not (similar for regular MIs) so in their case text are aligned individually depending on presence of imageicon for that CBMI (or regular MI)

Why is it important?

The rule to switching to two-column layout is as simple as that:

There's a least one JCheckBoxMenuItem and/or JRadioButtonMenuItem that has an icon.

If there's at least one JCheckBoxMenuItem and/or JRadioButtonMenuItem in a popup menu, the entire menu should be laid out in two columns reserved columns:

  • The first column is for check mark or bullet;
  • The second column is for the icon associated with a menu item;
  • The text goes in the third column;
  • Accelerators are rendered in the fourth column.

This is absolutely possible, both Metal & Nimbus L&F use this menu layout.
JMenu layout with radio bullets, check marks and icons in Metal Look-and-Feel JMenu layout with radio bullets, check marks and icons in Nimbus Look-and-Feel

If none of JCheckBoxMenuItem and JRadioButtonMenuItem has an icon, both check marks / bullets and icons share the same first column.

@aivanov-jdk
Copy link
Member

As I mentioned accelerators column in my previous comment, I realised that I hadn't tested accelerators so far.

Indeed, accelerators rendering is broken now:

JMenu showing accelerators are rendered on top of menu item text

This proves that the layout isn't right. You're trying to fit both check mark / bullet and an icon into the same allocated space by moving text to the right without reserving additional width for the icon. At the same time, you move the check mark / bullet to the left. Such approach doesn't respect the default insets added on the left and right of a menu item.

The popup menu has to become wider if both check marks / bullets and icons are rendered at the same time.

@@ -143,6 +155,128 @@ public static RepaintManager getDelegateRepaintManager(Component
return delegate;
}

public static void applyInsets(Rectangle rect, Insets insets) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't a similar helper function already exists…

Comment on lines +162 to +163
rect.width -= (insets.right + rect.x);
rect.height -= (insets.bottom + rect.y);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rect.width -= (insets.right + rect.x);
rect.height -= (insets.bottom + rect.y);
rect.width -= (insets.right + insets.left);
rect.height -= (insets.bottom + insets.top);

The formula doesn't look right. Why do you subtract rect.x and rect.y instead of insets.left and insets.top?

This would work correctly if both rect.x and rect.y are zero, but it would give wrong results in other cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was the same calculation used in BasicMenuItemUI which is now moved to SwingUtilities3 so that it can be called from Basic and WindowsMenuItemUI to avoid code duplication....you can compare the contents before the fix.....in this specific case, rect.x is inclusive of insets.left..

Similarly other SwingUtilities3 changes are also as it was in BasicMenuItemUI, it was just moved verbatim from BasicMenuItemUI to it so that it can be called from both Basic and WindowsMenuItemUI class with no changes..nothing more and nothing less...probably it can be optimized but I wanted to keep the call and the content in each method in SwingUtilities3 same as it was in BasicMenuItemUI

Comment on lines +75 to +77
private static Color disabledForeground;
private static Color acceleratorSelectionForeground;
private static Color acceleratorForeground;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these static? I'm pretty sure the colors can be menu item specific, although more commonly they would be L&F specific.

Comment on lines +719 to +723
SwingUtilities3.setDisabledForeground(disabledForeground);
SwingUtilities3.setAcceleratorSelectionForeground(
acceleratorSelectionForeground);
SwingUtilities3.setAcceleratorForeground(acceleratorForeground);
SwingUtilities3.paintAccText(g, lh, lr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really weird way… Pass the colors explicitly as parameters to the SwingUtilities3.paintAccText method.

Comment on lines +71 to +73
private static Color disabledForeground;
private static Color acceleratorSelectionForeground;
private static Color acceleratorForeground;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment on lines +133 to +136
/**
* Paint MenuItem.
*/
protected void paintMenuItem(Graphics g, JComponent c,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* Paint MenuItem.
*/
protected void paintMenuItem(Graphics g, JComponent c,
@Override
protected void paintMenuItem(Graphics g, JComponent c,

Comment on lines +78 to +81
/**
* Paint MenuItem.
*/
protected void paintMenuItem(Graphics g, JComponent c,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* Paint MenuItem.
*/
protected void paintMenuItem(Graphics g, JComponent c,
@Override
protected void paintMenuItem(Graphics g, JComponent c,

Comment on lines 250 to 262
String acceleratorDelimiter =
UIManager.getString("MenuItem.acceleratorDelimiter");
if (acceleratorDelimiter == null) { acceleratorDelimiter = "+"; }
Font acceleratorFont = UIManager.getFont("MenuItem.acceleratorFont");
if (acceleratorFont == null) {
acceleratorFont = UIManager.getFont("MenuItem.font");
}
MenuItemLayoutHelper lh = new MenuItemLayoutHelper(mi, checkIcon,
arrowIcon, viewRect, defaultTextIconGap, acceleratorDelimiter,
mi.getComponentOrientation().isLeftToRight(), mi.getFont(),
acceleratorFont, MenuItemLayoutHelper.useCheckAndArrow(menuItem),
prefix);
MenuItemLayoutHelper.LayoutResult lr = lh.layoutMenuItem();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment on lines 917 to 927
if (!isWindows11OrLater) {
skin.paintSkin(g, x, y,
getIconWidth(), getIconHeight(), backgroundState);
if (icon == null) {
skin = xp.getSkin(c, part);
skin.paintSkin(g, x + OFFSET, y + OFFSET, state);
}
} else {
skin.paintSkin(g, x - 2 * OFFSET, y,
getIconWidth(), getIconHeight(), backgroundState);
skinWidth = getIconWidth();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

public static void paintCheckIcon(Graphics g, MenuItemLayoutHelper lh,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest creating a new sun.swing.MenuItemRenderHelper class similar to sun.swing.MenuItemLayoutHelper to keep both layout and renderer closer together.

Comment on lines +917 to 928
if (!isWindows11OrLater) {
skin.paintSkin(g, x, y,
getIconWidth(), getIconHeight(), backgroundState);
if (icon == null) {
skin = xp.getSkin(c, part);
skin.paintSkin(g, x + OFFSET, y + OFFSET, state);
}
} else {
skin.paintSkin(g, x, y,
getIconWidth(), getIconHeight(), backgroundState);
skinWidth = getIconWidth();
skin = xp.getSkin(c, part);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I expect to see here is that the skin is always painted at (x + OFFSET, y + OFFSET)

skin.paintSkin(g, x + OFFSET, y + OFFSET, state);

in both Windows 10 and 11.

The check mark and bullet have to remain on their own designated place.

Comment on lines 935 to 940
if (!isWindows11OrLater) {
icon.paintIcon(c, g, x + OFFSET, y + OFFSET);
} else {
icon.paintIcon(c, g, x + icon.getIconWidth(),
y + OFFSET);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an icon is present, it gets painted instead of check mark or bullet in Windows 10.

In Windows 11 the icon gets painted on

  • on the same location in regular case when the current popup menu contains no check or radio menu items with an icon;
  • on its own column with has proper large margins when there's at least one check or radio menu item with an icon; the size of the entire popup has to increase to allocate space for the icon column.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client [email protected] rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

6 participants