On Fri, 23 Apr 2021 17:06:54 GMT, Florian Kirmaier <fkirma...@openjdk.org> wrote:
>> Fixing ListCell editing status is true, when index changes while editing. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > 8264127: > Added missing test case fix and tests are very near ready, good work :) - fix has some details to finalize pending [review](https://github.com/openjdk/jfx/pull/473#discussion_r617762020) for the exact same in TableCell - test missing one last assertion (about list editing state) left inline comments modules/javafx.controls/src/main/java/javafx/scene/control/ListCell.java line 550: > 548: updateEditingIndex = true; > 549: } > 550: } else { exactly :) And now we have duplicated code blocks in a single method which can be improved by extracting them. It's a bit a matter of judgement/style of the codebase: in this particular case we probably should because - it's the same across all cell implementations - a slight issue turned up in [review](https://github.com/openjdk/jfx/pull/473#discussion_r617762020) of the exact same code in TableCell (should guarantee that the flag is reliably reset to true, no matter what happens in cancelEdit) - fixing that increases the length of the duplicated block. Whatever the final outcome, it should be consistent across cells. We might wait with this until the TableCell is finalized, then do the same here - what do you think? Unrelated to extracting or not: the sequence should have the same code comment everywhere it appears. modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellTest.java line 921: > 919: assertFalse("cell must not be editing if cell index is " + > cell.getIndex(), cell.isEditing()); > 920: assertEquals(1, events.size()); > 921: } still missing: assert that list editingIndex is unchanged (see the assertTo test). Without that assert, you don't see any difference in the test between your previous and this commit (which was surrounding the call to cancelEdit with setting the updateEditingIndex flags), do you ;) A personal note: tests are our friends, red-green (repeatedly) the color sequence that brings satisfaction :) Just changing a code snippet without making certain to have a failing test before (and passes after) a change is .. well .. suboptimal. ------------- Changes requested by fastegal (Committer). PR: https://git.openjdk.java.net/jfx/pull/441
