Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8252236: TabPane: must keep header of selected tab visible #300

Closed
wants to merge 3 commits into from
Closed
Changes from 1 commit
Commits
File filter
Filter file types
Jump to
Jump to file
Failed to load files.

Always

Just for now

@@ -851,8 +851,8 @@ public TabHeaderArea() {
ensureSelectedTabIsVisible();
scrollOffsetDirty = false;
} else {
validateScrollOffset();
}
validateScrollOffset();
This conversation was marked as resolved by kevinrushforth
Comment on lines -854 to +855

This comment has been minimized.

@arapte

arapte Sep 23, 2020
Member

This seems to be an unintended change. If so please revert.

This comment has been minimized.

@kleopatra

kleopatra Sep 23, 2020
Author Collaborator

good eye :) Forgot to remove the empty else block.

Validate must be called always if the tabs are not fitting: needed if the last tab (in the list) is visible and we remove tabs from the end. Without, the now last tab is not kept glued to the trailing edge. There are two tests covering the corner case (testRemoveXXLast) which fail if we don't.

Question is if that should be mentioned in a code comment? Personally, I tend to just delete the empty block.

This comment has been minimized.

@arapte

arapte Sep 24, 2020
Member

Thanks, I confirmed that it is a needed change. I think, a comment will be useful here, as the problem that is solved by this change is not very vivid from source.

This comment has been minimized.

@kleopatra

kleopatra Sep 24, 2020
Author Collaborator

added a comment to clarify what the validate is doing

}

