On Wed, 29 Apr 2020 04:40:59 GMT, Ambarish Rapte <ara...@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
