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
8267307: Introduce new client property for XAWT: xawt.mwm_decor_title #4113
Conversation
/covered |
Hi @mkartashev, welcome to this OpenJDK project and thanks for contributing! We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user mkartashev" as summary for the issue. If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing |
Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated! |
@mkartashev The following label will be automatically applied to this pull request:
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. |
Webrevs
|
Hello, thank you for your contribution, One initial question: what the name "mwm_" of this property actually means? |
Mailing list message from Maxim Kartashev on awt-dev: On Fri, May 21, 2021 at 12:33 AM Sergey Bylokhov <serb at openjdk.java.net>
The name's historical: it's from the _MOTIF_WM_HINTS property of X11 that $ xprop -id 0x2600003 | grep -i MOTIF_WM_HINTS Despite the name ("motif"), this property has apparently become a de-facto -------------- next part -------------- |
1 similar comment
Mailing list message from Maxim Kartashev on awt-dev: On Fri, May 21, 2021 at 12:33 AM Sergey Bylokhov <serb at openjdk.java.net>
The name's historical: it's from the _MOTIF_WM_HINTS property of X11 that $ xprop -id 0x2600003 | grep -i MOTIF_WM_HINTS Despite the name ("motif"), this property has apparently become a de-facto -------------- next part -------------- |
Mailing list message from Maxim Kartashev on awt-dev: Hello All, I'd appreciate it very much if someone could find time to look at this pull On Fri, May 21, 2021 at 11:01 AM Maxim Kartashev <
-------------- next part -------------- |
1 similar comment
Mailing list message from Maxim Kartashev on awt-dev: Hello All, I'd appreciate it very much if someone could find time to look at this pull On Fri, May 21, 2021 at 11:01 AM Maxim Kartashev <
-------------- next part -------------- |
|
||
private void captureTitleBarNotVisible() { | ||
runSwing( () -> { | ||
titleBarImageNotVisible = robot.createScreenCapture(titleBarBounds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createScreenCapture
does not require to be called on EDT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Moved createScreenCapture()
out of EDT. Please, have a look at the updated code.
This fix most likely will require a CSR |
There already is one linked from the enhancement: JDK-8267308 |
/csr
Sorry, I missed that. |
@azvegint this pull request will not be integrated until the CSR request JDK-8267308 for issue JDK-8267307 has been approved. |
Please check that the test code added in the JDK-8265005 can be adapted to verify this fix as well. |
@mrserb Please clarify: do you suggest to have one common test code for this feature and JDK-8265005? This is certainly possible, but will contaminate the Mac test with Linux-specific window manager checking stuff, making it harder to debug failures, I think. |
You can reuse that test or create the new one, I just pointed out how the "same" feature was tested on mac. |
Mailing list message from Alexey Ushakov on awt-dev: Could anyone from the dev team review my CSR request for this change: https://bugs.openjdk.java.net/browse/JDK-8267308 <https://bugs.openjdk.java.net/browse/JDK-8267308> ? Best Regards, -------------- next part -------------- |
1 similar comment
Mailing list message from Alexey Ushakov on awt-dev: Could anyone from the dev team review my CSR request for this change: https://bugs.openjdk.java.net/browse/JDK-8267308 <https://bugs.openjdk.java.net/browse/JDK-8267308> ? Best Regards, -------------- next part -------------- |
} | ||
|
||
private void registerWindowDecorationChangeListener() { | ||
if (target instanceof javax.swing.RootPaneContainer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably this should be done via runtime check? Overwise the swing classes will be always loaded even if swing is not used. see the usage of SunToolkit.isInstanceOf in java.awt.WIndow.java:3965
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Updated the code; please, have a look.
…eContainer in order to avoid unnecessary class loading if it isn't. Also updated the test such that robot.createScreenCapture() is not executed on EDT.
public final Optional<Boolean> getMWMDecorTitleProperty() { | ||
Optional<Boolean> res = Optional.empty(); | ||
|
||
if (target instanceof javax.swing.RootPaneContainer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this instanceof will load the Swing as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, missed that one the first time. Corrected now.
…tPaneContainer in order to avoid unnecessary class loading if it isn't.
Looks fine, I'll run the tests. |
@mkartashev 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:
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 1079 new commits pushed to the
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 (@azvegint, @mrserb) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
/integrate |
@mkartashev |
/sponsor |
Going to push as commit 18f356a.
Your commit was automatically rebased without conflicts. |
@avu @mkartashev Pushed as commit 18f356a. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This commit introduces a new client property xawt.mwm_decor_title implementing JDK-8267307. The property can be set prior to showing a window or after the window has been displayed, in which case the window will have to be hidden/shown (re-mapped) for the property to take effect.
The general idea is to provide control over the "decorations" part of the X11 window property called _MOTIF_WM_HINTS. Those "decoration" bits are set to 1 (XWindowAttributesData.AWT_DECOR_ALL) to show all the decorations or 0 (XWindowAttributesData.AWT_DECOR_NONE) to ask the window manager (WM) not to decorate with anything, even borders or resize handles. With xawt.mwm_decor_title property set to "true", this commit adds the ability to set the bits to 2 (XWindowAttributesData.AWT_DECOR_BORDER), which some WMs take as "decorate with only a border", thus effectively removing the window's title bar, but still leaving the resize capability.
This feature was tested and works correctly on "vanilla" Ubuntu 20.04 with the "GNOME Shell" window manager. It was also tested with Xfwm4 and KDE, where it did not have any effect; these two WMs have limited respected for the "decorations" bits of the _MOTIF_WM_HINTS window property.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4113/head:pull/4113
$ git checkout pull/4113
Update a local copy of the PR:
$ git checkout pull/4113
$ git pull https://git.openjdk.java.net/jdk pull/4113/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 4113
View PR using the GUI difftool:
$ git pr show -t 4113
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4113.diff