8188026: TextFieldXXCell: NPE on calling startEdit #569

Maran23 · 2021-07-07T22:33:07Z

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 #529 in favor of the new parameterized TableCellStartEditTest~~
Note 2: This also fixes https://bugs.openjdk.java.net/browse/JDK-8268295

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issues

JDK-8188026: TextFieldXXCell: NPE on calling startEdit
JDK-8268295: Tree- and TableCell sub implementations should respect the row editability

Reviewers

Jeanette Winzenburg (@kleopatra - Reviewer)
Ajit Ghaisas (@aghaisas - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/569/head:pull/569
$ git checkout pull/569

Update a local copy of the PR:
$ git checkout pull/569
$ git pull https://git.openjdk.java.net/jfx pull/569/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 569

View PR using the GUI difftool:
$ git pr show -t 569

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/569.diff

# Conflicts: # modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java

bridgekeeper · 2021-07-07T22:33:30Z

👋 Welcome back mhanl! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2021-07-07T22:40:05Z

Webrevs

kevinrushforth · 2021-07-07T23:46:58Z

/reviewers 2

openjdk · 2021-07-07T23:47:59Z

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

kevinrushforth · 2021-07-07T23:48:54Z

Note 2: This also fixes https://bugs.openjdk.java.net/browse/JDK-8268295

In that case, you can add that issue to this PR using the Skara /issue add command.

kleopatra · 2021-07-08T11:31:15Z

not a review, just some initial comments :)

It has a fat scope : cross-product of

editing life-cycle methods (all xxEdit have the same issue with strengthening the precondition, not only startEdit)
base cell type (for each virtualized)
editing component on cell (textField, comboBox ..)

There might be particular problems for each in separation and additional when combined - pretty sure that there was a reason for implementing the inheritance constraint violation - we must be certain to understand that in each case and fix it correctly (might turn out to be very similar, though)

We probably should limit the scope somehow: f.i. solve

for all edit methods
for the most used editable cell (TextField)
in the most used editable control (TableView)

With the lessons learned, we can swarm out, either on the cell-type or editing-component axis. Probably should create an umbrella task to collect this and all follow-up issues.

Note 1: I removed the tests in TableCellTest added in #529 in favor of the new parameterized TableCellStartEditTest

don't know what the convention here is: but I would prefer not remove tests that are added for another issue (except they are testing the wrong thingy, as are the old tests expecting the NPE
don't mix :) here we are concerned about the NPE, nothing else (except that editing is still working, which should be covered by the old tests)

Maran23 · 2021-07-08T19:44:12Z

not a review, just some initial comments :)

It has a fat scope : cross-product of

editing life-cycle methods (all xxEdit have the same issue with strengthening the precondition, not only startEdit)

base cell type (for each virtualized)

editing component on cell (textField, comboBox ..)

There might be particular problems for each in separation and additional when combined - pretty sure that there was a reason for implementing the inheritance constraint violation - we must be certain to understand that in each case and fix it correctly (might turn out to be very similar, though)

We probably should limit the scope somehow: f.i. solve

for all edit methods

for the most used editable cell (TextField)

in the most used editable control (TableView)

With the lessons learned, we can swarm out, either on the cell-type or editing-component axis. Probably should create an umbrella task to collect this and all follow-up issues.

Note 1: I removed the tests in TableCellTest added in #529 in favor of the new parameterized TableCellStartEditTest

don't know what the convention here is: but I would prefer not remove tests that are added for another issue (except they are testing the wrong thingy, as are the old tests expecting the NPE

don't mix :) here we are concerned about the NPE, nothing else (except that editing is still working, which should be covered by the old tests)

Alright, well I don't think the tests do any harm, so I also can add them back. :)

Do we wanna create follow-ups for this, so this PR won''t get any bigger? That would be at least my preference.
Right now I see:

!isEditing check in cancelEdit() (we can return directly)
isEditing check in startEdit() (we can return directly)

