Skip to content

Commit

Permalink
8246195: ListViewSkin/Behavior: misbehavior on switching skin
Browse files Browse the repository at this point in the history
Reviewed-by: arapte
  • Loading branch information
Jeanette Winzenburg authored and arapte committed Jun 8, 2020
1 parent 9749982 commit a02e09d
Show file tree
Hide file tree
Showing 7 changed files with 260 additions and 17 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2013, 2016, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2013, 2020, 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 Down Expand Up @@ -213,16 +213,20 @@ public ListViewBehavior(ListView<T> control) {
ListView<T> control = getNode();

ListCellBehavior.removeAnchor(control);
control.selectionModelProperty().removeListener(weakSelectionModelListener);
if (control.getSelectionModel() != null) {
control.getSelectionModel().getSelectedIndices().removeListener(weakSelectedIndicesListener);
}
control.itemsProperty().removeListener(weakItemsListener);
if (control.getItems() != null) {
control.getItems().removeListener(weakItemsListListener);
}

if (tlFocus != null) tlFocus.dispose();
control.removeEventFilter(KeyEvent.ANY, keyEventListener);
super.dispose();

control.removeEventHandler(KeyEvent.ANY, keyEventListener);
}





/**************************************************************************
* State and Functions *
*************************************************************************/
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2018, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 2020, 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 Down Expand Up @@ -37,6 +37,7 @@
import javafx.collections.ObservableList;
import javafx.collections.ObservableMap;
import javafx.collections.WeakListChangeListener;
import javafx.collections.WeakMapChangeListener;
import javafx.event.EventHandler;
import javafx.geometry.Orientation;
import javafx.scene.AccessibleAction;
Expand Down Expand Up @@ -104,7 +105,6 @@ public class ListViewSkin<T> extends VirtualContainerBase<ListView<T>, ListCell<
private Node placeholderNode;

private ObservableList<T> listViewItems;
private final InvalidationListener itemsChangeListener = observable -> updateListViewItems();

private boolean needCellsRebuilt = true;
private boolean needCellsReconfigured = false;
Expand All @@ -129,6 +129,9 @@ public class ListViewSkin<T> extends VirtualContainerBase<ListView<T>, ListCell<
}
};

private WeakMapChangeListener<Object, Object> weakPropertiesMapListener =
new WeakMapChangeListener<>(propertiesMapListener);

