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