RFR: 8237602: TabPane doesn't respect order of TabPane.getTabs() list

[Kevin Rushforth]

On Wed, 29 Apr 2020 04:40:59 GMT, Ambarish Rapte <arapte at openjdk.org> wrote:

> Issue:
> When tabs are permuted as mentioned in the issue description as,
> 1. TabPane.getTabs().setAll(tab0, tab1)
> 2. TabPane.getTabs().setAll(tab0, tab1, tab2, tab3);
> the tab headers do not get permuted in same order as `TabPane.getTabs()`.
> 
> => tab headers should be shown in order as  tab0, tab1, tab2, tab3.
> => but are show in order as  tab2, tab3, tab0, tab1
> 
> Cause:
> Newly added tabs(tab2, tab3) are not inserted at correct index. The index `Change.getFrom()` (0) used from Change does
> not remain valid after the tabs to be moved(tab0, tab1) are removed from `tabsToAdd` list.
> Fix:
> Use the index of first newly added tab, from `TabPane.getTabs()`, which would be always reliable.
> 
> Verification:
> No existing tests fail due to this change.
> Added a system test which fails without and pass with fix.

I think the change is fine, although I still need to run it. I have a couple questions / comments.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java line 649:

> 648:                 if (!tabsToAdd.isEmpty()) {
> 649:                     addTabs(tabsToAdd, getSkinnable().getTabs().indexOf(tabsToAdd.get(0)));
> 650:                 }

@kleopatra is right in that there is no guarantee that the set of added sublists from multiple Changes are contiguous.
Since this assumption is preexisting, I agree that this is just something to keep in mind.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java line 994:

> 993:         private void moveTab(int moveToIndex, TabHeaderSkin tabHeaderSkin) {
> 994:             if (moveToIndex != headersRegion.getChildren().indexOf(tabHeaderSkin)) {
> 995:                 headersRegion.getChildren().remove(tabHeaderSkin);

Unless I am missing something, this check seems unrelated to the bug.

tests/system/src/test/java/test/robot/javafx/scene/TabPanePermuteGetTabsTest.java line 194:

> 193:     @Test
> 194:     public void testPermutGetTabsWithMoreTabs1() {
> 195:         // Step #1

Typo: Permut --> Permute

(or, as long as you are changing it anyway, you could rename it now to something other than "permute" since it isn't).

tests/system/src/test/java/test/robot/javafx/scene/TabPanePermuteGetTabsTest.java line 224:

> 223:         });
> 224:         delay();
> 225:

Does using Robot to select the Tab test add value? Probably not worth worrying about given that this will all move to
the controls unit tests with [JDK-8244195](https://bugs.openjdk.java.net/browse/JDK-8244195).

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

PR: https://git.openjdk.java.net/jfx/pull/201