Why does a TableCell's isSelected() method give different results when moving forwards vs backwards in a JavaFX8 TableRow?

Question

I have an InvalidationListener set on the selectedProperty() of a custom TableCell. I use it to change the editability and background colour of rows based on a boolean value in the data model.

I was debugging the mysterious disappearance of row color when I click on a cell and then click on a previous cell in the same row and tracked it down to isSelected()'s value at the point at which row colour is set.

From what I could see, isSelected() changes from false to true while moving forwards in a row, but changes from true to false while moving backwards.

Why does it behave like that? Shouldn't it be consistent in the changes it reports?

I tried using selectedProperty.get() instead of isSelected(), and a ChangeListener rather than an InvalidationListener, but got the same result. And the same thing happens if I navigate with the keyboard rather than the mouse.

Here's a MVCE that demonstrates the issue. It's based on user Slaw's answer here How to set a TableRow's background colour based on whether or not it's selected and/or a value in the data model, in a JavaFX8 TableView? and user kleopatra's answer here TreeTableView : setting a row not editable.

To reproduce the behaviour, click the second or third cell of a row. It will change colour according to the matrix shown below. Then click on a previous cell in the same row. The row colour should disappear and revert to default. The values of isSelected() and selectedProperty() will output to the console.

I'm using JavaFX8 (JDK1.8.0_181), NetBeans 8.2 and Scene Builder 8.3.

Test45_Listeners.java

package test45_listeners;

import java.util.Arrays;
import javafx.application.Application;
import static javafx.application.Application.launch;
import javafx.beans.Observable;
import javafx.beans.property.BooleanProperty;
import javafx.beans.property.SimpleBooleanProperty;
import javafx.beans.property.SimpleStringProperty;
import javafx.beans.property.StringProperty;
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;
import javafx.scene.Parent;
import javafx.scene.Scene;
import javafx.scene.control.TableColumn;
import javafx.scene.control.TableRow;
import javafx.scene.control.TableView;
import javafx.scene.layout.BorderPane;
import javafx.stage.Stage;

public class Test45_Listeners extends Application {

    private final ObservableList<TestModel> olTestModel 
            = FXCollections.observableArrayList(testmodel -> new Observable[] {});

    private Parent createContent() {

        //Initialise the TableView and data
        createDummyData(100);

        TableView<TestModel> table = new TableView<>();

        TableColumn<TestModel, String> colField1 = new TableColumn<>("field1");
        colField1.setCellValueFactory(features -> features.getValue().field1Property());
        colField1.setCellFactory(col -> TestTextCell.createStringTextCell(TestModel::lockedProperty));

        TableColumn<TestModel, String> colField2 = new TableColumn<>("field2");
        colField2.setCellValueFactory(features -> features.getValue().field2Property());
        colField2.setCellFactory(col -> TestTextCell.createStringTextCell(TestModel::lockedProperty));

        TableColumn<TestModel, String> colField3 = new TableColumn<>("field3");
        colField3.setCellValueFactory(features -> features.getValue().field3Property());
        colField3.setCellFactory(col -> TestTextCell.createStringTextCell(TestModel::lockedProperty));

        table.setItems(olTestModel);
        table.getColumns().addAll(Arrays.asList(colField1, colField2, colField3));
        table.setEditable(true);
        table.getSelectionModel().setCellSelectionEnabled(true);
        table.setColumnResizePolicy(TableView.CONSTRAINED_RESIZE_POLICY);

        //Set a row factory to set the background colour of any LOCKED row to be yellow
        table.setRowFactory(tv -> {
            TableRow<TestModel> row = new TableRow<TestModel>() {
                @Override
                public void updateItem(TestModel item, boolean empty) {
                    super.updateItem(item, empty);
                    boolean locked = false;
                    if ( getItem() != null ) {
                        locked = getItem().lockedProperty().get();
                        setEditable( ! locked );
                    }

                    if ( !isEmpty() && locked ) {
                        setStyle("-fx-background-color: yellow;");
                    } else {
                        setStyle(null);
                    }

                }
            };
            return row;
        });

        BorderPane content = new BorderPane(table);
        return content;
    }

    private void createDummyData(int count) {

        for ( int i=0; i<count; i++ ) {
            boolean locked = Math.random() >= 0.5;
            olTestModel.add(new TestModel(locked, (locked ? "row LOCKED" : "row NOT locked"),
                    Integer.toString(i), "a"+Integer.toString(i)));
        }

    }

    private class TestModel {

