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

[Jeanette Winzenburg]

On Thu, 8 Jul 2021 21:15:29 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:
> 
>   Reordered commit

will take a closer look next week or so, for now just a starter: fix looks fine, verified test failing/passing before/after :)

As to the tests, left a couple of inline comments to ListCellStartEditTest - others should be changed in the same way.

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

> 73:         this.listCell = listCell;
> 74:     }
> 75: 

this hit me when trying separate out the (accidentally?) single test method: all tests work on the same instance of the cell, so its state is unpredictable. Instead, parameterize on a supplier and let it provide a new cell in setup.

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

> 86: 
> 87:         listCell.updateIndex(0);
> 88: 

for my taste, this method is doing too much: the first line is the real subject of the issue (throwing an NPE), the rest of the method is testing the other issue (make sure that editability constraints are respected for all combinations). 

My suggestion: separate them out into different methods, f.i. 

- testStartEditMustNotThrowNPE (containing only the single offending line)
- testStartEditRespectsEditable (containing the rest)
- 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

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

Changes requested by fastegal (Reviewer).

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