Side tabPosition = getSkinnable().getSide();
@@ -986,20 +986,23 @@ private void updateHeaderClip() {
private void addTab(Tab tab, int addToIndex) {
TabHeaderSkin tabHeaderSkin = new TabHeaderSkin(tab);
headersRegion.getChildren().add(addToIndex, tabHeaderSkin);
invalidateScrollOffset();
}

private void removeTab(Tab tab) {
TabHeaderSkin tabHeaderSkin = getTabHeaderSkin(tab);
if (tabHeaderSkin != null) {
headersRegion.getChildren().remove(tabHeaderSkin);
}
invalidateScrollOffset();
}

private void moveTab(int moveToIndex, TabHeaderSkin tabHeaderSkin) {
if (moveToIndex != headersRegion.getChildren().indexOf(tabHeaderSkin)) {
headersRegion.getChildren().remove(tabHeaderSkin);
headersRegion.getChildren().add(moveToIndex, tabHeaderSkin);
}
invalidateScrollOffset();
}

private TabHeaderSkin getTabHeaderSkin(Tab tab) {
@@ -2250,6 +2253,7 @@ private void stopDrag() {
dragHeaderTransitionX = dragHeaderDestX - dragHeaderSourceX;
dragHeaderAnim.playFromStart();
}
tabHeaderArea.invalidateScrollOffset();
}

private void reorderTabs() {
@@ -2308,6 +2312,10 @@ void test_disableAnimations() {
return tabHeaderArea.getScrollOffset();
}

void test_setHeaderAreaScrollOffset(double offset) {
tabHeaderArea.setScrollOffset(offset);
}

boolean test_isTabsFit() {
return tabHeaderArea.tabsFit();
}
@@ -52,10 +52,14 @@ public static double getHeaderAreaScrollOffset(TabPane tabPane) {
return skin.test_getHeaderAreaScrollOffset();
}

public static void setHeaderAreaScrollOffset(TabPane tabPane, double offset) {
TabPaneSkin skin = (TabPaneSkin) tabPane.getSkin();
skin.test_setHeaderAreaScrollOffset(offset);
}

public static boolean isTabsFit(TabPane tabPane) {
TabPaneSkin skin = (TabPaneSkin) tabPane.getSkin();
return skin.test_isTabsFit();
}


}
@@ -25,6 +25,7 @@

package test.javafx.scene.control.skin;

import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
@@ -33,6 +34,7 @@
import org.junit.Before;
import org.junit.Test;

import com.sun.javafx.scene.control.TabObservableList;
import com.sun.javafx.tk.Toolkit;

import static javafx.scene.control.skin.TabPaneSkinShim.*;
@@ -66,6 +68,149 @@
private Pane root;
private TabPane tabPane;

//-------- tests around JDK-8252236

@Test
public void testMoveBySetAll() {
showTabPane();
// select last for max scrolling
int last = tabPane.getTabs().size() - 1;
tabPane.getSelectionModel().select(last);
Tab selectedTab = tabPane.getSelectionModel().getSelectedItem();
Toolkit.getToolkit().firePulse();
// move selected tab to first
List<Tab> tabs = new ArrayList<>(tabPane.getTabs());
tabs.remove(selectedTab);
tabs.add(0, selectedTab);
tabPane.getTabs().setAll(tabs);
Toolkit.getToolkit().firePulse();
assertEquals("scrolled to leading edge", 0, getHeaderAreaScrollOffset(tabPane), 1);
}

/**
* This test passes without the fix, must pass after as well.
*/
@Test
public void testMoveByTabObservableList() {
showTabPane();
// select last for max scrolling
int last = tabPane.getTabs().size() - 1;
tabPane.getSelectionModel().select(last);
Tab selectedTab = tabPane.getSelectionModel().getSelectedItem();
Toolkit.getToolkit().firePulse();
// move selected tab to first
((TabObservableList<Tab>) tabPane.getTabs()).reorder(selectedTab, tabPane.getTabs().get(0));
Toolkit.getToolkit().firePulse();
assertEquals("scrolled to leading edge", 0, getHeaderAreaScrollOffset(tabPane), 1);
}

/**
* Scroll to last (by selecting it) -> remove last.
*
* Without fix, fails by not scrolling at all: the gap is increasing every time the
* last selected (after removal that's the previous) is removed.
*
*/
@Test
public void testRemoveSelectedAsLast() {
showTabPane();
int last = tabPane.getTabs().size() - 1;
Tab secondLastTab = tabPane.getTabs().get(last - 1);
Tab lastTab = tabPane.getTabs().get(last);
// select for max scroll
tabPane.getSelectionModel().select(last);
Toolkit.getToolkit().firePulse();

// at this point, the header is scrolled such that the last is at the very right
double scrollOffset = getHeaderAreaScrollOffset(tabPane);
double lastTabOffset = getTabHeaderOffset(tabPane, lastTab);
double secondLastTabOffset = getTabHeaderOffset(tabPane, secondLastTab);
// expected change in scroll offset
double expectedDelta = lastTabOffset - secondLastTabOffset;

// remove last (== selected)
tabPane.getTabs().remove(last);
Toolkit.getToolkit().firePulse();

assertEquals("scrollOffset adjusted: ", scrollOffset + expectedDelta, getHeaderAreaScrollOffset(tabPane), 1);
}

/**
* Scroll to last (by selecting it) -> select previous -> remove last.
*
* This test passes without the fix, must pass after as well.
*/
@Test
public void testRemoveLastIfSelectedIsSecondLast() {
showTabPane();
int last = tabPane.getTabs().size() - 1;
Tab lastTab = tabPane.getTabs().get(last);
int secondLast = last - 1;
Tab secondLastTab = tabPane.getTabs().get(secondLast);

// select for max scroll
tabPane.getSelectionModel().select(last);
Toolkit.getToolkit().firePulse();

// at this point, the header is scrolled such that the last is at the very right
double scrollOffset = getHeaderAreaScrollOffset(tabPane);
double lastTabOffest = getTabHeaderOffset(tabPane, lastTab);
double secondeLastTabOffset = getTabHeaderOffset(tabPane, secondLastTab);
// expected change in scroll offset
double expectedDelta = lastTabOffest - secondeLastTabOffset;

// select previous tab
tabPane.getSelectionModel().select(secondLast);
Toolkit.getToolkit().firePulse();

// remove last
tabPane.getTabs().remove(last);
Toolkit.getToolkit().firePulse();

assertEquals("scrollOffset adjusted: ", scrollOffset + expectedDelta, getHeaderAreaScrollOffset(tabPane), 1);
}

@Test
public void testRemoveBefore() {
showTabPane();
int selected = 4;
tabPane.getSelectionModel().select(selected);
Tab selectedTab = tabPane.getSelectionModel().getSelectedItem();
Toolkit.getToolkit().firePulse();
// state before tabs modification
double selectedTabOffset = getTabHeaderOffset(tabPane, selectedTab);
double scrollOffset = getHeaderAreaScrollOffset(tabPane);
assertEquals("sanity: tab visible but not scrolled", 0, scrollOffset, 1);

// scroll selected to leading edge
setHeaderAreaScrollOffset(tabPane, - selectedTabOffset);
Toolkit.getToolkit().firePulse();
assertEquals("sanity: really scrolled", - selectedTabOffset, getHeaderAreaScrollOffset(tabPane), 1);
tabPane.getTabs().remove(0);
Toolkit.getToolkit().firePulse();
assertEquals("scroll offset", - getTabHeaderOffset(tabPane, selectedTab), getHeaderAreaScrollOffset(tabPane), 1);
}

@Test
public void testAddBefore() {
showTabPane();
int last = tabPane.getTabs().size() - 1;
tabPane.getSelectionModel().select(last);
Tab selectedTab = tabPane.getSelectionModel().getSelectedItem();
Toolkit.getToolkit().firePulse();
// state before tabs modification
double selectedTabOffset = getTabHeaderOffset(tabPane, selectedTab);
double scrollOffset = getHeaderAreaScrollOffset(tabPane);

Tab added = new Tab("added", new Label("added"));
tabPane.getTabs().add(0, added);
Toolkit.getToolkit().firePulse();
Node addedHeader = getTabHeaderFor(tabPane, added);
double addedWidth = addedHeader.prefWidth(-1);
assertEquals("sanity", selectedTabOffset + addedWidth, getTabHeaderOffset(tabPane, selectedTab), 1);
assertEquals("scroll offset", scrollOffset - addedWidth, getHeaderAreaScrollOffset(tabPane), 1);
}

/**
* Test scroll on changing tabPane width.
*/
ProTip! Use n and p to navigate between commits in a pull request.