8301797: Pagination control has the wrong size #1021

andy-goryachev-oracle · 2023-02-03T20:15:46Z

Fixed regression introduced by JDK-8295754, targeting upstream jfx20 branch.

The method PaginationSkin.resetIndexes(true) has been moved to the original position in the constructor, fixing the initialization, while making sure no properties of the control are touched in the constructor (only in install()).

Added a test case.

Tested with the PaginationDisappear.java and the LeakTest app
https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTestApp.java

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-8301797: Pagination control has the wrong size

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Ajit Ghaisas (@aghaisas - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1021

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

Using diff file

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

bridgekeeper · 2023-02-03T20:16:53Z

👋 Welcome back angorya! A progress list of the required criteria for merging this PR into jfx20 will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2023-02-03T20:37:55Z

Webrevs

01: Full - Incremental (a01348e6)
00: Full (3fbbfccb)

kevinrushforth · 2023-02-03T21:48:30Z

A fix request in JBS is in progress (since we're in RDP2), which I'll review along with the code.

@aghaisas Can I ask you to also review it?

/reviewers 2

openjdk · 2023-02-03T21:49:03Z

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

kevinrushforth

While the fix works for the specific case in the bug report, I don't think it's quite right.

In your description you say:

while making sure no properties of the control are touched in the constructor (only in install()).

However, the current fix does touch properties of the control in the constructor via the moved resetIndexes method. First, the "if" check you added to avoid calling setCurrentPageIndex on the control is backwards, meaning that it is being called in the case you were trying to avoid. Second, createPage, which is also called from resetIndexes has a code path that also calls setCurrentPageIndex.

If it really is important that the skin constructor not call any setter on the control, then you will need to rework the solution. Possibly by moving the call to resetIndexes back to install, and figuring out why calling it later isn't causing the pref size to be recomputed.

kevinrushforth · 2023-02-03T22:17:08Z

modules/javafx.controls/src/main/java/javafx/scene/control/skin/PaginationSkin.java

@@ -649,7 +651,11 @@ private void resetIndexes(boolean usePageIndex) {
        currentStackPane.getChildren().clear();
        nextStackPane.getChildren().clear();

-        getSkinnable().setCurrentPageIndex(currentIndex);
+        if (usePageIndex) {


Shouldn't this be !usePageIndex, given the stated intent? The resetIndexes method is called twice: once from the constructor with usePageIndex = true and once from resetIndiciesAndNav with usePageIndex = false.

kevinrushforth · 2023-02-03T22:28:06Z

modules/javafx.controls/src/main/java/javafx/scene/control/skin/PaginationSkin.java

@@ -194,6 +194,8 @@ public PaginationSkin(final Pagination control) {
        nextStackPane.getStyleClass().add("page");
        nextStackPane.setVisible(false);

+        resetIndexes(true);


This will end up calling into the control in a couple places, which the PR description says you are trying to avoid.

true, but since it sets to the same value it will be a no-op

andy-goryachev-oracle · 2023-02-03T22:56:39Z

You are right!

In fact, the added logic to bypass setting control properties in the constructor is unnecessary - it will be essentially a no-op, since it uses the control's current page index.

Thank you!

kevinrushforth · 2023-02-03T23:05:43Z

The changes you just made look good to me, since everything is now internally consistent. I'll retest and then finish my review.

andy-goryachev-oracle · 2023-02-03T23:07:53Z

Thank you, @kevinrushforth .

kevinrushforth

Looks good.

kevinrushforth · 2023-02-03T23:20:38Z

modules/javafx.controls/src/main/java/javafx/scene/control/skin/PaginationSkin.java

@@ -194,6 +194,8 @@ public PaginationSkin(final Pagination control) {
        nextStackPane.getStyleClass().add("page");
        nextStackPane.setVisible(false);

+        resetIndexes(true);


aghaisas

The simplified fix looks good to me!
Thanks for fixing this regression.

openjdk · 2023-02-06T08:35:46Z

@andy-goryachev-oracle 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:

8301797: Pagination control has the wrong size

Reviewed-by: kcr, aghaisas

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 no new commits pushed to the jfx20 branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

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

andy-goryachev-oracle · 2023-02-06T16:13:05Z

/integrate

openjdk · 2023-02-06T16:15:18Z

Going to push as commit 192ba4f.

openjdk · 2023-02-06T16:15:34Z

@andy-goryachev-oracle Pushed as commit 192ba4f.

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

8301797: fixed pagination initialization

3fbbfcc

andy-goryachev-oracle marked this pull request as ready for review February 3, 2023 20:32

andy-goryachev-oracle changed the title ~~8301797: fixed pagination initialization~~ 8301797: Pagination control has the wrong size Feb 3, 2023

openjdk bot added the rfr Ready for review label Feb 3, 2023

kevinrushforth requested review from kevinrushforth and aghaisas February 3, 2023 21:48

kevinrushforth requested changes Feb 3, 2023

View reviewed changes

8301797: review comments

a01348e

kevinrushforth approved these changes Feb 3, 2023

View reviewed changes

aghaisas approved these changes Feb 6, 2023

View reviewed changes

openjdk bot added the ready Ready to be integrated label Feb 6, 2023

openjdk bot added the integrated Pull request has been integrated label Feb 6, 2023

openjdk bot closed this Feb 6, 2023

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Feb 6, 2023

andy-goryachev-oracle deleted the 8301797.pagination branch February 6, 2023 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8301797: Pagination control has the wrong size #1021

8301797: Pagination control has the wrong size #1021

andy-goryachev-oracle commented Feb 3, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 3, 2023

mlbridge bot commented Feb 3, 2023 •

edited

Loading

kevinrushforth commented Feb 3, 2023

openjdk bot commented Feb 3, 2023

kevinrushforth left a comment

kevinrushforth Feb 3, 2023

kevinrushforth Feb 3, 2023

andy-goryachev-oracle Feb 3, 2023

kevinrushforth Feb 3, 2023

andy-goryachev-oracle commented Feb 3, 2023 •

edited

Loading

kevinrushforth commented Feb 3, 2023

andy-goryachev-oracle commented Feb 3, 2023

kevinrushforth left a comment

kevinrushforth Feb 3, 2023

aghaisas left a comment

openjdk bot commented Feb 6, 2023

andy-goryachev-oracle commented Feb 6, 2023

openjdk bot commented Feb 6, 2023

openjdk bot commented Feb 6, 2023

8301797: Pagination control has the wrong size #1021

8301797: Pagination control has the wrong size #1021

Conversation

andy-goryachev-oracle commented Feb 3, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 3, 2023

mlbridge bot commented Feb 3, 2023 • edited Loading

Webrevs

kevinrushforth commented Feb 3, 2023

openjdk bot commented Feb 3, 2023

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth Feb 3, 2023

Choose a reason for hiding this comment

kevinrushforth Feb 3, 2023

Choose a reason for hiding this comment

andy-goryachev-oracle Feb 3, 2023

Choose a reason for hiding this comment

kevinrushforth Feb 3, 2023

Choose a reason for hiding this comment

andy-goryachev-oracle commented Feb 3, 2023 • edited Loading

kevinrushforth commented Feb 3, 2023

andy-goryachev-oracle commented Feb 3, 2023

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth Feb 3, 2023

Choose a reason for hiding this comment

aghaisas left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 6, 2023

andy-goryachev-oracle commented Feb 6, 2023

openjdk bot commented Feb 6, 2023

openjdk bot commented Feb 6, 2023

andy-goryachev-oracle commented Feb 3, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 3, 2023 •

edited

Loading

andy-goryachev-oracle commented Feb 3, 2023 •

edited

Loading