RFR: 8264127: ListCell editing status is true, when index changes while editing [v8]
Florian Kirmaier
fkirmaier at openjdk.java.net
Tue Apr 27 10:03:42 UTC 2021
On Mon, 26 Apr 2021 11:46:57 GMT, Jeanette Winzenburg <fastegal at openjdk.org> 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
More information about the openjfx-dev
mailing list