RFR: 8265206: Tree-/TableCell: editing state not updated on cell re-use

[Johan Vos]

On Thu, 22 Apr 2021 10:40:15 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> I see the updateEditingIndex is used as a hacky flag as the comment says, and `table.edit(-1, null)` is called conditionally.
>> But my question is about the pre- and postconditions. What would happen if a subclass overrides `cancelEdit` and throws an exception, so that `updateEditingIndex = true` is not called after the `cancelEdit`? In that case, `updateEditingIndex` stays false.
>> I'm not saying this is an issue, just wondering about this as this flag does an important thing -- and I do know this was in the code before the PR, so the PR is definitely an improvement, I'm just thinking that this might be an opportunity to deal with the precondition that updateEditingIndex *should* be true when entering the method and false when leaving the method.
>
> thanks for the explanation and spelling it out to me :)
> 
>> But my question is about the pre- and postconditions. What would happen if a subclass overrides `cancelEdit` and throws an exception, so that `updateEditingIndex = true` is not called after the `cancelEdit`? In that case, `updateEditingIndex` stays false.
> 
> talking about pre/postCondiditions: wouldn't an override of cancelEdit (and the other editing life-cycle methods) that's throwing an exception violate its contract? Don't see any constraints in its spec at Cell, so I would say it must do it's very best not to throw anything, at least not intentionally. And if it happens somewhere along the chain, f.i.  an edit handler going wild, then it would be the responsibility of that handler to behave (aka: usage error except for the little bugs in the editEvents .. ). 
> 
>> I'm not saying this is an issue, just wondering about this as this flag does an important thing -- and I do know this was in the code before the PR, so the PR is definitely an improvement, I'm just thinking that this might be an opportunity to deal with the precondition that updateEditingIndex _should_ be true when entering the method and false when leaving the method.
> 
> hmm .. again not quite sure I understand: shouldn't updateEditingIndex be true both on entering and leaving doCancelEdit?. Anyway, if there's something going wrong in cancelEdit, the cell is in an arbitrary state: could still be editing (if the wrong-going happened before calling cell.cancelEdit) or not, could have fired the cancel event or not, could have updated its visuals or not .. there's not much we can do to recover from it. When going from this undeterminate state into the next editing cycle the outcome is .. unpredictable. 
> 
> All that said: we could wrap the flag un-/setting into a try-finally block - then at least the whacky flag is in a specific state after all hell broke loose ..

I agree a try-finally block here (setting the flag in a specific state) might be useful -- I prefer to keep unpredictable states for quantum computing :)

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

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