On Fri, 10 Jun 2022 05:55:35 GMT, Harshitha Onkar <hon...@openjdk.org> wrote:

> 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 6px from the expected value. 
> `::GetSystemMetrics(SM_CXPADDEDBORDER)` is additionally added to top, bottom, 
> left and right insets to account for the 6px. [GetSystemMetric() 
> Document](https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getsystemmetrics)
> 
> 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 - 
> ![image](https://user-images.githubusercontent.com/95945681/172999272-981a0b07-2bd1-425c-b73d-f21f68922262.png)

Changes requested by aivanov (Reviewer).

src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 1402:

> 1400:     ZeroMemory(&info, sizeof(OSVERSIONINFOEX));
> 1401:     info.dwOSVersionInfoSize = sizeof(OSVERSIONINFOEX);
> 1402:     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](https://docs.microsoft.com/en-us/cpp/build/reference/subsystem-specify-subsystem?view=msvc-140)
 version being set to 6.0. When targeting to previous versions, the 
`SM_CXPADDEDBORDER` is excluded from the non-client area of a window.

src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 1440:

> 1438:                     ::GetSystemMetrics(SM_CXDLGFRAME);
> 1439:                 m_insets.top = m_insets.bottom =
> 1440:                     ::GetSystemMetrics(SM_CYDLGFRAME);

It's likely needed here as well.

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

test/jdk/java/awt/Frame/AwtFramePackTest.java line 28:

> 26:  * @bug 8265586
> 27:  * @key headful
> 28:  * @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.

test/jdk/java/awt/Frame/AwtFramePackTest.java line 30:

> 28:  * @summary Tests the functionality of AWT frame.pack() on Windows.
> 29:  * @run main AwtFramePackTest
> 30:  * @requires (os.family == "windows")

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

test/jdk/java/awt/Frame/AwtFramePackTest.java line 31:

> 29:  * @run main AwtFramePackTest
> 30:  * @requires (os.family == "windows")
> 31:  */

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.

test/jdk/java/awt/Frame/AwtFramePackTest.java line 73:

> 71: 
> 72:             frame.pack();
> 73:             frame.setVisible(true);

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

test/jdk/java/awt/Frame/AwtFramePackTest.java line 84:

> 82:                 throw new RuntimeException("Expected and Actual frame 
> size" +
> 83:                         " are different. frame.pack() does not work!!");
> 84:            }

One space of indent is missing before the closing brace.

test/jdk/java/awt/Frame/AwtFramePackTest.java line 93:

> 91:     private static void saveScreenCapture() {
> 92:         robot.delay(500);
> 93:         robot.waitForIdle();

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

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

test/jdk/java/awt/Frame/AwtFramePackTest.java line 95:

> 93:         robot.waitForIdle();
> 94:         BufferedImage image = robot.createScreenCapture(
> 95:                 new Rectangle(0, 0, 500, 500));

You probably want to capture `frame.getBounds()`.

test/jdk/java/awt/Frame/AwtFramePackTest.java line 97:

> 95:                 new Rectangle(0, 0, 500, 500));
> 96:         try {
> 97:             ImageIO.write(image, "png", new File("Frame"));

Suggestion:

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

Add extension to the image file.

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

PR: https://git.openjdk.org/jdk/pull/9118

Reply via email to