Is there more?

Maran23 · 2021-07-08T19:44:29Z

/issue add JDK-8268295

openjdk · 2021-07-08T19:44:53Z

@Maran23
Adding additional issue to issue list: 8268295: Tree- and TableCell sub implementations should respect the row editability.

kleopatra · 2021-07-09T10:51:16Z

Do we wanna create follow-ups for this, so this PR won''t get any bigger? That would be at least my preference.
Right now I see:
* !isEditing check in cancelEdit() (we can return directly)

* isEditing check in startEdit() (we can return directly)
Is there more?

just checked my notes (there's a cell-editing branch in my fork where I'm experimenting) - astonishingly the answer is no, could not see anything :) And actually, seems like we don't even need to return immediately: would have expected some unhealthy side-effects on doing the switching into visual editing state more than once, but couldn't detect anything. You might try, though :)

Maran23 · 2021-07-11T18:06:52Z

Do we wanna create follow-ups for this, so this PR won''t get any bigger? That would be at least my preference.
Right now I see:
* !isEditing check in cancelEdit() (we can return directly)

* isEditing check in startEdit() (we can return directly)
Is there more?
just checked my notes (there's a cell-editing branch in my fork where I'm experimenting) - astonishingly the answer is no, could not see anything :) And actually, seems like we don't even need to return immediately: would have expected some unhealthy side-effects on doing the switching into visual editing state more than once, but couldn't detect anything. You might try, though :)

Okay. Question is, should we guard against a double edit? There is already one in TreeTableCell#startEdit, but probably forgotten in TableCell. I think it makes sense and as there is already the check in TreeTableCell, there was at least a thought of it somewhere in the past.
If there is nothing left, should I create a ticket for startEdit and for cancelEdit (this only affects the sub classes) ? :)

kleopatra · 2021-07-12T12:49:15Z

just checked my notes (there's a cell-editing branch in my fork where I'm experimenting) - astonishingly the answer is no, could not see anything :) And actually, seems like we don't even need to return immediately: would have expected some unhealthy side-effects on doing the switching into visual editing state more than once, but couldn't detect anything. You might try, though :)

Okay. Question is, should we guard against a double edit? There is already one in TreeTableCell#startEdit, but probably forgotten in TableCell. I think it makes sense and as there is already the check in TreeTableCell, there was at least a thought of it somewhere in the past.

good question, next question ;)

the oversight in startEdit of the base List/TableCell is not part of this (covered and soon fixed by JDK-8188027, the concrete misbehavior is that they fire multiple edit events
as to the "real" editing cell types (that is those that actually have an editingComponent) - we (that is now you *grin) should try hard to find a scenario where multiple starts (== multiple configuration passes of the editingComponent) might hurt. Like when the user already typed something and for some reason startEdit is called again, the configuration would delete the input.

If there is nothing left, should I create a ticket for startEdit and for cancelEdit (this only affects the sub classes) ? :)

hmm - not sure I understand what you are asking: startEdit is covered, and cancelEdit would be similar - either you find a scenario where multiple un-configure of the cell (after cancel) would hurt or not?

Maran23 · 2021-07-22T19:15:54Z

just checked my notes (there's a cell-editing branch in my fork where I'm experimenting) - astonishingly the answer is no, could not see anything :) And actually, seems like we don't even need to return immediately: would have expected some unhealthy side-effects on doing the switching into visual editing state more than once, but couldn't detect anything. You might try, though :)

Okay. Question is, should we guard against a double edit? There is already one in TreeTableCell#startEdit, but probably forgotten in TableCell. I think it makes sense and as there is already the check in TreeTableCell, there was at least a thought of it somewhere in the past.

good question, next question ;)

