Skip to content

Commit

Permalink
8295531: ComboBoxBaseSkin: memory leak when changing skin
Browse files Browse the repository at this point in the history 
Reviewed-by: aghaisas
  • Loading branch information
Andy Goryachev committed Dec 6, 2022
1 parent f333662 commit bb13920
Show file tree
Hide file tree
Showing 6 changed files with 144 additions and 103 deletions.
52 changes: 24 additions & 28 deletions modules/javafx.controls/src/main/java/javafx/scene/control/skin/ColorPickerSkin.java
View file
@@ -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,46 +25,42 @@

package javafx.scene.control.skin;

import com.sun.javafx.css.StyleManager;

import com.sun.javafx.scene.control.Properties;
import com.sun.javafx.scene.control.behavior.ComboBoxBaseBehavior;
import com.sun.javafx.scene.control.skin.Utils;
import javafx.beans.property.StringProperty;
import javafx.css.StyleOrigin;
import javafx.css.StyleableBooleanProperty;
import javafx.css.CssMetaData;

import javafx.css.converter.BooleanConverter;
import static javafx.scene.paint.Color.*;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;

import javafx.beans.property.BooleanProperty;
import javafx.beans.property.StringProperty;
import javafx.css.CssMetaData;
import javafx.css.StyleOrigin;
import javafx.css.Styleable;
import javafx.css.StyleableBooleanProperty;
import javafx.css.StyleableDoubleProperty;
import javafx.css.StyleableProperty;
import javafx.css.StyleableStringProperty;
import javafx.css.converter.BooleanConverter;
import javafx.css.converter.SizeConverter;
import javafx.css.converter.StringConverter;
import javafx.geometry.Pos;
import javafx.scene.Node;
import javafx.scene.control.ColorPicker;
import javafx.scene.control.Control;
import javafx.scene.control.Label;
import javafx.scene.control.TextField;
import javafx.scene.image.ImageView;
import javafx.scene.layout.StackPane;
import javafx.scene.paint.Color;
import javafx.scene.shape.Rectangle;

import javafx.css.converter.SizeConverter;
import javafx.css.converter.StringConverter;
import com.sun.javafx.css.StyleManager;
import com.sun.javafx.scene.control.ListenerHelper;
import com.sun.javafx.scene.control.Properties;
import com.sun.javafx.scene.control.behavior.ColorPickerBehavior;

import java.util.Map;

import javafx.scene.control.ColorPicker;
import javafx.scene.control.TextField;
import javafx.beans.property.BooleanProperty;
import javafx.css.Styleable;
import javafx.css.StyleableProperty;
import javafx.scene.control.Label;
import javafx.scene.paint.Color;

import static javafx.scene.paint.Color.*;
import com.sun.javafx.scene.control.behavior.ComboBoxBaseBehavior;
import com.sun.javafx.scene.control.skin.Utils;

