On Thu, 8 Jul 2021 21:15:29 GMT, Marius Hanl <mh...@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
