On Mon, 5 Jul 2021 06:35:24 GMT, Ajit Ghaisas <[email protected]> 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
