-
Bug
-
Resolution: Fixed
-
P4
-
jfx14
for details see comments in the code snippet below, which is the causing all of them. Basically it's trying to wire a path listener to the listView's fixedCellSize (to update internal state of the skin).
private void setupListeners() {
ListView listView = getSkinnable().getListView();
// memory leak if cell never attached to a listView
if (listView == null) {
getSkinnable().listViewProperty().addListener(new InvalidationListener() {
@Override public void invalidated(Observable observable) {
// a one-time listener for null -> !null listView
// so neither catches listViewA -> listViewB nor -> null
getSkinnable().listViewProperty().removeListener(this);
setupListeners();
}
});
} else {
// note: this is the listView (A) at the moment of installing the listener
this.fixedCellSize = listView.getFixedCellSize();
this.fixedCellSizeEnabled = fixedCellSize > 0;
registerChangeListener(listView.fixedCellSizeProperty(), e -> {
// 1. note: this is the cell's listView at the time of changing
// the property of the above - which might not be the same as (A)!
// being, introduces false green in tests
// 2. throws npe if cell's listView reset to null
this.fixedCellSize = getSkinnable().getListView().getFixedCellSize();
this.fixedCellSizeEnabled = fixedCellSize > 0;
});
}
}
Technical fix is to implement a reliable pattern for path listening.
But do we really need that internal state? The listener itself does not trigger any action (like f.i. a relayout), it's just setting two fields that are used in computing the xxSizes. These might as well access the current state (extracted to private methods to relieve the computation code from null checks).
Similar pattern in Tree/Table/Row/Skin and TreeCell/Skin (not in Tree/Table/Cell/Skin)
private void setupListeners() {
ListView listView = getSkinnable().getListView();
// memory leak if cell never attached to a listView
if (listView == null) {
getSkinnable().listViewProperty().addListener(new InvalidationListener() {
@Override public void invalidated(Observable observable) {
// a one-time listener for null -> !null listView
// so neither catches listViewA -> listViewB nor -> null
getSkinnable().listViewProperty().removeListener(this);
setupListeners();
}
});
} else {
// note: this is the listView (A) at the moment of installing the listener
this.fixedCellSize = listView.getFixedCellSize();
this.fixedCellSizeEnabled = fixedCellSize > 0;
registerChangeListener(listView.fixedCellSizeProperty(), e -> {
// 1. note: this is the cell's listView at the time of changing
// the property of the above - which might not be the same as (A)!
// being, introduces false green in tests
// 2. throws npe if cell's listView reset to null
this.fixedCellSize = getSkinnable().getListView().getFixedCellSize();
this.fixedCellSizeEnabled = fixedCellSize > 0;
});
}
}
Technical fix is to implement a reliable pattern for path listening.
But do we really need that internal state? The listener itself does not trigger any action (like f.i. a relayout), it's just setting two fields that are used in computing the xxSizes. These might as well access the current state (extracted to private methods to relieve the computation code from null checks).
Similar pattern in Tree/Table/Row/Skin and TreeCell/Skin (not in Tree/Table/Cell/Skin)
- blocks
-
JDK-8241364 ☂ Cleanup skin implementations to allow switching
- Open
- relates to
-
JDK-8274061 Tree-/TableRowSkin: misbehavior on switching skin
- Resolved
-
JDK-8253634 TreeCell/Skin: misbehavior on switching skin
- Resolved