the oversight in startEdit of the base List/TableCell is not part of this (covered and soon fixed by JDK-8188027, the concrete misbehavior is that they fire multiple edit events

as to the "real" editing cell types (that is those that actually have an editingComponent) - we (that is now you *grin) should try hard to find a scenario where multiple starts (== multiple configuration passes of the editingComponent) might hurt. Like when the user already typed something and for some reason startEdit is called again, the configuration would delete the input.

If there is nothing left, should I create a ticket for startEdit and for cancelEdit (this only affects the sub classes) ? :)

hmm - not sure I understand what you are asking: startEdit is covered, and cancelEdit would be similar - either you find a scenario where multiple un-configure of the cell (after cancel) would hurt or not?

Finally coming back to this, when firing a startEdit() while already editing e.g. a TextFieldCell, the input which was made by you gets lost and you need to start over. So this can be a potential follow-up. I didn't found anything for cancelEdit().

kleopatra · 2021-07-28T15:09:27Z

Finally coming back to this, when firing a startEdit() while already editing e.g. a TextFieldCell, the input which was made by you gets lost and you need to start over. So this can be a potential follow-up. I didn't found anything for cancelEdit().

thinking about it again: for a fully configured cell, there is no difference in behaviour before/after the fix - so the fix should be fine :) I'll start a review next week or so (after tax deadline ;) - for now will just add a suggestion for the tests.

kleopatra

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

kleopatra

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.

Arguing in favor of testing the ui state after startEdit: this is covering an implicit fix (that is the other way round as the test added inline) for incorrect showing the editing ui if not editing (ComboBoxTree/TableCell)

@Test
public void testComboBoxTableCellEditing() {
    TableView<String> tableView = new TableView<>();
    tableView.setEditable(true);
    TableColumn<String, String> column = new TableColumn<>("test");
    ComboBoxTableCell<String, String> cell = new ComboBoxTableCell<>("one", "two");
    cell.updateTableView(tableView);
    cell.updateTableColumn(column);
    cell.startEdit();
    assertFalse(cell.isEditing());
    assertNull(cell.getGraphic());
}

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

kleopatra · 2021-08-19T12:16:28Z

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

+        super.startEdit();
+        if (!isEditing()) {
            return;
        }



(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

Hm.. weird that the super class is firing an edit event even with treeItem = null. Maybe this should be investigated in a follow-up. Good catch though. :)

yeah, there are whacky aspects:

we can switch a cell that's not attached to its surroundings (like table, column, treeItem, off-range) into editing state - the only constraint being that it's not empty

the other way round: a cell that didn't switch into editing will nevertheless fire a editStart on its target

Definitely leeway for improvements ;)

the other way round: a cell that didn't switch into editing will nevertheless fire a editStart on its target

actually, it's worse because it's not only firing but might switch the control into an invalid editing state (f.i. off-range editingIndex for listView) or producing a stackOverflowError (TableCell/View) when properly updating the control's editing location (that is after fix of JDK-8187474)

filed JDK-8274433 as P3 to cover this part

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

kleopatra · 2021-08-19T12:25:16Z

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

+        super.startEdit();
+        if (!isEditing()) {
            return;
        }


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

hm interesting enough this tree cell has no treeItem = null guard (unlike the others). TreeCells are very weird 😃

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellStartEditTest.java

kleopatra · 2021-09-10T11:42:07Z

@Maran23 wondering why my review comments aren't addressed? Anything unclear?

Maran23 · 2021-09-13T19:41:49Z

@Maran23 wondering why my review comments aren't addressed? Anything unclear?

Everything clear, I was just busy the last days. But I think I will address everything in this week. If you can't continue without it e.g. this PR is blocking your (future) work, you can also continue on this. :)

- cell is supplied in setup() method - treeItem = null guard is done before calling startEdit() - Added checks for the cell graphic

kleopatra

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.

openjdk · 2021-10-01T11:20:18Z

@Maran23 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8188026: TextFieldXXCell: NPE on calling startEdit
8268295: Tree- and TableCell sub implementations should respect the row editability

