On Thu, 1 Jul 2021 12:38:06 GMT, Jeanette Winzenburg <faste...@openjdk.org> 
wrote:

> the bug is an incorrect edit location (for Tree/Table: Tree/TablePosition) in 
> edit cancel events - expected is the location at the time the cell edit was 
> started, actual was the location of at the time the edit was cancelled. See 
> the report for details.
> 
> The fix is analogue to those for ListCell/TreeCell, that is storing the edit 
> location in startEdit and use that in cancelEdit.
> 
> Added tests that failed before and passed after and tests that (accidentally 
> :) passed before and still pass after.
> 
> Related issues:
> 
> - also fixes [JDK-8269136](https://bugs.openjdk.java.net/browse/JDK-8269136) 
> (Tree/TablePosition: must not throw NPE on instantiating with null table), in 
> hind-sight it seemed too small to warrant its own PR. 
> - does not fix the implementation of CellEditEvent (todo: file follow-up 
> issue) which must not throw with null Tree/TablePosition.

The fix is fine.
I have few suggestions-
1) Please add JDK-8269136 to this PR using "/issue add JDK-8269136" command
2) We do not have a precedent of adding comments to the tests indicating 
pass/fail before/after a bug. It is OK if some of them pass without this fix as 
well. Suggest to remove these type of comments from newly added tests.
3) Please file a follow on bug and update the todo: in PR description.

Something for the future :
I see that the fix of JDK-8269136 has its own separate code changes and tests. 
I see that you have combined it with this PR only due to the fact that the 
changes are smaller. I personally prefer NOT combining unrelated fixes. This 
ensures a better cohesion of the bug fixes. Added advantage is reverting a fix 
is easier in case of future regressions.

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

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

Reply via email to