/**
* Default skin implementation for the {@link ColorPicker} control.
Expand Down Expand Up @@ -107,10 +103,10 @@ public ColorPickerSkin(final ColorPicker control) {

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

updateComboBoxMode();
registerChangeListener(control.valueProperty(), e -> updateColor());

ListenerHelper.get(this).addChangeListener(control.valueProperty(), (ev) -> updateColor());

// create displayNode
displayNode = new Label();
Expand Down
23 changes: 15 additions & 8 deletions modules/javafx.controls/src/main/java/javafx/scene/control/skin/ComboBoxBaseSkin.java
View file
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 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,18 +25,20 @@

package javafx.scene.control.skin;

import java.util.List;

import javafx.event.EventHandler;
import javafx.scene.control.SkinBase;
import com.sun.javafx.scene.control.behavior.ComboBoxBaseBehavior;
import javafx.geometry.HPos;
import javafx.geometry.VPos;
import javafx.scene.Node;
import javafx.scene.control.ComboBoxBase;
import javafx.scene.control.SkinBase;
import javafx.scene.input.MouseEvent;
import javafx.scene.layout.Region;
import javafx.scene.layout.StackPane;

import java.util.List;
import com.sun.javafx.scene.control.ListenerHelper;
import com.sun.javafx.scene.control.behavior.ComboBoxBaseBehavior;

/**
* An abstract class intended to be used as the base skin for ComboBox-like
Expand Down Expand Up @@ -110,27 +112,32 @@ public ComboBoxBaseSkin(final ComboBoxBase<T> control) {

getChildren().add(arrowButton);

ListenerHelper lh = ListenerHelper.get(this);

// When ComboBoxBase focus shifts to another node, it should hide.
getSkinnable().focusedProperty().addListener((observable, oldValue, newValue) -> {
lh.addChangeListener(getSkinnable().focusedProperty(), (observable, oldValue, newValue) -> {
if (!newValue) {
focusLost();
}
});

// Register listeners
updateArrowButtonListeners();
registerChangeListener(control.editableProperty(), e -> {

lh.addChangeListener(control.editableProperty(), e -> {
updateArrowButtonListeners();
updateDisplayArea();
});
registerChangeListener(control.showingProperty(), e -> {

lh.addChangeListener(control.showingProperty(), e -> {
if (getSkinnable().isShowing()) {
show();
} else {
hide();
}
});
registerChangeListener(control.valueProperty(), e -> updateDisplayArea());

lh.addChangeListener(control.valueProperty(), e -> updateDisplayArea());
}


Expand Down
64 changes: 35 additions & 29 deletions modules/javafx.controls/src/main/java/javafx/scene/control/skin/ComboBoxListViewSkin.java
View file
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 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,14 +25,9 @@

package javafx.scene.control.skin;

import com.sun.javafx.scene.control.behavior.ComboBoxBaseBehavior;
import com.sun.javafx.scene.control.behavior.ComboBoxListViewBehavior;

import java.util.List;
import java.util.function.Supplier;

import javafx.beans.InvalidationListener;
import javafx.beans.WeakInvalidationListener;
import javafx.beans.property.BooleanProperty;
import javafx.beans.property.SimpleBooleanProperty;
import javafx.beans.value.ObservableValue;
Expand All @@ -55,10 +50,16 @@
import javafx.scene.control.SelectionModel;
import javafx.scene.control.SingleSelectionModel;
import javafx.scene.control.TextField;
import javafx.scene.input.*;
import javafx.scene.input.KeyCode;
import javafx.scene.input.MouseEvent;
import javafx.util.Callback;
import javafx.util.StringConverter;

import com.sun.javafx.scene.control.IDisconnectable;
import com.sun.javafx.scene.control.ListenerHelper;
import com.sun.javafx.scene.control.behavior.ComboBoxBaseBehavior;
import com.sun.javafx.scene.control.behavior.ComboBoxListViewBehavior;

/**
* Default skin implementation for the {@link ComboBox} control.
*
Expand Down Expand Up @@ -100,7 +101,7 @@ public class ComboBoxListViewSkin<T> extends ComboBoxPopupControl<T> {
private boolean listViewSelectionDirty = false;

private final ComboBoxListViewBehavior behavior;

private IDisconnectable selectedItemWatcher;


/* *************************************************************************
Expand All @@ -117,8 +118,6 @@ public class ComboBoxListViewSkin<T> extends ComboBoxPopupControl<T> {
}
};

private final InvalidationListener itemsObserver;

private final WeakListChangeListener<T> weakListViewItemsListener =
new WeakListChangeListener<>(listViewItemsListener);

Expand All @@ -141,16 +140,16 @@ public ComboBoxListViewSkin(final ComboBox<T> control) {

// install default input map for the control
this.behavior = new ComboBoxListViewBehavior<>(control);
// control.setInputMap(behavior.getInputMap());

this.comboBox = control;
updateComboBoxItems();

itemsObserver = observable -> {
ListenerHelper lh = ListenerHelper.get(this);

lh.addInvalidationListener(control.itemsProperty(), (x) -> {
updateComboBoxItems();
updateListViewItems();
};
control.itemsProperty().addListener(new WeakInvalidationListener(itemsObserver));
});

// listview for popup
this.listView = createListView();
Expand All @@ -168,32 +167,33 @@ public ComboBoxListViewSkin(final ComboBox<T> control) {
// Fix for RT-19431 (also tested via ComboBoxListViewSkinTest)
updateValue();

registerChangeListener(control.itemsProperty(), e -> {
lh.addChangeListener(control.itemsProperty(), e -> {
updateComboBoxItems();
updateListViewItems();
});
registerChangeListener(control.promptTextProperty(), e -> updateDisplayNode());
registerChangeListener(control.cellFactoryProperty(), e -> updateCellFactory());
registerChangeListener(control.visibleRowCountProperty(), e -> {
lh.addChangeListener(control.promptTextProperty(), e -> updateDisplayNode());
lh.addChangeListener(control.cellFactoryProperty(), e -> updateCellFactory());
lh.addChangeListener(control.visibleRowCountProperty(), e -> {
if (listView == null) return;
listView.requestLayout();
});
registerChangeListener(control.converterProperty(), e -> updateListViewItems());
registerChangeListener(control.buttonCellProperty(), e -> {
lh.addChangeListener(control.converterProperty(), e -> updateListViewItems());
lh.addChangeListener(control.buttonCellProperty(), e -> {
updateButtonCell();
updateDisplayArea();
});
registerChangeListener(control.valueProperty(), e -> {
lh.addChangeListener(control.valueProperty(), e -> {
updateValue();
control.fireEvent(new ActionEvent());
});
registerChangeListener(control.editableProperty(), e -> updateEditable());
lh.addChangeListener(control.editableProperty(), e -> updateEditable());

// Refer to JDK-8095306
if (comboBox.isShowing()) {
show();
}
comboBox.sceneProperty().addListener(o -> {

lh.addInvalidationListener(comboBox.sceneProperty(), (o) -> {
if (((ObservableValue)o).getValue() == null) {
comboBox.hide();
}
Expand Down Expand Up @@ -573,12 +573,18 @@ private ListView<T> createListView() {
comboBox.notifyAccessibleAttributeChanged(AccessibleAttribute.TEXT);
});

SingleSelectionModel<T> selectionModel = comboBox.getSelectionModel();
if (selectionModel != null) {
selectionModel.selectedItemProperty().addListener(o -> {
listViewSelectionDirty = true;
});
}
ListenerHelper lh = ListenerHelper.get(this);
lh.addChangeListener(comboBox.selectionModelProperty(), true, (src, oldsm, newsm) -> {
if (selectedItemWatcher != null) {
selectedItemWatcher.disconnect();
}

if (newsm != null) {
selectedItemWatcher = lh.addInvalidationListener(newsm.selectedItemProperty(), (x) -> {
listViewSelectionDirty = true;
});
}
});

_listView.addEventFilter(MouseEvent.MOUSE_RELEASED, t -> {
// RT-18672: Without checking if the user is clicking in the
Expand Down

1 comment on commit bb13920

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Issues

Please sign in to comment.