Reviewed-by: fastegal, aghaisas

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 71 new commits pushed to the master branch:

30f5606: 8231644: TreeTableView Regression: Indentation wrong using Label as column content type
55faac4: 8271474: Tree-/TableCell: inconsistent edit event firing pattern
478512b: 8274107: Cherry pick GTK WebKit 2.32.4 changes
4b9cb21: 8273969: Memory Leak on the Runnable provided to Platform.startup
338b999: 8273946: Move clearQuad method to BaseShaderGraphics superclass
5c355fa: 8090158: Wrong implementation of adjustValue in scrollBars
f59a057: 8090547: Allow for transparent backgrounds in WebView
d356340: 8172095: Let Node.managed become CSS-styleable
b2ecfac: 8271398: GTK3 drag view image swaps red and blue color channels
7329279: 8273754: Re-introduce Automatic-Module-Name in empty jars
... and 61 more: https://git.openjdk.java.net/jfx/compare/386f6d7a56d9cfb334c210fccd29e1c5da58b591...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kleopatra, @aghaisas) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

Maran23 · 2021-10-01T11:51:15Z

/integrate

openjdk · 2021-10-01T11:52:21Z

@Maran23
Your change (at version bf9190b) is now ready to be sponsored by a Committer.

kleopatra · 2021-10-01T12:26:08Z

/sponsor

openjdk · 2021-10-01T12:26:56Z

Going to push as commit 64aa926.
Since your change was applied there have been 72 commits pushed to the master branch:

f3c72b9: 8214158: Implement HostServices.showDocument on macOS without calling AWT
30f5606: 8231644: TreeTableView Regression: Indentation wrong using Label as column content type
55faac4: 8271474: Tree-/TableCell: inconsistent edit event firing pattern
478512b: 8274107: Cherry pick GTK WebKit 2.32.4 changes
4b9cb21: 8273969: Memory Leak on the Runnable provided to Platform.startup
338b999: 8273946: Move clearQuad method to BaseShaderGraphics superclass
5c355fa: 8090158: Wrong implementation of adjustValue in scrollBars
f59a057: 8090547: Allow for transparent backgrounds in WebView
d356340: 8172095: Let Node.managed become CSS-styleable
b2ecfac: 8271398: GTK3 drag view image swaps red and blue color channels
... and 62 more: https://git.openjdk.java.net/jfx/compare/386f6d7a56d9cfb334c210fccd29e1c5da58b591...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-10-01T12:27:07Z

@kleopatra @Maran23 Pushed as commit 64aa926.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Maran23 added 6 commits July 8, 2021 00:01

8188026: Implemented for TableCell

e60632c

8188026: Implemented for TreeTableCell

7ba0caa

8188026: Implemented for ListCell

f00d867

8188026: Implemented for TreeCell

4fdd50e

8188026: Test clean up + added ignored test case

246f28c

Merge remote-tracking branch 'openjfx/master' into 8188026-cell-editing

b55970c

# Conflicts: # modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java

openjdk bot added the rfr Ready for review label Jul 7, 2021

kevinrushforth requested review from aghaisas and kleopatra July 7, 2021 23:49

Added tests back

8189eb0

Reordered commit

19da910

kleopatra suggested changes Jul 28, 2021

View reviewed changes

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellStartEditTest.java Outdated Show resolved Hide resolved

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellStartEditTest.java Outdated Show resolved Hide resolved

Separated test and made the cell a supplier instead

fa89fcc

kleopatra suggested changes Aug 19, 2021

View reviewed changes

Addressed review comments

bf9190b

- cell is supplied in setup() method - treeItem = null guard is done before calling startEdit() - Added checks for the cell graphic

kleopatra approved these changes Sep 26, 2021

View reviewed changes

aghaisas approved these changes Oct 1, 2021

View reviewed changes

openjdk bot added the ready Ready to be integrated label Oct 1, 2021

