TableCell: how to find the origin of fishy code?

[Jeanette Winzenburg]

fyi: the inconsistently must be resolved (back) to checking for column  
!= null in all edit methods. The reason is that the event is fired  
onto the column as target, which must be not null. Can be seen in the  
test below (concededly very artificial):

     @Test
     public void testEditCancelNoColumn() {
         table.setEditable(true);
         cell.updateTableView(table);
         // force artificially not-empty cell state
         TableCellShim.set_lockItemOnEdit(cell, true);
         CellShim.updateItem(cell, "something", false);
         // start edit: succeeds without firing event (null check  
against column)
         cell.startEdit();
         assertTrue(cell.isEditing());
         // cancel edit: NPE from Event.fire - which must (all params  
must be != null)
         cell.cancelEdit();
         assertFalse(cell.isEditing());
     }

will file an issue.

Thanks for your feedback :)

Zitat von Jeanette Winzenburg <fastegal at swingempire.de>:

> Hi Jonathan,
>
> thanks for quick reply :) Yeah, agree that inconsistency looks  
> unintentional - leaving the question which way to go.
>
> Meanwhile I found the commit that changed the logic for cancel:
>
> since RT-18513 (https://bugs.openjdk.java.net/browse/JDK-8116392 -  
> ListView stackoverflow) it is similar as today:
>
>     super.cancel();
>     if (table != null) {
>        table.edit(-1, null);
>        fireEvent
>
> before it was:
>
>     it (getTableColumn() != null) {
>        fireEvent
>     }
>     super.cancel();
>     if (table != null) table.edit(-1, null)
>
> the older feels like the way to go, what do you think?
>
> Anyway, will try to find tests to detect side-effects of the one or other.
>
> Thanks again, Jeanette
>
> Zitat von Jonathan Giles <jonathan at jonathangiles.net>:
>
>> Seeing as I wrote the code, I wish I could remember why it is that way, but
>> I unfortunately can't. My initial instinct is that this was unintentional,
>> but I can't discount the possibility that a reason exists.
>>
>> I probably have much earlier versions of the code lying around, so I could
>> research it if necessary, but I'd suggest making it consistent first and
>> seeing if anything jumps out at you.
>>
>> -- Jonathan
>>
>> On Tue, 6 Jul 2021, 9:15 pm Jeanette Winzenburg, <fastegal at swingempire.de>
>> wrote:
>>
>>>
>>> Just noticed an inconsistency in event firing pattern in tableCell's
>>> xxEdit methods:
>>>
>>> the pattern in cancel/commit:
>>>
>>>     if (table != null) {
>>>         // create and fire the event
>>>
>>> in start:
>>>
>>>     if (column != null) {
>>>        // create and fire the event
>>>
>>> usual question: bug or feature? Could there be any reason for the
>>> difference? As I can't think of any, so the usual tracking into
>>> history: it was there since the beginning of time, earliest in git is
>>>
>>>   e89e55e07972ce208ed5f89d091946392dc98114 add javafx-ui-control
>>> classes 2011-11-10
>>>
>>> which seems to be the same as master 2.1
>>> (
>>> http://hg.openjdk.java.net/openjfx/2.1/master/rt/file/b7d368850c33/javafx-ui-controls/src/javafx/scene/control/TableCell.java)
>>> in old
>>> mercurial
>>>
>>> Any earlier versions that are accessible, if so where?
>>>
>>>