On Fri, 3 Feb 2023 20:15:46 GMT, Andy Goryachev <ango...@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
