8260528: Clean glass-gtk sizing and positioning code #915

tsayao · 2022-10-13T00:48:09Z

This cleans size and positioning code, reducing special cases, code complexity and size.

Changes:

cached extents: 28, 1, 1, 1 are old defaults - modern gnome uses different sizes. It does not assume any size because it varies - it does cache because it's unlikely to vary on the same system - but if it does occur, it will only waste a resize event.
window geometry, min/max size are centralized in update_window_constraints;
Frame extents (the window decoration size used for "total window size"):
- frame extents are received in process_property_notify;
- removed quirks in java code;
- When received, call set_bounds again to adjust the size (to account decorations later received);
Removed activate_window because it's the same as focusing the window. gtk_window_present will deiconify and focus it.
ensure_window_size was a quirk - removed;
requested_bounds removed - not used anymore;
window_configure incorporated in set_bounds with gtk_window_move and gtk_window_resize;
process_net_wm_property is a work-around for Unity only (added a check if Unity - but it can probably be removed at some point)
restack split in to_front() and to_back() to conform to managed code;

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8260528: Clean glass-gtk sizing and positioning code

Reviewers

Johan Vos (@johanvos - Reviewer)
Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 915

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/915.diff

Webrev

Link to Webrev Comment

Merge master

Merge from jfx

Merge

merge from jfx

Merge upstream

Merge from upstream

Update from master

Merge from upstream

Merge with main

Merge master

Update from jfx

Update

Pull from origin

Merge master

bridgekeeper · 2022-10-13T00:49:12Z

👋 Welcome back tsayao! 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-10-13T00:50:50Z

⚠️ @tsayao This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

mlbridge · 2022-10-16T23:31:33Z

Webrevs

tsayao · 2023-02-19T18:24:31Z

It's now ready for a re-review. Black header is gone, iconify tests pass.

kevinrushforth · 2023-02-23T23:59:33Z

I can confirm that the black header area is fixed on 20.04. I'll do some final testing and code review soon.

kevinrushforth · 2023-02-24T22:51:32Z

I ran a full set of headful tests on both Ubuntu 20.04 and 22.04. The only remaining problem I see is a new failure on one test on both platforms:

$ gradle --info -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests CheckWindowOrderTest

CheckWindowOrderTest > topWindowShouldBeTheLast FAILED
    java.lang.AssertionError: Last Window Should be Focused
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.assertTrue(Assert.java:42)
        at test.robot.javafx.stage.CheckWindowOrderTest.topWindowShouldBeTheLast(CheckWindowOrderTest.java:57)

That test passes without the patch from this PR.

This reverts commit 1ab6099.

tsayao · 2023-02-26T22:05:31Z

It's working now, but I found out that calling gtk_window_move() after show() changes the "focus on map" order. It's called then frame extents are received to adjust position by gravity. It might be a bug in gtk.

With the current solution it is working, but it does send extra notifyFocusevents, as before (I refer to the code in master branch).

kevinrushforth · 2023-03-01T15:15:51Z

I can confirm that this fixes the window order test without introducing new test failures. It looks ready for final testing and review.

kevinrushforth

I have finished my testing. All looks good, with one problem seen on Ubuntu 16.04 that warrants a P4 follow-up bug (which we may or may not care to fix).

The following test fails consistently on Ubuntu 16.04 with this patch:

DeiconifiedWithChildTest > testDeiconifiedPosition FAILED
    java.lang.AssertionError: Child window was moved expected:<343.0> but was:<333.0>
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:555)
        at test.javafx.stage.DeiconifiedWithChildTest.testDeiconifiedPosition(DeiconifiedWithChildTest.java:99)

I also reviewed the code changes, and they look good (although I did leave one question for you to consider).

kevinrushforth · 2023-03-04T17:45:05Z

modules/javafx.graphics/src/main/native-glass/gtk/glass_window.cpp

    Display *display = GDK_DISPLAY_XDISPLAY(gdk_window_get_display(gdk_window));
-    Atom rfeAtom = XInternAtom(display, "_NET_REQUEST_FRAME_EXTENTS", True);
+    static Atom rfeAtom = XInternAtom(display, "_NET_REQUEST_FRAME_EXTENTS", False);


If display could be different for two Windows, then making this static would not be the right thing to do. I don't think it can, so this is probably OK.

Atoms are per server, so I also think it's ok.

kevinrushforth · 2023-03-11T14:42:34Z

@johanvos Can you re-review this when you have time?

johanvos

I looked at the diffs, and re-did my tests. All working as expected on Ubuntu 22.04.

openjdk · 2023-04-07T14:01:40Z

@tsayao 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:

8260528: Clean glass-gtk sizing and positioning code

Reviewed-by: jvos, kcr

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

2b2a7f1: 8304441: [macos] Crash when putting invalid unicode char on clipboard
b6e5737: 8283063: Optimize Observable{List/Set/Map}Wrapper.retainAll/removeAll
810bd90: 8286089: Intermittent WebKit build failure on macOS in JavaScriptCore
a264435: 8282359: Intermittent WebKit build failure on Windows: C1090: PDB API call failed, error code 23
4c0e0bd: 8305248: TableView not rendered correctly after column is made visible if fixed cell size is set

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

tsayao · 2023-04-07T14:46:43Z

/integrate

openjdk · 2023-04-07T14:48:35Z

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

2b2a7f1: 8304441: [macos] Crash when putting invalid unicode char on clipboard
b6e5737: 8283063: Optimize Observable{List/Set/Map}Wrapper.retainAll/removeAll
810bd90: 8286089: Intermittent WebKit build failure on macOS in JavaScriptCore
a264435: 8282359: Intermittent WebKit build failure on Windows: C1090: PDB API call failed, error code 23
4c0e0bd: 8305248: TableView not rendered correctly after column is made visible if fixed cell size is set

