[jfx20] RFR: 8301797: Pagination control has the wrong size

[Kevin Rushforth]

On Fri, 3 Feb 2023 20:15:46 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

> 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

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.

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

> 195:         nextStackPane.setVisible(false);
> 196: 
> 197:         resetIndexes(true);

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

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

> 652:         nextStackPane.getChildren().clear();
> 653: 
> 654:         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`.

-------------

Changes requested by kcr (Lead).

PR: https://git.openjdk.org/jfx/pull/1021