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

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

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

@tsayao
Copy link
Collaborator

@tsayao tsayao commented Dec 15, 2020

This is a new approach to rewrite parts of gtk glass backend to be more clean.

I will provide small "manageable" PR to incrementally make the backend better.

This PR adresses cleanup of the Size and Positioning code. It makes code more "straightforward" and easier to maintain.

Current status (Ubuntu 20.04):
image

(*) Some of the iconify tests are also failing on the main branch.

gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests test.robot.javafx.stage.IconifyTest on a second run produces 4 tests, 2 failures.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

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

Download

$ git fetch https://git.openjdk.java.net/jfx pull/367/head:pull/367
$ git checkout pull/367

tsayao added 11 commits Jan 23, 2020
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
Merge master
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 15, 2020

👋 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.

@tsayao tsayao changed the title WIP: 8236651: Simplify and update glass gtk backend [WIP] 8236651: Simplify and update glass gtk backend Dec 16, 2020
@tsayao tsayao changed the title [WIP] 8236651: Simplify and update glass gtk backend 8236651: Simplify and update glass gtk backend Dec 20, 2020
@openjdk openjdk bot added the rfr label Dec 20, 2020
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Dec 21, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Dec 21, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

Update from jfx
@tsayao tsayao mentioned this pull request Jan 25, 2021
2 of 3 tasks complete
@openjdk
Copy link

@openjdk openjdk bot commented Jan 25, 2021

⚠️ @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).

@tsayao tsayao changed the title 8236651: Simplify and update glass gtk backend 8236651: Merge jdk:master Simplify and update glass gtk backend Jan 25, 2021
@tsayao tsayao changed the title 8236651: Merge jdk:master Simplify and update glass gtk backend 8236651: Merge jfx:master Simplify and update glass gtk backend Jan 25, 2021
@tsayao tsayao changed the title 8236651: Merge jfx:master Simplify and update glass gtk backend 8236651: Simplify and update glass gtk backend Jan 25, 2021
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jan 26, 2021

This does look like a much more manageable approach.

One thing to be aware of from a bookkeeping point of view is that a JBS issues is resolved by a single PR. Once a PR has been integrated for a given JBS bug ID, that bug ID cannot be reused.

This means that each separate PR will need it's own JBS issue to be filed, even if those enhancements taken together are part of a larger set of improvements. Incremental improvements are fine (and in this case a good way to proceed), but you might give some thought to the title of each such improvement. I wouldn't want to see 5 fixes go in each with the same title of Simplify and update glass gtk backend.

@tsayao tsayao changed the title 8236651: Simplify and update glass gtk backend 8260528: Clean glass-gtk sizing and positioning code Jan 27, 2021
Update
@tsayao tsayao changed the title 8260528: Clean glass-gtk sizing and positioning code WIP: 8260528: Clean glass-gtk sizing and positioning code Feb 14, 2021
@tsayao
Copy link
Collaborator Author

@tsayao tsayao commented Feb 14, 2021

Changed to WIP to reduce sizing events.

@openjdk openjdk bot removed the rfr label Feb 14, 2021
@tsayao tsayao force-pushed the tsayao:glass_gtk_new_position_and_size branch from 07219e7 to 6766502 Feb 18, 2021
@tsayao tsayao changed the title WIP: 8260528: Clean glass-gtk sizing and positioning code 8260528: Clean glass-gtk sizing and positioning code Feb 18, 2021
@openjdk openjdk bot added the rfr label Feb 18, 2021
@tsayao tsayao changed the title 8260528: Clean glass-gtk sizing and positioning code WIP: 8260528: Clean glass-gtk sizing and positioning code Mar 3, 2021
@openjdk openjdk bot removed the rfr label Mar 3, 2021
@tsayao
Copy link
Collaborator Author

@tsayao tsayao commented Mar 3, 2021

Changed to WIP while we run some more tests to make sure latest changes are ok.
I'm fixing some residual "extra resize" issues (these issues exists on the current versions, so this makes it better, hopefully).

tsayao added 2 commits Mar 6, 2021
Pull from origin
Default to 320x200 if no size is assigned

Hopefully fix all "extra resize" problems due to frame extents.

Small change to reuse get_net_frame_extents_atom()

Fix more tests (restore 1 behaviour)

More test fixes

Partial progress

Adjust positioning (not 100% yet)

It's looking good. Except for fixed initial extents, but it seems a reasonable fix due to nature o frame extents.

It's probably good now

One more small issue
@tsayao tsayao force-pushed the tsayao:glass_gtk_new_position_and_size branch from c601603 to 34518b8 Mar 6, 2021
@tsayao tsayao changed the title WIP: 8260528: Clean glass-gtk sizing and positioning code 8260528: Clean glass-gtk sizing and positioning code Mar 6, 2021
@tsayao
Copy link
Collaborator Author

@tsayao tsayao commented Mar 6, 2021

Looks good now.

@openjdk openjdk bot added the rfr label Mar 6, 2021
tsayao and others added 2 commits Mar 7, 2021
@tsayao
Copy link
Collaborator Author

@tsayao tsayao commented Mar 21, 2021

Testing:

master
master

tsayao:glass_gtk_new_position_and_size
branch

--tests test.robot.javafx.scene.RobotTest causes some intermittent error but eventually all passes.

@pankaj-bansal
Copy link
Collaborator

@pankaj-bansal pankaj-bansal commented Mar 24, 2021

I ran the full test on Ubuntu 20.10 and the results look fine. I do not see any new failure.

@pankaj-bansal
Copy link
Collaborator

@pankaj-bansal pankaj-bansal commented Apr 28, 2021

I ran the full test on OL 8.2 and the results look fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 participants