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

Jeanette Winzenburg fastegal at openjdk.java.net
Sun Sep 26 12:01:01 UTC 2021

On Sun, 19 Sep 2021 11:52:55 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:
>   Addressed review comments
>   - cell is supplied in setup() method
>   - treeItem = null guard is done before calling startEdit()
>   - Added checks for the cell graphic

fix looks okay now :)

Still a bit weary about the tests: personally, I don't like re-using and re-configuring the same instance of an object inside a longish test method (here: in the tight loop). But then, there are other tests in the code base doing it so probably okay here as well.


Marked as reviewed by fastegal (Reviewer).

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

More information about the openjfx-dev mailing list