RFR: 8188026: TextFieldXXCell: NPE on calling startEdit [v4]

[Jeanette Winzenburg]

On Tue, 17 Aug 2021 16:15:47 GMT, Marius Hanl <mhanl at openjdk.org> wrote:

>> This PR sets an unified logic to every **startEdit()** method of all Cell implementations.
>> So startEdit() is always doing the same now:
>> 
>> `super.startEdit();`
>> `if (!isEditing()) {
>> return;
>> }` 
>> 
>> This will prevent a NPE while also being cleaner (no more double checks)
>> The commits are splitted into 4 base commits:
>> - First commit changes the startEdit() for TableCell implementations (8 files)
>> - Second commit changes the startEdit() for TreeTableCell implementations (7 files)
>> - Third commit changes the startEdit() for ListCell implementations (7 files)
>> - Fourth commit changes the startEdit() for TreeCell implementations (7 files)
>> 
>> While writing tests, I found out that the CheckBoxListCell and the CheckBoxTreeCell don't disable their CheckBox, which is wrong.
>> In CheckBoxTableCell and CheckBoxTreeTableCell the CheckBox is disabled, but they don't check their dependencies e.g. TableColumn for null, leading to a NPE if not set.
>> 
>> I wrote a follow-up and assigned it to myself: https://bugs.openjdk.java.net/browse/JDK-8270042
>> The aim of this should be, that all 4 CheckBox...Cells have a proper CheckBox disablement while also being nullsafe.
>> 
>> ~Note 1: I removed the tests in TableCellTest added in https://github.com/openjdk/jfx/pull/529 in favor of the new parameterized TableCellStartEditTest~
>> Note 2: This also fixes https://bugs.openjdk.java.net/browse/JDK-8268295
>
> Marius Hanl has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Separated test and made the cell a supplier instead

added inline comments, plus: there's an open change request in my last review: 

-   not yet covered: sanity test that startEdit really installs the editing visuals - f.i. by checking that its graphic is null before/ not-null after startEdit

might be arguable (might have found the failing tests, though, .. or not :) - personally, I would prefer to only resolve if fully addressed: either after adding the change or consenting to not adding it.

modules/javafx.controls/src/main/java/javafx/scene/control/cell/ChoiceBoxListCell.java line 304:

> 302:         if (isEditing()) {
> 303:             setText(null);
> 304:             setGraphic(choiceBox);

at this point, the cell is editing (backed out earlier, if not)

modules/javafx.controls/src/main/java/javafx/scene/control/cell/ChoiceBoxTreeCell.java line 301:

> 299:             return;
> 300:         }
> 301: 

(darn, can't add the important lines - which is backing out if treeItem is null)

The re-ordering leads to change of behavior, here's a test that's passing/failing before/after:

    /**
     * change of behavior: cell must not be editing if treeItem == null.
     * fails with fix, passes without
     */
    @Test
    public void testChoiceBoxTreeCellEditing() {
        TreeView<String> treeView = new TreeView<>();
        treeView.setEditable(true);
        ChoiceBoxTreeCell<String> cell = new ChoiceBoxTreeCell<>();
        cell.updateTreeView(treeView);
        cell.updateItem("TEST", false);
        
        cell.startEdit();
        assertFalse(cell.isEditing());
        assertNull(cell.getGraphic());
    }
    
same for ComboBoxTreeCell

modules/javafx.controls/src/main/java/javafx/scene/control/cell/ComboBoxTreeCell.java line 331:

> 329:         if (!isEditing()) {
> 330:             return;
> 331:         }

same as ChoiceBoxTreeCell

modules/javafx.controls/src/main/java/javafx/scene/control/cell/TextFieldTreeCell.java line 195:

> 193:         if (!isEditing()) {
> 194:             return;
> 195:         }

similar to ChoiceBox/ComboBoxTreeCell, except that a similar test fails both before/after the fix

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellStartEditTest.java line 27:

> 25: 
> 26: package test.javafx.scene.control;
> 27: 

the issue is about specialized cells in package xx.cell, the tests should be also reside in that package

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellStartEditTest.java line 65:

> 63:     public static Collection<Object[]> data() {
> 64:         return wrapAsObjectArray(List.of(ListCell::new, ComboBoxListCell::new, TextFieldListCell::new,
> 65:                 ChoiceBoxListCell::new, () -> new CheckBoxListCell<>(obj -> new SimpleBooleanProperty())));

the issue is about the specialized cells, so I would suggest to remove the base Cell here, it's fully (maybe :) already

Doing so, makes it also simpler to test the not/existance of the editing ui (which is not yet addressed in the tests)

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellStartEditTest.java line 84:

> 82:     @Test
> 83:     public void testStartEditMustNotThrowNPE() {
> 84:         ListCell<String> listCell = listCellSupplier.get();

the usual pattern is to create the instances is to do so in setup, not in every single test

-------------

Changes requested by fastegal (Reviewer).

PR: https://git.openjdk.java.net/jfx/pull/569