private final ListChangeListener<T> listViewItemsListener = new ListChangeListener<T>() {
@Override public void onChanged(Change<? extends T> c) {
while (c.next()) {
Expand Down Expand Up @@ -166,6 +169,12 @@ public class ListViewSkin<T> extends VirtualContainerBase<ListView<T>, ListCell<
new WeakListChangeListener<T>(listViewItemsListener);


private final InvalidationListener itemsChangeListener = observable -> updateListViewItems();

private WeakInvalidationListener
weakItemsChangeListener = new WeakInvalidationListener(itemsChangeListener);

private EventHandler<MouseEvent> ml;

/***************************************************************************
* *
Expand Down Expand Up @@ -208,7 +217,7 @@ public ListViewSkin(final ListView<T> control) {
flow.setFixedCellSize(control.getFixedCellSize());
getChildren().add(flow);

EventHandler<MouseEvent> ml = event -> {
ml = event -> {
// RT-15127: cancel editing on scroll. This is a bit extreme
// (we are cancelling editing on touching the scrollbars).
// This can be improved at a later date.
Expand All @@ -230,11 +239,11 @@ public ListViewSkin(final ListView<T> control) {

updateItemCount();

control.itemsProperty().addListener(new WeakInvalidationListener(itemsChangeListener));
control.itemsProperty().addListener(weakItemsChangeListener);

final ObservableMap<Object, Object> properties = control.getProperties();
properties.remove(Properties.RECREATE);
properties.addListener(propertiesMapListener);
properties.addListener(weakPropertiesMapListener);

// Register listeners
registerChangeListener(control.itemsProperty(), o -> updateListViewItems());
Expand Down Expand Up @@ -263,6 +272,20 @@ public ListViewSkin(final ListView<T> control) {

/** {@inheritDoc} */
@Override public void dispose() {
if (getSkinnable() == null) return;
// listener cleanup fixes side-effects (NPE on refresh, setItems, modifyItems)
getSkinnable().getProperties().removeListener(weakPropertiesMapListener);
getSkinnable().itemsProperty().removeListener(weakItemsChangeListener);
if (listViewItems != null) {
listViewItems.removeListener(weakListViewItemsListener);
listViewItems = null;
}
// flow related cleanup
// leaking without nulling factory
flow.setCellFactory(null);
// for completeness - but no effect with/out?
flow.getVbar().removeEventFilter(MouseEvent.MOUSE_PRESSED, ml);
flow.getHbar().removeEventFilter(MouseEvent.MOUSE_PRESSED, ml);
super.dispose();

if (behavior != null) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2017, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 2020, 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,6 +25,7 @@

package javafx.scene.control.skin;

import javafx.event.EventHandler;
import javafx.scene.control.Control;
import javafx.scene.control.IndexedCell;
import javafx.scene.control.ScrollToEvent;
Expand Down Expand Up @@ -53,6 +54,7 @@ public abstract class VirtualContainerBase<C extends Control, I extends IndexedC
*/
private final VirtualFlow<I> flow;

private EventHandler<? super ScrollToEvent<Integer>> scrollToEventHandler;


/***************************************************************************
Expand All @@ -69,7 +71,7 @@ public VirtualContainerBase(final C control) {
super(control);
flow = createVirtualFlow();

control.addEventHandler(ScrollToEvent.scrollToTopIndex(), event -> {
scrollToEventHandler = event -> {
// Fix for RT-24630: The row count in VirtualFlow was incorrect
// (normally zero), so the scrollTo call was misbehaving.
if (itemCountDirty) {
Expand All @@ -78,7 +80,8 @@ public VirtualContainerBase(final C control) {
itemCountDirty = false;
}
flow.scrollToTop(event.getScrollTarget());
});
};
control.addEventHandler(ScrollToEvent.scrollToTopIndex(), scrollToEventHandler);
}


Expand Down Expand Up @@ -123,6 +126,17 @@ protected VirtualFlow<I> createVirtualFlow() {
return new VirtualFlow<>();
}

/**
* {@inheritDoc} <p>
* Overridden to remove EventHandler.
*/
@Override
public void dispose() {
if (getSkinnable() == null) return;
getSkinnable().removeEventHandler(ScrollToEvent.scrollToTopIndex(), scrollToEventHandler);
super.dispose();
}

/**
* Get the virtualized container.
* Subclasses can invoke this method to get the VirtualFlow instance.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
/*
* Copyright (c) 2020, 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
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation. Oracle designates this
* particular file as subject to the "Classpath" exception as provided
* by Oracle in the LICENSE file that accompanied this code.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

package test.com.sun.javafx.scene.control.behavior;

import java.lang.ref.WeakReference;

import org.junit.After;
import org.junit.Before;
import org.junit.Test;

import com.sun.javafx.scene.control.behavior.BehaviorBase;
import com.sun.javafx.scene.control.behavior.ListCellBehavior;

import static javafx.collections.FXCollections.*;
import static org.junit.Assert.*;
import static test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactory.*;

import javafx.scene.control.ListView;

/**
* Test for misbehavior of individual implementations that turned
* up in binch testing.
*
*/
public class BehaviorCleanupTest {

// ---------- ListView

/**
* Test cleanup of listener to itemsProperty.
*/
@Test
public void testListViewBehaviorDisposeSetItems() {
ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
WeakReference<BehaviorBase<?>> weakRef = new WeakReference<>(createBehavior(listView));
weakRef.get().dispose();
int last = 1;
ListCellBehavior.setAnchor(listView, last, false);
listView.setItems(observableArrayList("other", "again"));
assertEquals("sanity: anchor unchanged", last, listView.getProperties().get("anchor"));
listView.getItems().remove(0);
assertEquals("anchor must not be updated on items modification when disposed",
last, listView.getProperties().get("anchor"));
}

@Test
public void testListViewBehaviorSetItems() {
ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
createBehavior(listView);
int last = 1;
ListCellBehavior.setAnchor(listView, last, false);
listView.setItems(observableArrayList("other", "again"));
assertEquals("sanity: anchor unchanged", last, listView.getProperties().get("anchor"));
listView.getItems().remove(0);
assertEquals("anchor must be updated on items modification",
last -1, listView.getProperties().get("anchor"));
}

/**
* Test cleanup of itemsList listener in ListViewBehavior.
*/
@Test
public void testListViewBehaviorDisposeRemoveItem() {
ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
WeakReference<BehaviorBase<?>> weakRef = new WeakReference<>(createBehavior(listView));
weakRef.get().dispose();
int last = 1;
ListCellBehavior.setAnchor(listView, last, false);
listView.getItems().remove(0);
assertEquals("anchor must not be updated on items modification when disposed",
last,
listView.getProperties().get("anchor"));
}

@Test
public void testListViewBehaviorRemoveItem() {
ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
createBehavior(listView);
int last = 1;
ListCellBehavior.setAnchor(listView, last, false);
assertEquals("behavior must set anchor on select", last, listView.getProperties().get("anchor"));
listView.getItems().remove(0);
assertEquals("anchor must be updated on items modification",
last -1, listView.getProperties().get("anchor"));
}

/**
* Test cleanup of selection listeners in ListViewBehavior.
*/
@Test
public void testListViewBehaviorDisposeSelect() {
ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
WeakReference<BehaviorBase<?>> weakRef = new WeakReference<>(createBehavior(listView));
listView.getSelectionModel().select(1);
weakRef.get().dispose();
listView.getSelectionModel().select(0);
assertNull("anchor must remain cleared on selecting when disposed",
listView.getProperties().get("anchor"));
}

@Test
public void testListViewBehaviorSelect() {
ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
createBehavior(listView);
int last = 1;
listView.getSelectionModel().select(last);
assertEquals("anchor must be set", last, listView.getProperties().get("anchor"));
}

@Test
public void testListViewBehaviorDispose() {
ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
WeakReference<BehaviorBase<?>> weakRef = new WeakReference<>(createBehavior(listView));
listView.getSelectionModel().select(1);
weakRef.get().dispose();
assertNull("anchor must be cleared after dispose", listView.getProperties().get("anchor"));
}

//------------------ setup/cleanup

@After
public void cleanup() {
Thread.currentThread().setUncaughtExceptionHandler(null);
}

@Before
public void setup() {
Thread.currentThread().setUncaughtExceptionHandler((thread, throwable) -> {
if (throwable instanceof RuntimeException) {
throw (RuntimeException)throwable;
} else {
Thread.currentThread().getThreadGroup().uncaughtException(thread, throwable);
}
});
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ public static Collection<Object[]> data() {
// step 1: file issues (where not yet done), add informal ignore to entry
// step 2: fix and remove from list
List<Class<? extends Control>> leakingClasses = List.of(
ListView.class,
PasswordField.class,
TableView.class,
TextArea.class,
Expand Down
Loading

0 comments on commit a02e09d

Please sign in to comment.