        private final BooleanProperty locked;
        private final StringProperty field1;
        private final StringProperty field2;
        private final StringProperty field3;

        public TestModel(

            boolean locked,
            String field1,
            String field2,
            String field3
        ) {
            this.locked = new SimpleBooleanProperty(locked);
            this.field1 = new SimpleStringProperty(field1);
            this.field2 = new SimpleStringProperty(field2);
            this.field3 = new SimpleStringProperty(field3);
        }

        public boolean getLocked() {return locked.get();}
        public void setLocked(boolean locked) {this.locked.set(locked);}
        public BooleanProperty lockedProperty() {return locked;}

        public String getField1() {return field1.get();}
        public void setField1(String field1) {this.field1.set(field1);}
        public StringProperty field1Property() {return field1;}

        public String getField2() {return field2.get();}
        public void setField2(String field2) {this.field2.set(field2);}
        public StringProperty field2Property() {return field2;}

        public String getField3() {return field3.get();}
        public void setField3(String field3) {this.field3.set(field3);}
        public StringProperty field3Property() {return field3;}

    }

    @Override
    public void start(Stage stage) throws Exception {
        stage.setScene(new Scene(createContent()));
        stage.setTitle("Test");
        stage.setWidth(350);
        stage.show();
    }

    public static void main(String[] args) {
        launch(args);
    }

}

TestTextCell.java

package test45_listeners;

import java.util.function.Function;
import javafx.beans.InvalidationListener;
import javafx.beans.Observable;
import javafx.beans.WeakInvalidationListener;
import javafx.beans.binding.Bindings;
import javafx.beans.property.BooleanProperty;
import javafx.beans.property.SimpleBooleanProperty;
import javafx.beans.value.ChangeListener;
import javafx.beans.value.ObservableValue;
import javafx.beans.value.WeakChangeListener;
import javafx.scene.control.ContentDisplay;
import javafx.scene.control.TableCell;
import javafx.scene.control.TextField;
import javafx.util.StringConverter;
import javafx.util.converter.DefaultStringConverter;

public class TestTextCell<S, T> extends TableCell<S, T> {

    public final TextField textField = new TextField();
    public final StringConverter<T> converter;

    private BooleanProperty isLockedProperty;

    private final InvalidationListener strongListener = (Observable observable) -> {
        updateStyle();
    };
    private final WeakInvalidationListener weakListener = new WeakInvalidationListener(strongListener);
/*
    public ChangeListener<Boolean> strongListener = (ObservableValue<? extends Boolean> observable, Boolean wasFocused, Boolean isNowFocused) -> {
        updateStyle();
    };
    public final WeakChangeListener<Boolean> weakListener = new WeakChangeListener<Boolean>(strongListener);
*/
    //********************************************************************************************************************* 
    public TestTextCell(StringConverter<T> converter, Function<S, BooleanProperty> methodGetLockedProperty) {

        this.converter = converter;
        setGraphic(textField);
        setContentDisplay(ContentDisplay.TEXT_ONLY);

        itemProperty().addListener((obx, oldItem, newItem) -> {
            if (newItem == null) {
                setText(null);
            } else {
                setText(converter.toString(newItem));
                if ( methodGetLockedProperty != null ) {
                    S datamodel = getTableView().getItems().get(getIndex());
                    isLockedProperty = methodGetLockedProperty.apply(datamodel);
                } else {
                    isLockedProperty = new SimpleBooleanProperty(false);
                }
            }
        });

        //Add the invalidation listener
        selectedProperty().addListener(strongListener);

    }

    //*******************************************************************************************************************    
    public static <S> TestTextCell<S, String> createStringTextCell(Function<S, BooleanProperty> methodGetLockedProperty) {
        return new TestTextCell<S, String>(new DefaultStringConverter(), methodGetLockedProperty);
    }

    //*******************************************************************************************************************    
    @Override
    protected void updateItem(T item, boolean empty) {

        T oldItem = (T) getItem();
        if (oldItem != null) {
            selectedProperty().removeListener(weakListener);
        }

        super.updateItem(item, empty);

        if (item != null) {
            selectedProperty().addListener(weakListener);

            if ( getTableRow() != null ) {
                if (getGraphic() != null) {
                    getGraphic().disableProperty().bind(
                        Bindings.not(getTableRow().editableProperty())
                    );
                }
            }
        }

    }

    @Override
    public void startEdit() {
        if ( ! isLockedProperty.get() ) {
            super.startEdit();
            if (getGraphic() != null) {
                textField.setText(converter.toString(getItem()));
                setContentDisplay(ContentDisplay.GRAPHIC_ONLY);
                getGraphic().requestFocus();
            }
        }
    }

