TableCell: how to find the origin of fishy code?

[Jeanette Winzenburg]

forgot: the null check for table might have been introduced at some  
time due to (arguably illegal, at least unspecified, see  
https://bugs.openjdk.java.net/browse/JDK-8269871) precondition of  
CellEditEvent requiring a not-null table. If we decide to keep that  
requirement (and solve the issue by documenting it), we have to check  
for both in all edit methods

-- Jeanette


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

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