Skip to content
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

6829250: Reg test: java/awt/Toolkit/ScreenInsetsTest/ScreenInsetsTest.java fails in Windows #8314

Closed
wants to merge 1 commit into from

Conversation

azuev-java
Copy link
Member

@azuev-java azuev-java commented Apr 20, 2022

Only check that insets of the fully expanded undecorated window is not bigger than device insets. They can be smaller, it is a normal situation.


Progress

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

Issue

  • JDK-6829250: Reg test: java/awt/Toolkit/ScreenInsetsTest/ScreenInsetsTest.java fails in Windows

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8314/head:pull/8314
$ git checkout pull/8314

Update a local copy of the PR:
$ git checkout pull/8314
$ git pull https://git.openjdk.java.net/jdk pull/8314/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8314

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8314.diff

….java fails in Windows

Only check that insets of the fully expanded undecorated window is not
bigger than device insets. They can be smaller, it is a normal
situation.
@bridgekeeper
Copy link

bridgekeeper bot commented Apr 20, 2022

👋 Welcome back kizune! 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 openjdk bot added the rfr Pull request is ready for review label Apr 20, 2022
@openjdk
Copy link

openjdk bot commented Apr 20, 2022

@azuev-java 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 client client-libs-dev@openjdk.org label Apr 20, 2022
@mlbridge
Copy link

mlbridge bot commented Apr 20, 2022

Webrevs

@mrserb
Copy link
Member

mrserb commented Apr 27, 2022

I as far as I remember the same behavior was on old windows and jdk7 or even jdk6, the root cause is that awt does not support the "MAXIMIZED_BOTH"/etc states for the undecorated frames, we should implement it similar to this https://bugs.openjdk.java.net/browse/JDK-8176359

@azuev-java
Copy link
Member Author

I as far as I remember the same behavior was on old windows and jdk7 or even jdk6, the root cause is that awt does not support the "MAXIMIZED_BOTH"

The MAXIMIZED_BOTH for undecorated frames works, however, for undecorated frames it allows window to overlap the taskbar so the window occupies the entirety of the screen. I do not think it is a bug.

@mrserb
Copy link
Member

mrserb commented Apr 28, 2022

The MAXIMIZED_BOTH for undecorated frames works, however, for undecorated frames it allows window to overlap the taskbar so the window occupies the entirety of the screen. I do not think it is a bug.

It is a bug, it prevents the creation of the custom window decorations which behave like the native ones since the maximization action does not work. We have a number of JBS bugs for that, this one, then 4737788, and others see "relates to/duplicates"

@openjdk
Copy link

openjdk bot commented May 9, 2022

@azuev-java 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:

6829250: Reg test: java/awt/Toolkit/ScreenInsetsTest/ScreenInsetsTest.java fails in Windows

Reviewed-by: prr

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 405 new commits pushed to the master branch:

  • 837928b: 8222323: ChildAlwaysOnTopTest.java fails with "RuntimeException: Failed to unset alwaysOnTop"
  • 397d095: 8285743: Ensure each IntegerPolynomial object is only created once
  • 29ccb8f: 8285914: AppCDS crash when using shared archive with old class file
  • fe6e0c0: 8286371: Avoid use of deprecated str[n]icmp
  • 97a9835: 8274517: java/util/DoubleStreamSums/CompensatedSums.java fails with expected [true] but found [false]
  • 034f20f: 8212136: Remove finalizer implementation in SSLSocketImpl
  • 36e4df9: 8285516: clearPassword should be called in a finally try block
  • b849efd: 8285923: [REDO] JDK-8285802 AArch64: Consistently handle offsets in MacroAssembler as 64-bit quantities
  • f143386: 8286293: Tests ShortResponseBody and ShortResponseBodyWithRetry should use less resources
  • 64b05cc: 8286346: 3-parameter version of AllocateHeap should not ignore AllocFailType
  • ... and 395 more: https://git.openjdk.java.net/jdk/compare/4451257b1432e4180a16757aafca6141b8063772...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label May 9, 2022
@azuev-java
Copy link
Member Author

It is a bug, it prevents the creation of the custom window decorations which behave like the native ones

This is exactly the way native applications are working so should we treat this as a bug is questionable. Many applications - namely games and video editing software - using this feature on Windows to create a borderless window that covers the entire screen basically engaging in a "soft full screen mode" where no additional virtual screen is created. So if anything this feature would require some way of controlling of should we honor the native Windows behavior or not and i see it as that - a new feature that might be never implemented. Said that - this test is not valid because it does not test the behavior of the window - it tests the correct calculation of screen insets by assuming that window behaves to the set of properties in a certain way which it does not. So i am going to fix the test so it works with a current state of the Frame.

@azuev-java
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented May 13, 2022

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

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label May 13, 2022
@openjdk openjdk bot closed this May 13, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 13, 2022
@openjdk
Copy link

openjdk bot commented May 13, 2022

@azuev-java Pushed as commit f56396f.

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

@mrserb
Copy link
Member

mrserb commented May 14, 2022

That a mistake to consider that as a correct behavior even based on the amount of bugs we have.
The MAXIMIZED_BOTH property was considered as a "maximize" action, and should allow such behavior for the custom application when the decorations are rendered by the app. You can check all that bugs and see how many people report it and then accept, for example this one: https://bugs.openjdk.java.net/browse/JDK-4976497 which shows that even Swing is broken if setDefaultLookAndFeelDecorated is used.

Note that this test is not for the borderless window which should cover the full screen but specifically for the MAXIMIZED_BOTH action which expected to maximize the window in the same way as for decorated frame. And I think this is how it works on other platforms since the test passed there. This is why this test created 20 years ago was not changed, it was expected that we fix the JDK.

I suggest to rollback this and apply the fix to JDK.

@azuev-java
Copy link
Member Author

That a mistake to consider that as a correct behavior even based on the amount of bugs we have. The MAXIMIZED_BOTH property was considered as a "maximize" action, and should allow such behavior for the custom application when the decorations are rendered by the app. You can check all that bugs and see how many people report it and then accept, for example this one: https://bugs.openjdk.java.net/browse/JDK-4976497 which shows that even Swing is broken if setDefaultLookAndFeelDecorated is used.

Note that this test is not for the borderless window which should cover the full screen but specifically for the MAXIMIZED_BOTH action which expected to maximize the window in the same way as for decorated frame. And I think this is how it works on other platforms since the test passed there. This is why this test created 20 years ago was not changed, it was expected that we fix the JDK.

I suggest to rollback this and apply the fix to JDK.

You probably meant https://bugs.openjdk.java.net/browse/JDK-4737788 because 4976497 is closed as a duplicate of it.

And i still think it is debatable if this behavior constitutes a bug by itself. As pointed out in 2004:

"I think, there is no problem in covering taskbar after
maximizing for undecorated frames. For example, Internet Explorer covers
taskbar after pressing F11. I think, this is a normal behaviour.
But may be we should modify documentation to describe this."

So this is at beast a questionable feature that might not be implemented ever. I will not roll back this test change but i will ad the comment to the JDK-4737788 pointing out that change in behavior can warrant revisiting of this test should this bug be addressed at any point in time. There, i just did that.

@mrserb
Copy link
Member

mrserb commented May 24, 2022

No I mean the JDK-4976497 which was closed as duplicate of another one(as well as some others). It has a test case which shows that the Swing does not work as expected when the custom decorations are used. Note that bug is not directly related to the case when the undercoated window cover or not the taskbar this could be done by simple setSize() method, but only to the case when we maximize such window.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client client-libs-dev@openjdk.org integrated Pull request has been integrated
3 participants