I've encountered what appears to be an anomaly in how ListChangeListener handles batch removals (i.e. removeAll(Collection). If the items in the Collection are contiguous then the handling operation specified in the listener works fine. However, if the Collection are not contiguous then the operation specified in the listener halts once contiguity is broken.
This can best be explained by way of example. Assume the ObservableList consists of the following items:
Assume also that there is a separate ObservableList that tracks the hashCode values for the colors, and that a ListChangeListener has been added that removes the hashCode from the second list whenever one or more of the items in the first list is removed. If the 'removal' Collection consists of "red", "orange" and "yellow" then the code in the listener removes the hashCodes for all three items from the second list as expected. However, if the 'removal' Collection consists of "red", "orange" and "green", then the code in the listener stops after removing the hashCode for "orange" and never reaches "green" like it should.
A short app that illustrates the problem is set out below. The listener code is in a method named buildListChangeListener() that returns a listener that is added to the 'Colors' list. To run the app it helps to know that:
ComboBox specifies three colors that are contiguous as explained above; clicking the 'Remove' button will cause them to be removed from the 'Colors' list and their hashCodes
from the other list.Here's the code for the app:
package test;
import static java.util.Objects.isNull;
import java.util.ArrayList;
import java.util.List;
import java.util.stream.Collectors;
import javafx.application.Application;
import javafx.application.Platform;
import javafx.collections.FXCollections;
import javafx.collections.ListChangeListener;
import javafx.event.EventHandler;
import javafx.geometry.Insets;
import javafx.scene.Scene;
import javafx.scene.canvas.Canvas;
import javafx.scene.canvas.GraphicsContext;
import javafx.scene.control.Button;
import javafx.scene.control.ComboBox;
import javafx.scene.control.ContentDisplay;
import javafx.scene.control.Label;
import javafx.scene.control.ListCell;
import javafx.scene.control.ListView;
import javafx.scene.layout.Background;
import javafx.scene.layout.BackgroundFill;
import javafx.scene.layout.CornerRadii;
import javafx.scene.layout.HBox;
import javafx.scene.layout.StackPane;
import javafx.scene.layout.VBox;
import javafx.scene.paint.Color;
import javafx.stage.Stage;
import javafx.stage.WindowEvent;
import javafx.util.Pair;
public class RemoveAllItemsBug extends Application {
private StackPane stackPane;
private HBox hbox;
private VBox vbox1;
private Label label1;
private ListView<Pair<String, Color>> colors;
private VBox vbox2;
private Label label2;
private ListView<Integer> hashCodes;
private VBox vbox3;
private Label label3;
private ComboBox<String> actionModes;
private Button btnRemove;
private Button btnRefresh;
final static private String CONSECUTIVE = "consecutive", BROKEN = "broken";
private final EventHandler<WindowEvent> onCloseRequestListener = evt -> {
Platform.exit();
System.exit(0);
};
@Override
public void start(Stage primaryStage) throws Exception {
primaryStage.setTitle("DUMMY APP");
// Necessary to ensure stage closes completely and javaw.exe stops running
primaryStage.setOnCloseRequest(onCloseRequestListener);
primaryStage.setWidth(550);
primaryStage.setHeight(310);
// primaryStage.setMinWidth(550);
// primaryStage.setMinHeight(310);
/*
* Code block below for width/height property printouts is used to
* test for an optimal size for the app. Once the size is determined
* they may (and should be) commented out as here.
*/
primaryStage
.widthProperty()
.addListener((width, oldWidth, newWidth) -> {
System.out.println("width: " + newWidth);
});
primaryStage
.heightProperty()
.addListener((height, oldHeight, newHeight) -> {
System.out.println("height: " + newHeight);
});
initializeUI();
installSimpleBehavior();
installListChangeListener();
primaryStage.setScene(new Scene(stackPane));
primaryStage.show();
}
private void installListChangeListener() {
/*
* The 'listChangeListenerUsingIf()' method returns a listener that
* uses an 'if (c.next()) ...' statement to access the first change in
* the Change variable (c). For purposes of accessing the first change
* this is functionally equivalent to a 'while (c.next()) ...'
* statement. However, because the Change variable may contain
* multiple 'remove' changes where each change is represented by a
* separate 'getRemoved()' list, the 'if (c.next())' statement will
* catch only the first change while the 'while (c.next())' statement
* (which is used in the 'listChangeListenerUsingWhile()' method)
* catches them all.
*
* The code below should be commented out as appropriate before
* running the app in order to see the difference.
*
* This case illustrates a serious flaw in the ListChangeListener API
* documentation because it fails to indicate that the Change variable
* may include multiple 'remove' changes and that each such change
* must be accessed in a separate iteration (e.g. the 'while
* (c.next()...').
*
* In contrast, 'add' changes (i.e. changes resulting from the
* addition of one or more items to the source list), the name of the
* method that returns the change(s) is 'getAddSublist()'. This
* clearly indicates that there may be more than one list of items
* that have been added, or similarly that the total items that have
* been 'added' by the change(s) represented by the Change variable
* may be included in more than one list; thus the use of the term
* 'sublist'.
*
* The flaw is illustrated further in the cautionary note in the API
* that reads as follows:
*
* "[I]n case the change contains multiple changes of different type,
* these changes must be in the following order: <em> permutation
* change(s), add or remove changes, update changes </em> This is
* because permutation changes cannot go after add/remove changes as
* they would change the position of added elements. And on the other
* hand, update changes must go after add/remove changes because they
* refer with their indexes to the current state of the list, which
* means with all add/remove changes applied."
*
* This is certainly useful information. However, the problems
* illustrated by the case at hand (i.e. different treatment based on
* whether the changed items are continguous in the source list) are
* just as significant as the situation addressed by the note, yet
* they are not mentioned.
*
* A better understanding as to how the process works can be gained by
* running a system printout for the Change variable class
* (System.out.println("Change variable class: " +
* c.getClass().getSimpleName())) and compare the results yielded from
* changing the choice in the 'Action modes' combo box from
* 'consecutive' to 'broken'. For 'consecutive' (i.e. continguous),
* the class for the Change variable is
* ListChangeBuilder$SingleChange, for 'broken' (i.e. non-continguous)
* the class is ListChangeBuilder$IterableChange. These classes aren't
* well documented, which while regrettable is understandable inasmuch
* as they're private inner classes for restricted API. Interestingly,
* however, there is a public class MultipleAdditionAndRemovedChange
* (also restricted API) that appears to fit this case perfectly and
* is a bit more informative.
*/
// colors.getItems().addListener(listChangeListenerUsingIf());
colors.getItems().addListener(listChangeListenerUsingWhile());
}
private void initializeUI() {
//- Controls for colors
label1 = new Label("Colors");
colors = new ListView<Pair<String, Color>>();
colors.setPrefSize(150, 200);
colors.setItems(FXCollections.observableList(new ArrayList<>(colorsList())));
vbox1 = new VBox(label1, colors);
//- Controls for colors
label2 = new Label("Hash codes");
hashCodes = new ListView<Integer>();
hashCodes.setPrefSize(150, 200);
hashCodes.setItems(FXCollections.observableList(new ArrayList<>(
colorsList().stream()
.map(e -> e.hashCode())
.collect(Collectors.toCollection(ArrayList::new)))));
vbox2 = new VBox(label2, hashCodes);
//- 'Action mode' controls
label3 = new Label("Action mode");
actionModes = new ComboBox<>(
FXCollections.observableList(List.of(CONSECUTIVE, BROKEN)));
actionModes.setPrefWidth(150);
actionModes.getSelectionModel().select(0);
btnRemove = new Button("Remove");
btnRefresh = new Button("Refresh");
List.of(btnRemove, btnRefresh).forEach(b -> {
b.setMaxWidth(Double.MAX_VALUE);
VBox.setMargin(b, new Insets(5, 0, 0, 0));
});
vbox3 = new VBox(label3, actionModes, btnRemove, btnRefresh);
hbox = new HBox(vbox1, vbox2, vbox3);
hbox.setPadding(new Insets(10));
hbox.setSpacing(15);
hbox.setBackground(new Background(
new BackgroundFill(Color.DARKGRAY, CornerRadii.EMPTY, Insets.EMPTY),
new BackgroundFill(Color.WHITESMOKE, CornerRadii.EMPTY, new Insets(1))));
stackPane = new StackPane(hbox);
stackPane.setPadding(new Insets(15));
}
private void installSimpleBehavior() {
//- 'Colors' cell factory
colors.setCellFactory(listView -> {
return new ListCell<Pair<String, Color>>() {
@Override
protected void updateItem(Pair<String, Color> item, boolean empty) {
super.updateItem(item, empty);
if (isNull(item) || empty) {
setGraphic(null);
setText(null);
}
else {
HBox graphic = new HBox();
graphic.setPrefSize(15, 15);
graphic.setBackground(new Background(new BackgroundFill(
item.getValue(),
CornerRadii.EMPTY,
Insets.EMPTY)));
setGraphic(graphic);
setText(item.getKey());
setContentDisplay(ContentDisplay.LEFT);
}
}
};
});
//- 'Colors' cell factory
hashCodes.setCellFactory(listView -> {
return new ListCell<Integer>() {
@Override
protected void updateItem(Integer item, boolean empty) {
super.updateItem(item, empty);
if (isNull(item) || empty) {
setGraphic(null);
setText(null);
}
else {
HBox graphic = new HBox();
graphic.setPrefSize(15, 15);
graphic.setBackground(new Background(new BackgroundFill(
colorForHashCode(item),
CornerRadii.EMPTY,
Insets.EMPTY)));
Canvas c = new Canvas(15, 15);
GraphicsContext graphics = c.getGraphicsContext2D();
graphics.setFill(colorForHashCode(item));
graphics.fillRect(0, 0, c.getWidth(), c.getHeight());
setGraphic(c);
setText("" + item);
setContentDisplay(ContentDisplay.LEFT);
}
}
private Color colorForHashCode(int hash) {
return colorsList().stream()
.filter(e -> e.hashCode() == hash)
.map(e -> e.getValue())
.findFirst()
.orElseThrow();
}
};
});
//- 'Remove' button action
btnRemove.setOnAction(e -> {
String actionMode = actionModes.getValue();
if (CONSECUTIVE.equals(actionMode)) {
colors.getItems().removeAll(consecutiveColors());
}
else if (BROKEN.equals(actionMode)) {
colors.getItems().removeAll(brokenColors());
}
});
//- 'Refresh' button action
btnRefresh.setOnAction(e -> {
colors.getItems().setAll(colorsList());
hashCodes.getItems().setAll(colorsList()
.stream()
.map(ee -> ee.hashCode())
.collect(Collectors.toCollection(ArrayList::new)));
});
}
private ListChangeListener<Pair<String, Color>> listChangeListenerUsingIf() {
return c -> {
if (c.next()) {
System.out.println("Change variable class: " + c.getClass().getName());
if (c.wasRemoved()) {
System.out.println("Removing " + c.getRemovedSize() + " items");
c.getRemoved().forEach(e -> {
Integer hash = Integer.valueOf(e.hashCode());
hashCodes.getItems().remove(hash);
});
System.out.println("number of 'hash codes' after removal: " + hashCodes.getItems().size());
System.out.println();
}
if (c.wasAdded()) {
c.getAddedSubList().forEach(e -> {
if (hashCodes.getItems().stream().noneMatch(ee -> ee == e.hashCode()))
hashCodes.getItems().add(e.hashCode());
});
}
}
};
}
private ListChangeListener<Pair<String, Color>> listChangeListenerUsingWhile() {
return c -> {
while (c.next()) {
System.out.println("Change variable class: " + c.getClass().getName());
if (c.wasRemoved()) {
System.out.println("Removing " + c.getRemovedSize() + " items");
c.getRemoved().forEach(e -> {
Integer hash = Integer.valueOf(e.hashCode());
hashCodes.getItems().remove(hash);
});
System.out.println("number of 'hash codes' after removal: " + hashCodes.getItems().size());
System.out.println();
}
if (c.wasAdded()) {
c.getAddedSubList().forEach(e -> {
if (hashCodes.getItems().stream().noneMatch(ee -> ee == e.hashCode()))
hashCodes.getItems().add(e.hashCode());
});
}
}
};
}
private List<Pair<String, Color>> colorsList() {
return List.of(
new Pair<>("rot", Color.RED),
new Pair<>("orange", Color.ORANGE),
new Pair<>("gelb", Color.YELLOW),
new Pair<>("grün", Color.GREEN),
new Pair<>("blau", Color.BLUE),
new Pair<>("violett", Color.PURPLE),
new Pair<>("grau", Color.GRAY),
new Pair<>("schwarz", Color.BLACK));
}
private List<Pair<String, Color>> consecutiveColors() {
return List.of(
new Pair<>("gelb", Color.YELLOW),
new Pair<>("grün", Color.GREEN),
new Pair<>("blau", Color.BLUE));
}
private List<Pair<String, Color>> brokenColors() {
return List.of(
new Pair<>("rot", Color.RED),
new Pair<>("grün", Color.GREEN),
new Pair<>("blau", Color.BLUE));
}
public static void main(String[] args) {
launch(args);
}
}
Thanks in advance for any feedback.
[Edit in view of @Slaw's first comment]
This case raises several problems. @Slaw's first comment caused me to look at this differently. @Slaw is right in pointing out that using a while (c.next()) ... clause fixes the problem that is caused when a if (c.next())... clause is used.
When this is viewed holistically, however, there is more fundamental problem the led not so much to the use of the if (c.next()) clause, but disguised that error and made it very difficult to discover. This problem is the abysmal documentation for the ListChangeListener class.
I've modified the code for the sample app to include a second listener method that works properly (with a name change to the one that generated the error), together a comment as to why it was necessary and how the ListChangeListener and more particularly its Change companion, seem to work. The relevant parts of that comment are repeated below:
The listChangeListenerUsingIf() method returns a listener that uses an if (c.next()) ... statement to access the first change in the Change variable (c). For purposes of accessing the first change this is functionally equivalent to a while (c.next()) ... statement. However, because the Change variable may contain multiple 'remove' changes where each change is represented by a separate getRemoved() list, the if (c.next()) statement will catch only the first change while the while (c.next()) statement (which is used in the listChangeListenerUsingWhile() method) catches them all.
This case illustrates a serious flaw in the ListChangeListener API documentation because it fails to indicate that the Change variable may include multiple 'remove' changes and that each such change must be accessed in a separate iteration (e.g. the while (c.next()...).
In contrast, for 'add' changes (i.e. changes resulting from the addition of one or more items to the source list) the name of the method that returns the change(s) is getAddedSublist(). This clearly indicates that there may be more than one list of items that have been added, or similarly that the total items that have been 'added' by the change(s) represented by the Change variable may be included in more than one list; thus the use of the term sublist.
The flaw is illustrated further in the cautionary note in the API that reads as follows:
"[I]n case the change contains multiple changes of different type, these changes must be in the following order: permutation change(s), add or remove changes, update changes This is because permutation changes cannot go after add/remove changes as they would change the position of added elements. And on the other hand, update changes must go after add/remove changes because they refer with their indexes to the current state of the list, which means with all add/remove changes applied."
This is certainly useful information. However, the problems illustrated by the case at hand (i.e. different treatment based on whether the changed items are continguous in the source list) are just as significant as the situation addressed by the note; yet they are not mentioned.
A better understanding as to how the process works can be gained by running a system printout for the Change variable class (System.out.println("Change variable class: " + c.getClass().getSimpleName())) and compare the results yielded from changing the choice in the 'Action modes' combo box from 'consecutive' to 'broken'. For 'consecutive' (i.e. continguous), the class for the Change variable is ListChangeBuilder$SingleChange, for 'broken' (i.e. non-continguous) the class is ListChangeBuilder$IterableChange. These classes aren't well documented, which while regrettable is understandable inasmuch as they're private inner classes for restricted API. Interestingly, however, there is a public class MultipleAdditionAndRemovedChange (also restricted API) that appears to fit this case perfectly and is a bit more informative.
I hope this helps, and thanks to @Slaw for the useful input.
From the documentation of ListChangeListener.Change:
Represents a report of changes done to an
ObservableList. The change may consist of one or more actual changes and must be iterated by calling thenext()method [emphasis added].
In your implementation of ListChangeListener you have:
if (c.next()) {
// handle change...
}
This will only process a single change. You need to loop over (i.e. iterate) the changes in case there are multiple:
while (c.next()) {
// handle change...
}
Simply changing that one if to a while in your example fixes the problem you describe.
Here's an example showing how a bulk removal of non-contiguous elements results in multiple changes coalesced into a single ListChangeListener.Change object:
import javafx.collections.FXCollections;
import javafx.collections.ListChangeListener;
public class Main {
public static void main(String[] args) {
var list = FXCollections.observableArrayList(1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
list.addListener(
(ListChangeListener<Integer>)
c -> {
System.out.println("----------BEGIN_CHANGE----------");
while (c.next()) {
// for example, assume c.wasRemoved() returns true
System.out.printf(
"Removed %d element(s): %s%n", c.getRemovedSize(), c.getRemoved());
}
System.out.println("-----------END_CHANGE-----------");
});
list.removeAll(1, 7, 3, 8, 2, 10);
}
}
And the output:
----------BEGIN_CHANGE----------
Removed 3 element(s): [1, 2, 3]
Removed 2 element(s): [7, 8]
Removed 1 element(s): [10]
-----------END_CHANGE-----------
If you're familiar with JDBC you'll notice the API for iterating a ListChangeListener.Change is similar to iterating a ResultSet.