Your commit was automatically rebased without conflicts.

openjdk · 2023-04-07T14:48:52Z

@tsayao Pushed as commit 0c03a41.

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

tsayao added 22 commits January 23, 2020 08:44

Merge pull request #4 from openjdk/master

9397a74

Merge master

Merge pull request #5 from openjdk/master

37519d7

Merge from jfx

Merge pull request #6 from openjdk/master

5dccb63

Merge

Merge pull request #7 from openjdk/master

6f10582

merge from jfx

Merge pull request #8 from openjdk/master

de2d517

Merge upstream

Merge pull request #9 from openjdk/master

e1b0372

Merge from upstream

Merge pull request #10 from openjdk/master

6744719

Update from master

Merge pull request #11 from openjdk/master

04516db

Merge from upstream

Merge pull request #12 from openjdk/master

c95598e

Merge with main

Merge pull request #13 from openjdk/master

ce12ddd

Merge master

Merge pull request #14 from openjdk/master

d3dd7f4

Merge master

Merge pull request #15 from openjdk/master

d80dae4

Update from jfx

Merge pull request #16 from openjdk/master

b8aad79

Update

Merge pull request #17 from openjdk/master

b8a9498

Pull from origin

Merge pull request #18 from openjdk/master

c442d81

Merge master

Merge branch 'openjdk:master' into master

ba7572f

Merge branch 'openjdk:master' into master

e717051

Merge branch 'openjdk:master' into master

475d6e6

Merge branch 'openjdk:master' into master

2f856c2

Merge branch 'openjdk:master' into master

2a9cff1

Merge branch 'openjdk:master' into master

c37b25c

Merge branch 'openjdk:master' into master

3226441

tsayao mentioned this pull request Oct 13, 2022

8260528: Clean glass-gtk sizing and positioning code #367

Closed

3 tasks

tsayao changed the title ~~WIP: 8260528: Clean glass-gtk sizing and positioning code~~ 8260528: Clean glass-gtk sizing and positioning code Oct 16, 2022

openjdk bot added the rfr Ready for review label Oct 16, 2022

tsayao force-pushed the clean_glass_gtk branch 2 times, most recently from 9fb69ec to bfdb4ad Compare October 17, 2022 00:11

tsayao added 2 commits February 18, 2023 13:44

Fix black header glitch

29074ea

Re-fix initial size

8627e11

tsayao added 4 commits February 26, 2023 13:37

Fix window order (focus event)

1ab6099

Revert "Fix window order (focus event)"

11cdab9

This reverts commit 1ab6099.

Fix window order

16c160f

Minor adjustments

75cae8e

Fix bug when window starts maximized and restores to wrong size

d344942

kevinrushforth approved these changes Mar 6, 2023

View reviewed changes

tsayao and others added 5 commits March 21, 2023 19:29

Merge branch 'openjdk:master' into master

aef5029

Merge branch 'openjdk:master' into master

65c14ad

Merge branch 'openjdk:master' into master

9db1c59

Merge branch 'openjdk:master' into master

ff57e3b

Merge branch 'master' into clean_glass_gtk

89e4efc

kevinrushforth approved these changes Apr 3, 2023

View reviewed changes

johanvos approved these changes Apr 7, 2023

View reviewed changes

openjdk bot added the ready Ready to be integrated label Apr 7, 2023

openjdk bot added the integrated Pull request has been integrated label Apr 7, 2023

openjdk bot closed this Apr 7, 2023

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Apr 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8260528: Clean glass-gtk sizing and positioning code #915

8260528: Clean glass-gtk sizing and positioning code #915

tsayao commented Oct 13, 2022 •

edited by openjdk bot

bridgekeeper bot commented Oct 13, 2022

openjdk bot commented Oct 13, 2022

mlbridge bot commented Oct 16, 2022 •

edited

tsayao commented Feb 19, 2023

kevinrushforth commented Feb 23, 2023

kevinrushforth commented Feb 24, 2023

tsayao commented Feb 26, 2023

kevinrushforth commented Mar 1, 2023

kevinrushforth left a comment

kevinrushforth Mar 4, 2023

tsayao Mar 6, 2023

kevinrushforth commented Mar 11, 2023

johanvos left a comment

openjdk bot commented Apr 7, 2023

tsayao commented Apr 7, 2023

openjdk bot commented Apr 7, 2023

openjdk bot commented Apr 7, 2023

8260528: Clean glass-gtk sizing and positioning code #915

8260528: Clean glass-gtk sizing and positioning code #915

Conversation

tsayao commented Oct 13, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Oct 13, 2022

openjdk bot commented Oct 13, 2022

mlbridge bot commented Oct 16, 2022 • edited

Webrevs

tsayao commented Feb 19, 2023

kevinrushforth commented Feb 23, 2023

kevinrushforth commented Feb 24, 2023

tsayao commented Feb 26, 2023

kevinrushforth commented Mar 1, 2023

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth Mar 4, 2023

Choose a reason for hiding this comment

tsayao Mar 6, 2023

Choose a reason for hiding this comment

kevinrushforth commented Mar 11, 2023

johanvos left a comment

Choose a reason for hiding this comment

openjdk bot commented Apr 7, 2023

tsayao commented Apr 7, 2023

openjdk bot commented Apr 7, 2023

openjdk bot commented Apr 7, 2023

tsayao commented Oct 13, 2022 •

edited by openjdk bot

mlbridge bot commented Oct 16, 2022 •

edited