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

Jeanette Winzenburg fastegal at openjdk.java.net
Mon Jul 5 10:32:49 UTC 2021


On Mon, 5 Jul 2021 06:35:24 GMT, Ajit Ghaisas <aghaisas at openjdk.org> wrote:

> 
> 
> The fix is fine.
> I have few suggestions-
> 
>     1. Please add JDK-8269136 to this PR using "/issue add JDK-8269136" command
> 

done

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

argg .. you are right, sloppy me left more of them than I intended to (my locale branches are a bit crowded with comments ;) - aligned with TreeCellTest, which meant to remove most

> 
>     3. Please file a follow on bug and update the todo: in PR description.
> 

will do

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

Fully agree with not co-fixing unrelated issues - just: I found it a bit hard to really separate them because they are not as unrelated as it looks: 

- fixing this introduces failing tests in CellTest without also fixing JDK-8269136 (because it seems to be allowed to switch a cell into editing state without it being attached to a control) 
- fixing JDK-8269136 without context (just the plain NPE) has no obvious relation to editing 

Hmm ... probably should update the description of this PR to explain that mix.

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

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


More information about the openjfx-dev mailing list