Skip to content

Commit

Permalink
8295754: PaginationSkin: memory leak when changing skin
Browse files Browse the repository at this point in the history 
Reviewed-by: kcr, aghaisas
  • Loading branch information
Andy Goryachev committed Nov 30, 2022
1 parent 2fa9f4b commit 0a785ae
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 72 deletions.
154 changes: 83 additions & 71 deletions modules/javafx.controls/src/main/java/javafx/scene/control/skin/PaginationSkin.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2012, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2012, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand All @@ -25,31 +25,31 @@

package javafx.scene.control.skin;

import com.sun.javafx.scene.control.skin.Utils;
import javafx.beans.property.DoubleProperty;
import javafx.css.StyleableBooleanProperty;
import javafx.css.StyleableDoubleProperty;
import javafx.css.StyleableObjectProperty;
import javafx.css.CssMetaData;

import javafx.css.converter.BooleanConverter;
import javafx.css.converter.EnumConverter;
import javafx.css.converter.SizeConverter;
import com.sun.javafx.scene.control.behavior.PaginationBehavior;

import static com.sun.javafx.scene.control.skin.resources.ControlResources.getString;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

import javafx.animation.*;
import com.sun.javafx.scene.control.ListenerHelper;
import com.sun.javafx.scene.control.behavior.PaginationBehavior;
import com.sun.javafx.scene.control.skin.Utils;
import javafx.animation.Interpolator;
import javafx.animation.KeyFrame;
import javafx.animation.KeyValue;
import javafx.animation.Timeline;
import javafx.application.Platform;
import javafx.beans.property.BooleanProperty;
import javafx.beans.property.DoubleProperty;
import javafx.beans.property.ObjectProperty;
import javafx.beans.value.ChangeListener;
import javafx.beans.value.WritableValue;
import javafx.collections.ListChangeListener;
import javafx.css.CssMetaData;
import javafx.css.Styleable;
import javafx.css.StyleableBooleanProperty;
import javafx.css.StyleableDoubleProperty;
import javafx.css.StyleableObjectProperty;
import javafx.css.StyleableProperty;
import javafx.css.converter.BooleanConverter;
import javafx.css.converter.EnumConverter;
import javafx.css.converter.SizeConverter;
import javafx.event.ActionEvent;
import javafx.event.EventHandler;
import javafx.geometry.HPos;
Expand All @@ -61,16 +61,22 @@
import javafx.scene.AccessibleAttribute;
import javafx.scene.AccessibleRole;
import javafx.scene.Node;
import javafx.scene.control.*;
import javafx.scene.control.Button;
import javafx.scene.control.Control;
import javafx.scene.control.Label;
import javafx.scene.control.Pagination;
import javafx.scene.control.SkinBase;
import javafx.scene.control.Toggle;
import javafx.scene.control.ToggleButton;
import javafx.scene.control.ToggleGroup;
import javafx.scene.control.Tooltip;
import javafx.scene.input.MouseEvent;
import javafx.scene.input.TouchEvent;
import javafx.scene.layout.HBox;
import javafx.scene.layout.StackPane;
import javafx.scene.shape.Rectangle;
import javafx.util.Duration;

import static com.sun.javafx.scene.control.skin.resources.ControlResources.getString;

/**
* Default skin implementation for the {@link Pagination} control.
*
Expand Down Expand Up @@ -98,7 +104,6 @@ public class PaginationSkin extends SkinBase<Pagination> {
* *
**************************************************************************/

