RFR: 8188026: TextFieldXXCell: NPE on calling startEdit

[Marius Hanl]

On Wed, 7 Jul 2021 22:33:07 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

> 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?

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

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