On Tue, 17 Aug 2021 16:15:47 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:
>
> 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
