On Mon, 26 Apr 2021 11:46:57 GMT, Jeanette Winzenburg <[email protected]>
wrote:
>> Florian Kirmaier has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8264127:
>> Added missing test case
>
> 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.
I've now rewritten the code. It's now way simpler and avoids duplicate code. I
guess the different implementation in the different cells all have some subtle
differences.
Then i guess we wait for the other review to finish. The change to use try
finalize seems very very reasonable and important to me.
-------------
PR: https://git.openjdk.java.net/jfx/pull/441
