On Fri, 3 Apr 2020 12:03:33 GMT, Jeanette Winzenburg <faste...@openjdk.org>
wrote:
>>> Can you please provide a unit test? One that fails before your fix and
>>> passes after your fix.
>>
>> I can provide a manual test the next couple of days that demonstrates it
>> before and after, but I'm not sure how to
>> create an automated unit test because the issue is visual.
>
> A quick snippet of how-to write a unit test (for setup see other tests in
> controls) that will fail before and pass
> after the change:
> @Test
> public void testScrollInSkin() {
> int index = 50;
> comboBox.getSelectionModel().select(index);
> comboBox.show();
> VirtualFlow<IndexedCell<?>> virtualFlow = getFlow();
> int first = virtualFlow.getFirstVisibleCell().getIndex();
> int last = virtualFlow.getLastVisibleCell().getIndex();
> assertTrue(" visible range [" + first + ", " + last + "] must include
> " + index,
> first <= index && index <= last);
> }
>
> protected VirtualFlow<IndexedCell<?>> getFlow() {
> VirtualFlow<IndexedCell<?>> virtualFlow =
> (VirtualFlow<IndexedCell<?>>)
> VirtualFlowTestUtils.getVirtualFlow(comboBox);
> return virtualFlow;
> }
A couple of comments to the bug (and fix) itself:
- is it really a bug or a nice-to-have enhancement? couldn't find an example in
win, didn't try too hard and nowadays,
such plain combos are not a really frequent
- while none of the virtualized controls (nor ChoiceBox) combines selection
with scrolling to the selected item. For
combo and choice, I see no reason not make it the default behavior. We need
to make certain it behaves "naturally" when
navigating in the open popup. instead of catching every list.select (and not
forget the unselect) we might consider
doing it in a showing handler alternatively, we might consider to go deeper
and support it directly in the listView
-------------
PR: https://git.openjdk.java.net/jfx/pull/136