openjdk bot added the sponsor Ready to sponsor label Oct 1, 2021

openjdk bot closed this Oct 1, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review labels Oct 1, 2021

openjdk bot removed the sponsor Ready to sponsor label Oct 1, 2021

Maran23 deleted the 8188026-cell-editing-merged branch November 9, 2021 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8188026: TextFieldXXCell: NPE on calling startEdit #569

8188026: TextFieldXXCell: NPE on calling startEdit #569

Maran23 commented Jul 7, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 7, 2021

mlbridge bot commented Jul 7, 2021 •

edited

Loading

kevinrushforth commented Jul 7, 2021

openjdk bot commented Jul 7, 2021

kevinrushforth commented Jul 7, 2021

kleopatra commented Jul 8, 2021

Maran23 commented Jul 8, 2021

Maran23 commented Jul 8, 2021

openjdk bot commented Jul 8, 2021

kleopatra commented Jul 9, 2021

Maran23 commented Jul 11, 2021

kleopatra commented Jul 12, 2021

Maran23 commented Jul 22, 2021 •

edited

Loading

kleopatra commented Jul 28, 2021

kleopatra left a comment

kleopatra left a comment •

edited

Loading

kleopatra Aug 19, 2021

Maran23 Sep 19, 2021

kleopatra Sep 26, 2021

kleopatra Sep 28, 2021 •

edited

Loading

kleopatra Aug 19, 2021

Maran23 Sep 19, 2021

kleopatra commented Sep 10, 2021

Maran23 commented Sep 13, 2021

kleopatra left a comment

openjdk bot commented Oct 1, 2021 •

edited

Loading

Maran23 commented Oct 1, 2021

openjdk bot commented Oct 1, 2021

kleopatra commented Oct 1, 2021

openjdk bot commented Oct 1, 2021

openjdk bot commented Oct 1, 2021

8188026: TextFieldXXCell: NPE on calling startEdit #569

8188026: TextFieldXXCell: NPE on calling startEdit #569

Conversation

Maran23 commented Jul 7, 2021 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Jul 7, 2021

mlbridge bot commented Jul 7, 2021 • edited Loading

Webrevs

kevinrushforth commented Jul 7, 2021

openjdk bot commented Jul 7, 2021

kevinrushforth commented Jul 7, 2021

kleopatra commented Jul 8, 2021

Maran23 commented Jul 8, 2021

Maran23 commented Jul 8, 2021

openjdk bot commented Jul 8, 2021

kleopatra commented Jul 9, 2021

Maran23 commented Jul 11, 2021

kleopatra commented Jul 12, 2021

Maran23 commented Jul 22, 2021 • edited Loading

kleopatra commented Jul 28, 2021

kleopatra left a comment

Choose a reason for hiding this comment

kleopatra left a comment • edited Loading

Choose a reason for hiding this comment

kleopatra Aug 19, 2021

Choose a reason for hiding this comment

Maran23 Sep 19, 2021

Choose a reason for hiding this comment

kleopatra Sep 26, 2021

Choose a reason for hiding this comment

kleopatra Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

kleopatra Aug 19, 2021

Choose a reason for hiding this comment

Maran23 Sep 19, 2021

Choose a reason for hiding this comment

kleopatra commented Sep 10, 2021

Maran23 commented Sep 13, 2021

kleopatra left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 1, 2021 • edited Loading

Maran23 commented Oct 1, 2021

openjdk bot commented Oct 1, 2021

kleopatra commented Oct 1, 2021

openjdk bot commented Oct 1, 2021

openjdk bot commented Oct 1, 2021

Maran23 commented Jul 7, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 7, 2021 •

edited

Loading

Maran23 commented Jul 22, 2021 •

edited

Loading

kleopatra left a comment •

edited

Loading

kleopatra Sep 28, 2021 •

edited

Loading

openjdk bot commented Oct 1, 2021 •

edited

Loading