private Pagination pagination;
private StackPane currentStackPane;
private StackPane nextStackPane;
private Timeline timeline;
Expand Down Expand Up @@ -179,13 +184,8 @@ public PaginationSkin(final Pagination control) {

// install default input map for the Pagination control
behavior = new PaginationBehavior(control);
// control.setInputMap(behavior.getInputMap());

// setManaged(false);
clipRect = new Rectangle();
getSkinnable().setClip(clipRect);

this.pagination = control;

this.currentStackPane = new StackPane();
currentStackPane.getStyleClass().add("page");
Expand All @@ -194,20 +194,29 @@ public PaginationSkin(final Pagination control) {
nextStackPane.getStyleClass().add("page");
nextStackPane.setVisible(false);

resetIndexes(true);

this.navigation = new NavigationControl();

getChildren().addAll(currentStackPane, nextStackPane, navigation);

control.maxPageIndicatorCountProperty().addListener(o -> {
ListenerHelper lh = ListenerHelper.get(this);

lh.addInvalidationListener(control.maxPageIndicatorCountProperty(), (o) -> {
resetIndiciesAndNav();
});

registerChangeListener(control.widthProperty(), e -> clipRect.setWidth(getSkinnable().getWidth()));
registerChangeListener(control.heightProperty(), e -> clipRect.setHeight(getSkinnable().getHeight()));
registerChangeListener(control.pageCountProperty(), e -> resetIndiciesAndNav());
registerChangeListener(control.pageFactoryProperty(), e -> {
lh.addChangeListener(control.widthProperty(), true, (ev) -> {
clipRect.setWidth(control.getWidth());
});

lh.addChangeListener(control.heightProperty(), true, (ev) -> {
clipRect.setHeight(control.getHeight());
});

lh.addChangeListener(control.pageCountProperty(), (ev) -> {
resetIndiciesAndNav();
});

lh.addChangeListener(control.pageFactoryProperty(), (ev) -> {
if (animate && timeline != null) {
// If we are in the middle of a page animation.
// Speedup and finish the animation then update the page factory.
Expand All @@ -223,6 +232,12 @@ public PaginationSkin(final Pagination control) {
initializeSwipeAndTouchHandlers();
}

@Override
public void install() {
getSkinnable().setClip(clipRect);
resetIndexes(true);
}



/* *************************************************************************
Expand Down Expand Up @@ -382,12 +397,20 @@ public String getName() {
**************************************************************************/

/** {@inheritDoc} */
@Override public void dispose() {
super.dispose();
@Override
public void dispose() {
if (getSkinnable() == null) {
return;
}

getSkinnable().setClip(null);
getChildren().removeAll(currentStackPane, nextStackPane, navigation);

if (behavior != null) {
behavior.dispose();
}

super.dispose();
}

/** {@inheritDoc} */
Expand Down Expand Up @@ -449,13 +472,13 @@ public String getName() {

private void selectNext() {
if (getCurrentPageIndex() < getPageCount() - 1) {
pagination.setCurrentPageIndex(getCurrentPageIndex() + 1);
getSkinnable().setCurrentPageIndex(getCurrentPageIndex() + 1);
}
}

private void selectPrevious() {
if (getCurrentPageIndex() > 0) {
pagination.setCurrentPageIndex(getCurrentPageIndex() - 1);
getSkinnable().setCurrentPageIndex(getCurrentPageIndex() - 1);
}
}

Expand All @@ -467,8 +490,9 @@ private void resetIndiciesAndNav() {

private void initializeSwipeAndTouchHandlers() {
final Pagination control = getSkinnable();
ListenerHelper lh = ListenerHelper.get(this);

getSkinnable().addEventHandler(TouchEvent.TOUCH_PRESSED, e -> {
lh.addEventHandler(control, TouchEvent.TOUCH_PRESSED, e -> {
if (touchEventId == -1) {
touchEventId = e.getTouchPoint().getId();
}
Expand All @@ -481,7 +505,7 @@ private void initializeSwipeAndTouchHandlers() {
e.consume();
});

getSkinnable().addEventHandler(TouchEvent.TOUCH_MOVED, e -> {
lh.addEventHandler(control, TouchEvent.TOUCH_MOVED, e -> {
if (touchEventId != e.getTouchPoint().getId()) {
return;
}
Expand Down Expand Up @@ -559,7 +583,7 @@ private void initializeSwipeAndTouchHandlers() {
e.consume();
});

getSkinnable().addEventHandler(TouchEvent.TOUCH_RELEASED, e -> {
lh.addEventHandler(control, TouchEvent.TOUCH_RELEASED, e -> {
if (touchEventId != e.getTouchPoint().getId()) {
return;
} else {
Expand Down Expand Up @@ -625,7 +649,7 @@ private void resetIndexes(boolean usePageIndex) {
currentStackPane.getChildren().clear();
nextStackPane.getChildren().clear();

pagination.setCurrentPageIndex(currentIndex);
getSkinnable().setCurrentPageIndex(currentIndex);
createPage(currentStackPane, currentIndex);

if (isAnimate) {
Expand All @@ -634,8 +658,8 @@ private void resetIndexes(boolean usePageIndex) {
}

private boolean createPage(StackPane pane, int index) {
if (pagination.getPageFactory() != null && pane.getChildren().isEmpty()) {
Node content = pagination.getPageFactory().call(index);
if (getSkinnable().getPageFactory() != null && pane.getChildren().isEmpty()) {
Node content = getSkinnable().getPageFactory().call(index);
// If the content is null we don't want to switch pages.
if (content != null) {
pane.getChildren().setAll(content);
Expand All @@ -648,12 +672,12 @@ private boolean createPage(StackPane pane, int index) {
animate = false;
}

if (pagination.getPageFactory().call(previousIndex) != null) {
pagination.setCurrentPageIndex(previousIndex);
if (getSkinnable().getPageFactory().call(previousIndex) != null) {
getSkinnable().setCurrentPageIndex(previousIndex);
} else {
// Set the page index to 0 because both the current,
// and the previous pages have no content.
pagination.setCurrentPageIndex(0);
getSkinnable().setCurrentPageIndex(0);
}

if (isAnimate) {
Expand Down Expand Up @@ -893,9 +917,9 @@ private void initializeNavigationHandlers() {
requestLayout();
});

pagination.currentPageIndexProperty().addListener((arg0, arg1, arg2) -> {
previousIndex = arg1.intValue();
currentIndex = arg2.intValue();
ListenerHelper.get(PaginationSkin.this).addChangeListener(getSkinnable().currentPageIndexProperty(), (src, old, cur) -> {
previousIndex = old.intValue();
currentIndex = cur.intValue();
updatePageIndex();
if (animate) {
currentAnimatedIndex = currentIndex;
Expand Down Expand Up @@ -923,12 +947,6 @@ private void initializePageIndicators() {
}

private void clearIndicatorButtons() {
for (Toggle toggle : indicatorButtons.getToggles()) {
if (toggle instanceof IndicatorButton) {
IndicatorButton indicatorButton = (IndicatorButton) toggle;
indicatorButton.release();
}
}
indicatorButtons.getToggles().clear();
}

Expand Down Expand Up @@ -1268,33 +1286,32 @@ private Pos sideToPos(Side s) {
}

class IndicatorButton extends ToggleButton {
private final ListChangeListener<String> updateSkinIndicatorType =
c -> setIndicatorType();

private final ChangeListener<Boolean> updateTooltipVisibility =
(ob, oldValue, newValue) -> setTooltipVisible(newValue);

private int pageNumber;

public IndicatorButton(int pageNumber) {
this.pageNumber = pageNumber;
setFocusTraversable(false);
setIndicatorType();
setTooltipVisible(isTooltipVisible());

getSkinnable().getStyleClass().addListener(updateSkinIndicatorType);
ListenerHelper lh = ListenerHelper.get(PaginationSkin.this);

lh.addListChangeListener(getSkinnable().getStyleClass(), (ch) -> {
setIndicatorType();
});
setIndicatorType();

setOnAction(arg0 -> {
getNode().requestFocus();
int selected = getCurrentPageIndex();
// We do not need to update the selection if it has not changed.
if (selected != IndicatorButton.this.pageNumber) {
pagination.setCurrentPageIndex(IndicatorButton.this.pageNumber);
getSkinnable().setCurrentPageIndex(IndicatorButton.this.pageNumber);
requestLayout();
}
});

tooltipVisibleProperty().addListener(updateTooltipVisibility);
lh.addChangeListener(tooltipVisibleProperty(), true, (visible) -> {
setTooltipVisible(visible);
});

prefHeightProperty().bind(minHeightProperty());
setAccessibleRole(AccessibleRole.PAGE_ITEM);
Expand Down Expand Up @@ -1337,11 +1354,6 @@ public int getPageNumber() {
}
}

public void release() {
getSkinnable().getStyleClass().removeListener(updateSkinIndicatorType);
tooltipVisibleProperty().removeListener(updateTooltipVisibility);
}

/** {@inheritDoc} */
@Override
public Object queryAccessibleAttribute(AccessibleAttribute attribute, Object... parameters) {
Expand Down
2 changes: 1 addition & 1 deletion modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ public static Collection<Object[]> data() {
DatePicker.class,
MenuBar.class,
MenuButton.class,
Pagination.class,
//Pagination.class,
PasswordField.class,
//ScrollBar.class,
//ScrollPane.class,
Expand Down

0 comments on commit 0a785ae

Please sign in to comment.