8265586: [windows] last button is not shown in AWT Frame with BorderLayout and MenuBar set. #9118

honkar-jdk · 2022-06-10T05:55:35Z

Due to incorrect AWT Frame inset values being returned from native code, few of the components in the frame were not being shown completely on Windows. With the proposed fix, correct insets are obtained which in turn sizes and displays the frame correctly.

The default insets obtained from the Win system was adding only ::GetSystemMetrics(SM_CXSIZEFRAME) for WS_THICKFRAME and the insets were off by few pixels from the expected value. ::GetSystemMetrics(SM_CXPADDEDBORDER) is additionally added to top, bottom, left and right insets to account for the 6px. GetSystemMetric() Document

A test case is added which checks if the actual frame size is equal to the expected frame size (frame.getSize() == frame.getPreferredSize()), thus checking if frame.pack() works as expected.

Following are before and after screenshots -

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-8265586: [windows] last button is not shown in AWT Frame with BorderLayout and MenuBar set.

Reviewers

Alexey Ivanov (@aivanov-jdk - Reviewer)
Dmitry Markov (@dmarkov20 - Reviewer)
Alexander Zvegintsev (@azvegint - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9118

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

Using diff file

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

bridgekeeper · 2022-06-10T05:56:23Z

👋 Welcome back honkar! 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 · 2022-06-10T06:00:50Z

@honkar-jdk 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.

honkar-jdk · 2022-06-10T06:02:51Z

src/java.desktop/windows/native/libawt/windows/awt_Window.cpp

+    // add extra padded border only when Win OS version is 6+
+    int extraPaddedBorderInsets = info.dwMajorVersion >= 6 ?
+     ::GetSystemMetrics(SM_CXPADDEDBORDER) : 0;
+


The OS version check is added since SM_CXPADDEDBORDER metric is not supported on Win XP & 2000. If the current JDK Window target OS version is set to Windows Vista, probably this check is not required or redundant. Suggestions would be helpful as to whether to retain this check or not

Screenshot source: GetSystemMetrics Doc

You can look at the oficially supported versions of operating systems by different Java versions here: https://www.oracle.com/java/technologies/javase-subscription/documentation.html#sysconfig
Nothing even remotely relevant supports Windows XP.

@azuev-java Thank you for clarifying and sharing the link. It's a good reference for future as well.

mlbridge · 2022-06-10T06:05:44Z

Webrevs

aivanov-jdk · 2022-06-10T11:49:10Z

src/java.desktop/windows/native/libawt/windows/awt_Window.cpp

+    OSVERSIONINFOEX info;
+    ZeroMemory(&info, sizeof(OSVERSIONINFOEX));
+    info.dwOSVersionInfoSize = sizeof(OSVERSIONINFOEX);
+    GetVersionEx((LPOSVERSIONINFO) &info);


You don't need to check Windows version. Windows XP and 2000 are unsupported, java.exe doesn't even start there because its exe file header requires version 6.0 (Vista).

The lowest version supported by Microsoft is Windows 10, Windows 8.1 will reach its end of extended support in January 2023.

Thus, it's safe to call ::GetSystemMetrics(SM_CXPADDEDBORDER).

This issue came up after Windows SDK had been upgraded, which led to /subsystem version being set to 6.0. When targeting to previous versions, the SM_CXPADDEDBORDER is excluded from the non-client area of a window.

aivanov-jdk · 2022-06-10T11:53:16Z

test/jdk/java/awt/Frame/AwtFramePackTest.java

+            frame.setMenuBar(mb);
+
+            frame.pack();
+            frame.setVisible(true);


You should call Robot.waitForIdle() after making the frame visible. Calling it before you created the frame has no effect.

aivanov-jdk · 2022-06-10T11:54:24Z

test/jdk/java/awt/Frame/AwtFramePackTest.java

+        BufferedImage image = robot.createScreenCapture(
+                new Rectangle(0, 0, 500, 500));


You probably want to capture frame.getBounds().

aivanov-jdk · 2022-06-10T11:55:59Z

test/jdk/java/awt/Frame/AwtFramePackTest.java

+        BufferedImage image = robot.createScreenCapture(
+                new Rectangle(0, 0, 500, 500));
+        try {
+            ImageIO.write(image, "png", new File("Frame"));


Suggested change

ImageIO.write(image, "png", new File("Frame"));

ImageIO.write(image, "png", new File("Frame.png"));

Add extension to the image file.

aivanov-jdk · 2022-06-10T11:57:10Z

test/jdk/java/awt/Frame/AwtFramePackTest.java

+ * @test
+ * @bug 8265586
+ * @key headful
+ * @summary Tests the functionality of AWT frame.pack() on Windows.


Could you be more specific? It tests the insets of a frame are calculated correctly on Windows.

aivanov-jdk · 2022-06-10T11:59:24Z

test/jdk/java/awt/Frame/AwtFramePackTest.java

+ * @key headful
+ * @summary Tests the functionality of AWT frame.pack() on Windows.
+ * @run main AwtFramePackTest
+ * @requires (os.family == "windows")


Usually, @run is the latest tag in a test description; @requires usually goes before @summary.

aivanov-jdk · 2022-06-10T12:01:09Z

test/jdk/java/awt/Frame/AwtFramePackTest.java

+/*
+ * @test
+ * @bug 8265586
+ * @key headful
+ * @summary Tests the functionality of AWT frame.pack() on Windows.
+ * @run main AwtFramePackTest
+ * @requires (os.family == "windows")
+ */


I am for placing the test tags before the class declaration, after imports. They're easier to see when you open the file in the IDE because they aren't collapsed.

aivanov-jdk · 2022-06-10T12:01:21Z

src/java.desktop/windows/native/libawt/windows/awt_Window.cpp

                m_insets.left = m_insets.right =
                    ::GetSystemMetrics(SM_CXDLGFRAME);
                m_insets.top = m_insets.bottom =
                    ::GetSystemMetrics(SM_CYDLGFRAME);


It's likely needed here as well.

Make the frame non-resizeable to get into this block.

Added SM_CXPADDEDBORDER to Non-Resizable Frames. After the addition of padded border the second button is being shown similar to Resizable frames. But unlike resizable frames, for the non-resizable frames - the actual frame size and the preferred size are different. Ideally the frame.getSize() should be equal to preferred size when frame.pack() is used.

Side-by-Side comparison of Resizable and Non-Resizable frames - Resizable frame slightly wider and taller compared to non-resizable.

On UiScale =1,
Resizable Frame size= java.awt.Dimension[width=241,height=119]
Non-Resizable Frame size = java.awt.Dimension[width=237,height=115]

Tested the AwtWindow::UpdateInsets() method for Resizable and Non-Resizable frame.

Non-Resizable UI Scale=1

Outside Rect 0 56 0 198
Inside Rect 0 0 0 176
MenuBar Added
BEFORE Scale Down 73 9 9 9
AFTER Scale Down 73 9 9 9

Expected frame size: java.awt.Dimension[width=241,height=119]
Actual frame size: java.awt.Dimension[width=237,height=115]
Panel size: java.awt.Dimension[width=215,height=29]
Frame Insets: java.awt.Insets[top=75,left=11,bottom=11,right=11]

Non-Resizable

Outside Rect 0 56 0 198
Inside Rect 0 0 0 176
MenuBar Added
BEFORE Scale Down 73 9 9 9
AFTER Scale Down 49 6 6 6

Expected frame size: java.awt.Dimension[width=231,height=90]
Actual frame size: java.awt.Dimension[width=229,height=88]
Panel size: java.awt.Dimension[width=215,height=31]
Frame Insets: java.awt.Insets[top=50,left=7,bottom=7,right=7]

Resizable UI Scale=1

Outside Rect 0 56 0 198
Inside Rect 0 0 0 176
MenuBar Added
BEFORE Scale Down 75 11 11 11
AFTER Scale Down 75 11 11 11

Expected frame size: java.awt.Dimension[width=241,height=119]
Actual frame size: java.awt.Dimension[width=241,height=119]
Panel size: java.awt.Dimension[width=219,height=33]
Frame Insets: java.awt.Insets[top=75,left=11,bottom=11,right=11]

Resizable

Outside Rect 0 56 0 198
Inside Rect 0 0 0 176
MenuBar Added
BEFORE Scale Down 75 11 11 11
AFTER Scale Down 50 7 7 7

Expected frame size: java.awt.Dimension[width=231,height=90]
Actual frame size: java.awt.Dimension[width=231,height=90]
Panel size: java.awt.Dimension[width=217,height=33]
Frame Insets: java.awt.Insets[top=50,left=7,bottom=7,right=7]

Based on these logs, I observed that the final frame insets for non-resizable frame obtained by frame.getInsets() does not match with the scaled down insets where as for resizable frame it does.

Clarification Point - should the preferred size of a resizable and non-resizable frame be the same when frame.pack() is used? The default insets obtained for both are from different system metrics - ::GetSystemMetrics(SM_CYSIZEFRAME) & ::GetSystemMetrics(SM_CYDLGFRAME) respectively.

Clarification Point - should the preferred size of a resizable and non-resizable frame be the same when frame.pack() is used? The default insets obtained for both are from different system metrics - ::GetSystemMetrics(SM_CYSIZEFRAME) & ::GetSystemMetrics(SM_CYDLGFRAME) respectively.

Do ::GetSystemMetrics(SM_CYSIZEFRAME) and ::GetSystemMetrics(SM_CYDLGFRAME) return the same value? If yes, then I expect the size of the frame should be the same for resizable and non-resizable case.

Let's put it another way, I expect the pack() makes the panel size to its preferred size.

In either case, the frame.getPreferredSize() returns the same result but the actual size differs.

I don't consider it a blocker for this integration: the resizable case is handled perfectly now; the non-resizable case also ensures the second button is visible now.

However, I think we should find out what contributes to the difference in the actual size under a separate bugid.

@aivanov-jdk Thank you for clarifying. Yes ::GetSystemMetrics(SM_CYSIZEFRAME) and ::GetSystemMetrics(SM_CYDLGFRAME) return different values, thus we get different actual frame sizes for resizable and non-resizable frames.

As you rightly pointed out : the frame.getPreferredSize() returns the same result but the actual size differs for Non-Resizable frame.

As suggested I will create a new bug to track actual and preferred size difference for non-resizable frame.

@aivanov-jdk Thank you for clarifying. Yes ::GetSystemMetrics(SM_CYSIZEFRAME) and ::GetSystemMetrics(SM_CYDLGFRAME) return different values, thus we get different actual frame sizes for resizable and non-resizable frames.

If these are different, why does the preferred size remain the same?

If these parameters return different values, the preferred size should be different.

As you rightly pointed out : the frame.getPreferredSize() returns the same result but the actual size differs for Non-Resizable frame.

As suggested I will create a new bug to track actual and preferred size difference for non-resizable frame.

Thank you!

If these are different, why does the preferred size remain the same?

The updateInsets() is called multiple times depending on how many child components are present such as panel and buttons, each time the insets are updated accordingly. I believe the insets obtained for non-resizable frame is being overridden, thus causing preferred size to return wrong values for non-resizable frame.

My guess is it might be due to old or stale value of m_old_insets in this part of the code. Investigating it further.

insetsChanged = !::EqualRect( &m_old_insets, &m_insets ); ::CopyRect( &m_old_insets, &m_insets ); if (insetsChanged) { // Since insets are changed we need to update the surfaceData object // to reflect that change env->CallVoidMethod(peer, AwtComponent::replaceSurfaceDataLaterMID); } return insetsChanged;

However, I think we should find out what contributes to the difference in the actual size under a separate bugid.

Tracked by JDK-8288325

aivanov-jdk · 2022-06-10T12:09:11Z

test/jdk/java/awt/Frame/AwtFramePackTest.java

+                saveScreenCapture();
+                throw new RuntimeException("Expected and Actual frame size" +
+                        " are different. frame.pack() does not work!!");
+           }


One space of indent is missing before the closing brace.

aivanov-jdk · 2022-06-10T12:12:24Z

test/jdk/java/awt/Frame/AwtFramePackTest.java

+        robot.delay(500);
+        robot.waitForIdle();


I don't think another delay and waitForIdle are required here.

The frame should be properly displayed before frame.getSize() is called.

honkar-jdk · 2022-06-10T15:50:59Z

@aivanov-jdk Thank you for reviewing. I'll have the suggested changes fixed in the next commit.

aivanov-jdk · 2022-06-13T19:57:29Z

src/java.desktop/windows/native/libawt/windows/awt_Window.cpp

+                    ::GetSystemMetrics(SM_CXSIZEFRAME) +
+                     extraPaddedBorderInsets;


If you put them on the same line, it nearly fits 80-column limit. Does it make sense to do so?

aivanov-jdk · 2022-06-13T19:58:37Z

src/java.desktop/windows/native/libawt/windows/awt_Window.cpp

 #include <jlong.h>
+#include <windows.h>


Probably windows.h is not required… It compiled successfully without it, it should continue to compile now

openjdk · 2022-06-13T21:18:22Z

@honkar-jdk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8265586: [windows] last button is not shown in AWT Frame with BorderLayout and MenuBar set.

Reviewed-by: aivanov, dmarkov, azvegint

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 68 new commits pushed to the master branch:

fb29770: 8287186: JDK modules participating in preview
0530f4e: 8288094: cleanup old _MSC_VER handling
1a65332: 8287906: Rewrite of GitHub Actions (GHA) sanity tests
c2ccf4c: 8288003: log events for os::dll_unload
03dca56: 8287525: Extend IR annotation with new options to test specific target feature.
86c9241: 8287028: AArch64: [vectorapi] Backend implementation of VectorMask.fromLong with SVE2
fbe9266: 8288378: [BACKOUT] DST not applying properly with zone id offset set with TZ env variable
1904353: Merge
7aafc69: 8288105: [PPC64] Problems with -XX:+VerifyStack
f4b05a1: 8288173: JDK-8202449 fix causes conformance test failure : api/java_util/Random/RandomGenerator/NextFloat.html
... and 58 more: https://git.openjdk.org/jdk/compare/130ce7c6b8998764f1a9ce5d5c6d60f053511991...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@aivanov-jdk, @dmarkov20, @azvegint) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

aivanov-jdk · 2022-06-14T11:05:58Z

/sponsor

openjdk · 2022-06-14T11:06:16Z

@aivanov-jdk The change author (@honkar-jdk) must issue an integrate command before the integration can be sponsored.

aivanov-jdk · 2022-06-14T11:10:29Z

@aivanov-jdk The change author (@honkar-jdk) must issue an integrate command before the integration can be sponsored.

I was under the impression that the integrate command had been issued already.

honkar-jdk · 2022-06-14T17:23:26Z

/integrate

openjdk · 2022-06-14T17:24:52Z

@honkar-jdk
Your change (at version fd65365) is now ready to be sponsored by a Committer.

dmarkov20 · 2022-06-14T19:46:26Z

/sponsor

openjdk · 2022-06-14T19:48:18Z

Going to push as commit bbaeacb.
Since your change was applied there have been 69 commits pushed to the master branch:

0f58097: 8288134: Super class names don't have envelopes
fb29770: 8287186: JDK modules participating in preview
0530f4e: 8288094: cleanup old _MSC_VER handling
1a65332: 8287906: Rewrite of GitHub Actions (GHA) sanity tests
c2ccf4c: 8288003: log events for os::dll_unload
03dca56: 8287525: Extend IR annotation with new options to test specific target feature.
86c9241: 8287028: AArch64: [vectorapi] Backend implementation of VectorMask.fromLong with SVE2
fbe9266: 8288378: [BACKOUT] DST not applying properly with zone id offset set with TZ env variable
1904353: Merge
7aafc69: 8288105: [PPC64] Problems with -XX:+VerifyStack
... and 59 more: https://git.openjdk.org/jdk/compare/130ce7c6b8998764f1a9ce5d5c6d60f053511991...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-06-14T19:48:37Z

@dmarkov20 @honkar-jdk Pushed as commit bbaeacb.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

mrserb · 2022-06-15T05:37:42Z

test/jdk/java/awt/Frame/AwtFramePackTest.java

+ * @test
+ * @bug 8265586
+ * @key headful
+ * @requires (os.family == "windows")


Any reason why the test is windows specific? It does not have the platform specific code.

@mrserb I wanted to clarify if you meant - Though the fix is windows specific, the condition used in test case frame.getSize() == frame.getPreferredSize() should hold on all platforms when frame.pack() is used and the test case can be made generic?

@mrserb Since the PR was already integrated before the review suggestion was made to make the test generic, a new PR has been created to remove the @requires tag from the test.

PR link: #9248
JBS bug: https://bugs.openjdk.org/browse/JDK-8288993

honkar-jdk added 2 commits June 9, 2022 22:12

AWT frame.pack fix

956e2e4

removed extra newline

afe3afb

openjdk bot changed the title ~~JDK-8265586: [windows] last button is not shown in AWT Frame with BorderLayout and MenuBar set~~ 8265586: [windows] last button is not shown in AWT Frame with BorderLayout and MenuBar set. Jun 10, 2022

openjdk bot added the rfr Pull request is ready for review label Jun 10, 2022

openjdk bot added the client client-libs-dev@openjdk.org label Jun 10, 2022

honkar-jdk commented Jun 10, 2022

View reviewed changes

aivanov-jdk suggested changes Jun 10, 2022

View reviewed changes

removed os version check, added review changes

a6a2e42

aivanov-jdk reviewed Jun 13, 2022

View reviewed changes

minor changes

fd65365

aivanov-jdk approved these changes Jun 13, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 13, 2022

dmarkov20 approved these changes Jun 14, 2022

View reviewed changes

azvegint approved these changes Jun 14, 2022

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Jun 14, 2022

openjdk bot added the integrated Pull request has been integrated label Jun 14, 2022

openjdk bot closed this Jun 14, 2022

openjdk bot removed the ready Pull request is ready to be integrated label Jun 14, 2022

openjdk bot removed rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jun 14, 2022

mrserb reviewed Jun 15, 2022

View reviewed changes

honkar-jdk mentioned this pull request Jun 22, 2022

JDK-8288993: Make AwtFramePackTest generic by removing @requires tag #9248

Closed

3 tasks

		BufferedImage image = robot.createScreenCapture(
		new Rectangle(0, 0, 500, 500));

	ImageIO.write(image, "png", new File("Frame"));
	ImageIO.write(image, "png", new File("Frame.png"));

		::GetSystemMetrics(SM_CXSIZEFRAME) +
		extraPaddedBorderInsets;

8265586: [windows] last button is not shown in AWT Frame with BorderLayout and MenuBar set. #9118

8265586: [windows] last button is not shown in AWT Frame with BorderLayout and MenuBar set. #9118

Conversation

honkar-jdk commented Jun 10, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jun 10, 2022

openjdk bot commented Jun 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlbridge bot commented Jun 10, 2022 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honkar-jdk Jun 11, 2022 • edited Loading

Choose a reason for hiding this comment

honkar-jdk Jun 11, 2022 • edited Loading

Choose a reason for hiding this comment

honkar-jdk Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honkar-jdk commented Jun 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Jun 13, 2022 • edited Loading

aivanov-jdk commented Jun 14, 2022

openjdk bot commented Jun 14, 2022

aivanov-jdk commented Jun 14, 2022

honkar-jdk commented Jun 14, 2022

openjdk bot commented Jun 14, 2022

dmarkov20 commented Jun 14, 2022

openjdk bot commented Jun 14, 2022

openjdk bot commented Jun 14, 2022

Choose a reason for hiding this comment

honkar-jdk Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honkar-jdk commented Jun 10, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 10, 2022 •

edited

Loading

honkar-jdk Jun 11, 2022 •

edited

Loading

honkar-jdk Jun 11, 2022 •

edited

Loading

honkar-jdk Jun 13, 2022 •

edited

Loading

openjdk bot commented Jun 13, 2022 •

edited

Loading

honkar-jdk Jun 15, 2022 •

edited

Loading