jfx icon indicating copy to clipboard operation
jfx copied to clipboard

Published 20 hours ago • verified publisher icon openjdk

8260528: Clean glass-gtk sizing and positioning code

Open tsayao opened this issue 4 years ago • 12 comments

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

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

Issue

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

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 367

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

Using diff file

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

tsayao avatar Dec 15 '20 12:12 tsayao

:wave: 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.

bridgekeeper[bot] avatar Dec 15 '20 12:12 bridgekeeper[bot]

Webrevs

mlbridge[bot] avatar Dec 20 '20 00:12 mlbridge[bot]

/reviewers 2

kevinrushforth avatar Dec 21 '20 12:12 kevinrushforth

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

openjdk[bot] avatar Dec 21 '20 12:12 openjdk[bot]

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

openjdk[bot] avatar Jan 25 '21 01:01 openjdk[bot]

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.

kevinrushforth avatar Jan 26 '21 00:01 kevinrushforth

Changed to WIP to reduce sizing events.

tsayao avatar Feb 14 '21 22:02 tsayao

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 avatar Mar 03 '21 12:03 tsayao

Looks good now.

tsayao avatar Mar 06 '21 23:03 tsayao

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.

tsayao avatar Mar 21 '21 00:03 tsayao

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

pankaj-bansal avatar Mar 24 '21 12:03 pankaj-bansal

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

pankaj-bansal avatar Apr 28 '21 03:04 pankaj-bansal

@tsayao The work in this PR looks really good. If you want to continue with this, I will review it. There are some conflicts but those can easily be solved.

johanvos avatar Sep 30 '22 06:09 johanvos

/reviewers 2

kevinrushforth avatar Oct 08 '22 14:10 kevinrushforth

@kevinrushforth The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

openjdk[bot] avatar Oct 08 '22 15:10 openjdk[bot]

@tsayao this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout glass_gtk_new_position_and_size
git fetch https://git.openjdk.org/jfx master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk[bot] avatar Oct 08 '22 15:10 openjdk[bot]

@johanvos I think I can rework it better now.

tsayao avatar Oct 10 '22 23:10 tsayao

@johanvos I think I can rework it better now.

Do you mean you want to close this PR and create a new one? That's fine -- let me know if you need some help or a first review even before it's ready.

johanvos avatar Oct 11 '22 09:10 johanvos

@johanvos Yes, I have started working on it at https://github.com/tsayao/jfx/tree/clean_glass_gtk

tsayao avatar Oct 11 '22 11:10 tsayao

Closed in favor of #915

tsayao avatar Oct 13 '22 17:10 tsayao