RFR: JDK-8311113: Remove invalid pointer cast and clean up setLabel() in awt_MenuItem.cpp

Wed Aug 16 18:34:18 UTC 2023

On Mon, 14 Aug 2023 18:03:21 GMT, Harshitha Onkar <honkar at openjdk.org> wrote:

> In awt_MenuItem.cpp (712,22): ` mii.dwTypeData = (LPTSTR)(*sb)`  produces invalid pointer cast warning when complied on clang and moreover this is a no-op.  
> 
> `mii.dwTypeData` is used only when **MIIM_STRING** flag is set in the fMask (as per [Docs](https://learn.microsoft.com/en-us/windows/win32/api/winuser/ns-winuser-menuiteminfoa)), which is not the case in JDK [Ln#705](https://github.com/openjdk/jdk/blob/e56d3bc2dab3d32453b6eda66e8434953c436084/src/java.desktop/windows/native/libawt/windows/awt_MenuItem.cpp#L706). Hence the assignment ` mii.dwTypeData = (LPTSTR)(*sb)`  is not required and so is the label parameter. Additionally necessary cleanup is done at the following places -
> 
> - WMenuItemPeer.java - to the native function call
> - awt_MenuItem.cpp -  `WMenuItemPeer__1setLabel() ,_SetLabel(), SetLabel()`
> - awt_MenuItem.h
> 
> Added a test which checks setLabel() functionality on Menu, MenuItem and PopupMenu.

Changes requested by aivanov (Reviewer).

src/java.desktop/windows/native/libawt/windows/awt_MenuItem.cpp line 708:

> 706:               | MIIM_STATE | MIIM_SUBMENU | MIIM_TYPE;
> 707: 
> 708:     ::GetMenuItemInfo(hMenu, GetID(), FALSE, &mii);

Should we update the `fMask` to use `MIIM_FTYPE` instead of `MIIM_TYPE`? The latter requests both `fType` and `dwTypeData` whereas the former requests only `fType`, as we don't use `dwTypeData`.

(I understand that this code was written before Windows introduced new values for `fMask` field, which probably happened in Windows 98. Now we can update the code to document our intentions clearer.)

src/java.desktop/windows/native/libawt/windows/awt_MenuItem.cpp line 799:

> 797:         delete sls;
> 798: 
> 799:         if (badAlloc) {

~~You should preserve the formatting here, it follows the style used consistently in the file.~~

The `badAlloc` is unused now, its value is always zero, you should remove the variable.

src/java.desktop/windows/native/libawt/windows/awt_MenuItem.cpp line 983:

> 981:  * Class:     sun_awt_windows_WMenuItemPeer
> 982:  * Method:    _setLabel
> 983:  * Signature: (Ljava/lang/String;)V

Suggestion:

 * Signature: ()V

The string object is passed no more.

test/jdk/java/awt/MenuItem/SetLabelTest.java line 1:

> 1: /*

In my opinion, the test doesn't verify what we want to verify.

There are three scenarios:

1. Updating the label of a top-level menu (`Menu` object) which is displayed on the menu bar. It's done by the test: "Menu-1" becomes "New Menu-1".
2. Updating the label of a menu item in a displayed menu. Like this, robot clicks "Menu-1" to display the popup menu¹ which lists the `MenuItem` objects added to a `Menu` object. While the menu is displayed, the label changes from "MI-1" to "New MI-1". The user should see the updated label right away.
3. It's similar to the second case, yet here you display a popup menu¹ using a `PopupMenu` object. The objects of `PopupMenu` class aren't usually added to the menu bar, instead they keep a list of menu items which are displayed when user click the right mouse button or presses the _"context menu"_ key or shortcut (`Shift+F10`) on the keyboard. So, the popup menu is shown, and the label of one of its menu items is updated.

¹ In Windows, the menus that drop down when you click a menu item on the menu bar are also called popup menus. Essentially, these are the same structures as the popup menu shown in response to right-click. In AWT, the menu bar consists of `Menu` objects, and `PopupMenu` represents a type of menu the can be shown by right-click; `PopupMenu` is a subclass of `Menu`, therefore it can be added to the menu bar but it shouldn't.

test/jdk/java/awt/MenuItem/SetLabelTest.java line 82:

> 80:             robot.mouseMove(frameLoc.x + 35, frameLoc.y + 90);
> 81:             robot.mousePress(InputEvent.BUTTON1_DOWN_MASK);
> 82:             robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK);

I don't think there's a need to click a menu item to make the label be changed.

In fact, the test fails for me on a High DPI display even with `-Dsun.java2d.uiScale=1`. The displayed menus are smaller, yet the title bar remains large and the robot misses the menu items. At the same time, robot is able to open both the first and the second menus if I run the test without `uiScale`, but it fails to click "Change" menu items (likely because the menu items aren't large enough for a High DPI display).

test/jdk/java/awt/MenuItem/SetLabelTest.java line 97:

> 95:             captureScreen("Img_2");
> 96: 
> 97:             if(!checkLabels()) {

Suggestion:

            if (!checkLabels()) {

test/jdk/java/awt/MenuItem/SetLabelTest.java line 147:

> 145:             MenuItem mItem1 = m1.getItem(0);
> 146:             mItem1.setLabel(labels[1]);
> 147:     }

Suggestion:

    private static void changeLabels(int menuIndex, String[] labels) {
        Menu m1 = mb.getMenu(menuIndex);
        m1.setLabel(labels[0]);
        MenuItem mItem1 = m1.getItem(0);
        mItem1.setLabel(labels[1]);
    }

test/jdk/java/awt/MenuItem/SetLabelTest.java line 161:

> 159:         }
> 160:         return passed;
> 161:     }

My only concern here is that the test doesn't ensure that the menu has updated on the screen.

The data inside the Java object are always updated?

The test should probably make a screenshot before a label is changed, then make a screenshot after the label is changed — there should be differences on the screenshots.

test/jdk/java/awt/MenuItem/SetLabelTest.java line 176:

> 174:         try {
> 175:             ImageIO.write(
> 176:                     robot.createScreenCapture(new Rectangle(0, 0, screenSize.width, screenSize.height)),

Suggestion:

                    robot.createScreenCapture(new Rectangle(screenSize)),

-------------

PR Review: https://git.openjdk.org/jdk/pull/15276#pullrequestreview-1581031454
PR Review Comment: https://git.openjdk.org/jdk/pull/15276#discussion_r1296188625
PR Review Comment: https://git.openjdk.org/jdk/pull/15276#discussion_r1296174804
PR Review Comment: https://git.openjdk.org/jdk/pull/15276#discussion_r1296177573
PR Review Comment: https://git.openjdk.org/jdk/pull/15276#discussion_r1296274461
PR Review Comment: https://git.openjdk.org/jdk/pull/15276#discussion_r1296280897
PR Review Comment: https://git.openjdk.org/jdk/pull/15276#discussion_r1296191002
PR Review Comment: https://git.openjdk.org/jdk/pull/15276#discussion_r1296192963
PR Review Comment: https://git.openjdk.org/jdk/pull/15276#discussion_r1296211626
PR Review Comment: https://git.openjdk.org/jdk/pull/15276#discussion_r1296196763