RFR: 8187229: Tree/TableCell: cancel event must return correct editing location

[Marius Hanl]

On Thu, 1 Jul 2021 12:38:06 GMT, Jeanette Winzenburg <fastegal at 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.

Fix looks good, I just saw the concept is the same as in your previous PR: https://github.com/openjdk/jfx/pull/539

Verified tests are failing before and passing now. (and as you mnetioned some pass before and pass now)

I left some inline comments, mostly minor and my opinion, so it's up to you. :)

modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java line 434:

> 432:     }
> 433: 
> 434: 

Very minor: 2 new lines

modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java line 456:

> 454:      */
> 455:     @Test
> 456:     public void testEditCancelEventAfterCancelOnTable() {

Minor: I think one more sentence about what to expect here would be good (so what is tested against here). Can be copied to all similar tests as well.

modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java line 530:

> 528:      */
> 529:     @Test
> 530:     public void testEditCancelMemoryLeakAfterRemoveEditingItem() {

I took a while to understand this test. 
Maybe one more sentence with information would be helpful here (and same test in TreeTableCell).

(My first thought was we are testing basically WeakReference, but then I figured out we want to test whether there are some references left somewhere which prevents the GC or not. So a small sentence like this would be nice in my opinion :))

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableCellTest.java line 805:

> 803: 
> 804:     @Test
> 805:     public void testEditCancelEventAfterModifyItems() {

Does it make sense to have the same javadoc here as the same test in TableCellTest has?

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

Marked as reviewed by mhanl (Author).

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