    @Override
    public void cancelEdit() {
        super.cancelEdit();
        setContentDisplay(ContentDisplay.TEXT_ONLY);
    }

    //*******************************************************************************************************************    
    private void updateStyle() {

        System.out.println("in updateStyle(), isLockedProperty = " + isLockedProperty.get() 
                + ", isSelected() = " + isSelected() + ", selectedProperty.get() = " + selectedProperty().get());
        if ( getTableRow() != null ) {
            if ( isLockedProperty.get() && isSelected() ) {
//            if ( isLockedProperty.get() && selectedProperty().get() ) {
                getTableRow().setStyle("-fx-background-color: pink;");
            } else if ( isLockedProperty.get() && ! isSelected()) {
//            } else if ( isLockedProperty.get() && ! selectedProperty().get() ) {
                getTableRow().setStyle("-fx-background-color: yellow;");
            } else if ( ! isLockedProperty.get() && isSelected() ) {
//            } else if ( ! isLockedProperty.get() && selectedProperty().get() ) {
                getTableRow().setStyle("-fx-background-color: #b6e1fc;");
            } else if ( ! isLockedProperty.get() && ! isSelected() ) {
//            } else if ( ! isLockedProperty.get() && ! selectedProperty().get() ) {
                getTableRow().setStyle(null);
            } else {
                throw new AssertionError("how did I get here?");
            }
        }

    }

}

don't understand your code ;) weakListener? strongListener? listening to _item_? don't you are painting yourself into a corner (independent of whether or not you stumbled onto a bug which is well possible) - properties are independent of each other and the order of change is unspecified. That's probably what you seeing: basically updateStyle is called from listeners to different properties and try to access the other - that cannot work .. — kleopatra, Oct 22 '18 at 08:29
@kleopatra Sorry for the code, I’m still a relative Java newbie. I understand (conceptually) what you’re saying but have been stuck since I read your comment, trying to figure out how else I could (a) detect cell selection changes and (b) get the boolean value for the row for the current `item` (it’s not `item`; it’s in a different field in the model). I shall sleep on it and have another go in the morning. If you have any hints or suggestions, I’d love to have them. Thanks! — GreenZebra, Oct 22 '18 at 10:45
BTW: there is a [`updateSelected` method](https://docs.oracle.com/javase/9/docs/api/javafx/scene/control/TableCell.html#updateSelected-boolean-) in `TableCell`. You should be able to update the style similar to the way the cell is supposed to be updated in `updateItem`. BTW: using a `WeakInvalidationListener` for a property of the cell storing a reference to the listener is pointless, since the listener is available for garbage collection iff the cell is available for garbage collection... — fabian, Oct 22 '18 at 12:09
@fabian fyi: https://bugs.openjdk.java.net/browse/JDK-8145588 for some really dirty fix of order-of-notification ;) — kleopatra, Oct 22 '18 at 12:35

score 1 · Accepted Answer · edited Nov 19 '19 at 12:15

Apart from all the good & valuble suggestions in the comments, to answer the actual question of "why" :: the TableCell's isSelected() method is indeed working fine and there is a problem in your code which is computing the required logic wrongly.

To justify this, I would like you to update your print statement in updateStyle() method to below

System.out.println(getItem() + " :: in updateStyle(), isLockedProperty = " + isLockedProperty.get() + ", isSelected() = " + isSelected());

Lets consider for the first row:

If I select the cells from left to right, the output is as below::

// When field1 is selected
row LOCKED :: in updateStyle(), isLockedProperty = true, isSelected() = true
row LOCKED :: in updateStyle(), isLockedProperty = true, isSelected() = true

// When field2 is selected
row LOCKED :: in updateStyle(), isLockedProperty = true, isSelected() = false
row LOCKED :: in updateStyle(), isLockedProperty = true, isSelected() = false
0 :: in updateStyle(), isLockedProperty = true, isSelected() = true
0 :: in updateStyle(), isLockedProperty = true, isSelected() = true

// When field3 is selected
0 :: in updateStyle(), isLockedProperty = true, isSelected() = false
0 :: in updateStyle(), isLockedProperty = true, isSelected() = false
a0 :: in updateStyle(), isLockedProperty = true, isSelected() = true
a0 :: in updateStyle(), isLockedProperty = true, isSelected() = true

For time being, dont bother about the double print of each selection(because you are setting both stronglistener and weaklistener). But from the output what we understand is each time we select a cell, the earlier cell's selected value is set to false which is correct. This works well for your style updating as 'true' is always coming after the 'false'.

Now try selecting the cells from right to left, the output is as below::

// When field3 is selected
a0 :: in updateStyle(), isLockedProperty = false, isSelected() = true
a0 :: in updateStyle(), isLockedProperty = false, isSelected() = true

// When field2 is selected
0 :: in updateStyle(), isLockedProperty = false, isSelected() = true
0 :: in updateStyle(), isLockedProperty = false, isSelected() = true
a0 :: in updateStyle(), isLockedProperty = false, isSelected() = false
a0 :: in updateStyle(), isLockedProperty = false, isSelected() = false

// When field1 is selected
row NOT locked :: in updateStyle(), isLockedProperty = false, isSelected() = true
row NOT locked :: in updateStyle(), isLockedProperty = false, isSelected() = true
0 :: in updateStyle(), isLockedProperty = false, isSelected() = false
0 :: in updateStyle(), isLockedProperty = false, isSelected() = false

From the output it is very clear that the 'true' is coming before 'false'. In other words Perhaps JavaFX internally always updates the cell selection sequentially from left to right.

This is where your code fails. When you change the selection from righ to left, the left cell update is called first and then the right cell update is called. And as your right cell is not selected,you can never see your desired selected row style.

Possible Solution::

I am again mentioning that please do consider all the suggestions in the comments. As you are planning to give up the idea, I just want to let you know that this is still a doable implementation. There can be other better ways, but this is one possible solution.

From the above analysis, it is very clear that to get your desired behavior relying on cell selection is not an apt solution. I would recommend to perform all the row styling in the row factory itself.

Ofcourse for this you need a new BooleanProperty in you table item, to let you know whether the row is selected or not.

Please do the following changes in your current code:

1) Comment out the updateStyle() method in TestTextCell.java and get rid of all listeners.

