On Wed, 3 Jun 2026 17:29:55 GMT, Marius Hanl <[email protected]> wrote:

>> This is a much simpler fix for JDK-8384806 which does not have any side 
>> effects.
>> 
>> This restores the old code with only a one line change instead. For 
>> reference, see 
>> https://github.com/openjdk/jfx/commit/8d917ae738120e12ac12cd0957879b7c00e59b03.
>>  We now fix the issue by clearing the cell with 
>> `buttonCell.updateIndex(-1);` as using `buttonCell.setItem(null);` was 
>> causing the original issue when the item was already null.
>> 
>> ---------
>> - [x] I confirm that I make this contribution in accordance with the 
>> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai).
>
> Can we instead do something like this in `ComboBoxListViewSkin`:
> 
> 
> ...
>         lh.addChangeListener(control.converterProperty(), e -> {
>             listView.refresh();
>             buttonCell.updateIndex(-1); // <--- NEW
>             updateDisplayNode();
>         });
> ...
> 
> 
> This makes more sense to me. I understand the problem and your reasoning. 
> If the converter changes, we want to refresh the `ListView`. But we should 
> also force the `buttonCell` to refresh, hence we should reset to -1. 
> 
> Then you can revert `updateDisplayNode` to not call 
> `buttonCell.updateIndex(-1);`. All tests still pass for me and the fix makes 
> more sense - in this case the `buttonCell` is 100% stale and we need to 
> consider that.

@Maran23 For reviewing, this PR is essential a revert of 
https://github.com/openjdk/jfx/commit/8d917ae738120e12ac12cd0957879b7c00e59b03 
and a redo of the fix but simpler. I want to keep it minimal now as I now know 
how sensitive the combo box is to any changes, especially for custom button 
cells. I think the same bug will occur when you change the prompt text 
(https://github.com/openjdk/jfx/blob/7378d4079910653763626ce6e707b00adbec1459/modules/javafx.controls/src/main/java/javafx/scene/control/skin/ComboBoxListViewSkin.java#L175),
 so just adding the updateIndex() call in one listener is only a partial fix.

@hjohn I agree, the whole skin is a hack. I got burned by changing too much, 
that is why this issue exists in the first place. It would be easier to just 
create a `ComboBoxListViewSkinV2` for all the larger issues.

-------------

PR Comment: https://git.openjdk.org/jfx/pull/2179#issuecomment-4618296278

Reply via email to