2) Add a new property in TestModel and the appropriate getters and setters.

private final BooleanProperty selected = new SimpleBooleanProperty();

3) Add a tableView's selectedItem listener to update the item selection in the model.

table.getSelectionModel().selectedItemProperty().addListener((obs, oldItem, newItem) -> {
    if (oldItem != null) {
        oldItem.setSelected(false);
    }
    if (newItem != null) {
        newItem.setSelected(true);
    }
});

4) Update your row factory implementation to below:

//Set a row factory to set the background colour of any LOCKED row to be yellow
table.setRowFactory(tv -> {
    TableRow<TestModel> row = new TableRow<TestModel>() {

        private final ChangeListener<Boolean> listener = (o, v, newValue) -> updateStyle();

        {
            itemProperty().addListener((obs, oldItem, newItem) -> {
                if (oldItem != null) {
                    oldItem.selectedProperty().removeListener(listener);
                }
                if (newItem != null) {
                    newItem.selectedProperty().addListener(listener);
                }
            });
        }

        @Override
        public void updateItem(TestModel item, boolean empty) {
            super.updateItem(item, empty);
            if (getItem() != null) {
                setEditable(!getItem().getLocked());
            } else {
                setEditable(false);
            }
            updateStyle();
        }

        private void updateStyle(){
            if (getItem() != null) {
                boolean isLocked = getItem().getLocked();
                boolean isSelected = getItem().isSelected();
                if (isLocked) {
                    if (isSelected) {
                        setStyle("-fx-background-color: pink;");
                    } else {
                        setStyle("-fx-background-color: yellow;");
                    }
                } else {
                    if (isSelected) {
                        setStyle("-fx-background-color: #b6e1fc;");
                    } else {
                        setStyle("-fx-background-color: transparent;");
                    }
                }
            } else {
                setStyle("-fx-background-color: transparent;");
            }
        }
    };
    return row;
});

I would really welcome for any corrections in my understandings.

Thank you, that’s brilliant! I couldn’t figure out how to get the selected cell in the row factory and now I know. Having a `selected` property in the model is very clever. I actually got my MVCE working this morning using `updateSelected()`, but it was a workaround and I wasn’t comfortable with it. Your solution is perfect. On the point of `isSelected()`. I’m not sure I’d say it’s working fine, as it seems illogical to me to give different results for forward and backward movement. However, it is what it is. Thanks again for your help. It’s much appreciated. :-) — GreenZebra, Oct 23 '18 at 05:54

Why does a TableCell's isSelected() method give different results when moving forwards vs backwards in a JavaFX8 TableRow?

Test45_Listeners.java

TestTextCell.java

1 Answers1