Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
8277785: ListView scrollTo jumps to wrong location when CellHeight is…
… changed

Reviewed-by: kcr, fkirmaier, aghaisas
  • Loading branch information
Johan Vos committed Jul 14, 2022
1 parent eb8e9ef commit b8302f6
Show file tree
Hide file tree
Showing 4 changed files with 353 additions and 40 deletions.
Expand Up @@ -918,15 +918,13 @@ public final void setCellCount(int value) {

@Override protected void invalidated() {
super.invalidated();
adjustAbsoluteOffset();
requestLayout();
}
};
public final double getPosition() { return position.get(); }
public final void setPosition(double value) {
position.set(value);
// When the position is changed explicitly, we need to make sure
// the absolute offset is changed accordingly.
adjustAbsoluteOffset();
}
public final DoubleProperty positionProperty() { return position; }

Expand Down Expand Up @@ -1052,27 +1050,18 @@ void adjustPosition() {

/** {@inheritDoc} */
@Override protected void layoutChildren() {
double origAbsoluteOffset = absoluteOffset;
recalculateEstimatedSize();
// if the last modification to the position was done via scrollPixels,
// the absoluteOffset and position are already in sync.
// However, the position can be modified via different ways (e.g. by
// moving the scrollbar thumb), so we need to recalculate absoluteOffset
// There is an exception to this: if cells are added/removed, we want
// to keep the absoluteOffset constant, hence we need to adjust the position.

if (lastCellCount != getCellCount()) {
absoluteOffset = origAbsoluteOffset;
adjustPosition();
} else {
adjustAbsoluteOffset();
}
// when we enter this method, the absoluteOffset and position are
// already determined. In case this method invokes other methods
// that may change either absoluteOffset or position, it is the
// responsability of those methods to make sure both parameters are
// changed in a consistent way.
// For example, the recalculateEstimatedSize method also recalculates
// the absoluteOffset and position.

if (needsRecreateCells) {
lastWidth = -1;
lastHeight = -1;
releaseCell(accumCell);
// accumCell = null;
// accumCellParent.getChildren().clear();
sheet.getChildren().clear();
for (int i = 0, max = cells.size(); i < max; i++) {
cells.get(i).updateIndex(-1);
Expand Down Expand Up @@ -1334,6 +1323,7 @@ void adjustPosition() {
}
computeBarVisiblity();

recalculateAndImproveEstimatedSize(0);
recreatedOrRebuilt = recreatedOrRebuilt || rebuild;
updateScrollBarsAndCells(recreatedOrRebuilt);

Expand Down Expand Up @@ -1579,6 +1569,8 @@ private boolean tryScrollOneCell(int targetIndex, boolean downOrRight) {
* @param index the index
*/
public void scrollToTop(int index) {
// make sure we have the real size of cells that are likely to be rendered
getCellSizesInExpectedViewport(index);
boolean posSet = false;

if (index > getCellCount() - 1) {
Expand Down Expand Up @@ -1764,6 +1756,7 @@ public T getCell(int index) {
// to a severe performance decrease. This seems to be OK, as
// getCell() is only used for cell measurement purposes.
// pile.remove(i);
resizeCell(cell);
return cell;
}
}
Expand Down Expand Up @@ -1901,8 +1894,12 @@ private final double getViewportBreadth() {
*/
private double viewportLength;
void setViewportLength(double value) {
if (value == this.viewportLength) {
return;
}
this.viewportLength = value;
this.absoluteOffset = getPosition() * (estimatedSize - viewportLength);
recalculateEstimatedSize();
}
double getViewportLength() {
return viewportLength;
Expand Down Expand Up @@ -1996,6 +1993,8 @@ protected void resizeCell(T cell) {
double height = Math.max(getMaxPrefBreadth(), getViewportBreadth());
cell.resize(fixedCellSizeEnabled ? getFixedCellSize() : Utils.boundedSize(cell.prefWidth(height), cell.minWidth(height), cell.maxWidth(height)), height);
}
// when a cell is resized, our estimate needs to be updated.
recalculateAndImproveEstimatedSize(0);
}

/**
Expand Down Expand Up @@ -2311,6 +2310,28 @@ void setCellDirty(int index) {
requestLayout();
}

/**
* Make sure the sizes of the cells that are likely to be visible are known.
* When updates to the cell size estimates are occurring, we don't want the current
* visible content to be modified.
* @param index the index of the cell that should be positioned at the top of
* the viewport in the next layout cycle.
*/
void getCellSizesInExpectedViewport(int index) {
double estlength = getOrCreateCellSize(index);
int i = index;
while ((estlength < viewportLength) && (++i < getCellCount())) {
estlength = estlength + getOrCreateCellSize(i);
}
if (estlength < viewportLength) {
int j = index;
while ((estlength < viewportLength) && (j-- > 0)) {
estlength = estlength + getOrCreateCellSize(j);
}
}
recalculateEstimatedSize();
}

private void startSBReleasedAnimation() {
if (sbTouchTimeline == null) {
/*
Expand Down Expand Up @@ -2442,6 +2463,26 @@ private void initViewport() {
lengthBar.setVirtual(true);
}

/**
* In case we are not rendering the first cell
* AND
* there is empty room after the last cell,
* the cells need to be shifted down to fill the empty area.
*/
private void shiftDown() {
T lastNonEmptyCell = getLastVisibleCell();
T firstCell = cells.getFirst();
int index = getCellIndex(firstCell);
double end = getCellPosition(lastNonEmptyCell) + getCellLength(lastNonEmptyCell);
double delta = viewportLength - end;
if ((index > 0) && (delta > 0)) {
for (int i = 0; i < cells.size(); i++) {
T cell = cells.get(i);
positionCell(cell, getCellPosition(cell) + delta);
}
}
}

private void updateScrollBarsAndCells(boolean recreate) {
// Assign the hbar and vbar to the breadthBar and lengthBar so as
// to make some subsequent calculations easier.
Expand Down Expand Up @@ -2486,6 +2527,7 @@ private void updateScrollBarsAndCells(boolean recreate) {

offset += getCellLength(cell);
}
shiftDown();
}

// Toggle visibility on the corner
Expand Down Expand Up @@ -2848,6 +2890,9 @@ private double computeViewportOffset(double position) {
}

private void adjustPositionToIndex(int index) {
if (index > 0) getOrCreateCellSize(index-1);
getOrCreateCellSize(index);
recalculateEstimatedSize();
int cellCount = getCellCount();
if (cellCount <= 0) {
setPosition(0.0f);
Expand All @@ -2859,7 +2904,7 @@ private void adjustPositionToIndex(int index) {
if (cz < 0) cz = estSize;
targetOffset = targetOffset+ cz;
}
this.absoluteOffset = targetOffset;
this.absoluteOffset = (estimatedSize < viewportLength) ? 0 : targetOffset;
adjustPosition();
}

Expand Down Expand Up @@ -2954,6 +2999,7 @@ private double computeOffsetForCell(int itemIndex) {
}

private double getOrCreateCellSize (int idx, boolean create) {
if (idx < 0) return -1;
// is the current cache long enough to contain idx?
if (itemSizeCache.size() > idx) {
// is there a non-null value stored in the cache?
Expand All @@ -2964,28 +3010,34 @@ private double getOrCreateCellSize (int idx, boolean create) {
if (!create) return -1;
boolean doRelease = false;

// Do we have a visible cell for this index?
T cell = getVisibleCell(idx);
if (cell == null) { // we might get the accumcell here
cell = getCell(idx);
doRelease = true;
}
// Make sure we have enough space in the cache to store this index
while (idx >= itemSizeCache.size()) {
itemSizeCache.add(itemSizeCache.size(), null);
}

// if we have a valid cell, we can populate the cache
double answer = 1d;
if (isVertical()) {
answer = cell.getLayoutBounds().getHeight();
if (getFixedCellSize() > 0) {
answer = getFixedCellSize();
itemSizeCache.set(idx, answer);
} else {
answer = cell.getLayoutBounds().getWidth();
}
itemSizeCache.set(idx, answer);
// Do we have a visible cell for this index?
T cell = getVisibleCell(idx);
if (cell == null) { // we might get the accumcell here
cell = getCell(idx);
doRelease = true;
}

if (doRelease) { // we need to release the accumcell
releaseCell(cell);
// if we have a valid cell, we can populate the cache
if (isVertical()) {
answer = cell.getLayoutBounds().getHeight();
} else {
answer = cell.getLayoutBounds().getWidth();
}
itemSizeCache.set(idx, answer);

if (doRelease) { // we need to release the accumcell
releaseCell(cell);
}
}
return answer;
}
Expand Down Expand Up @@ -3020,6 +3072,11 @@ private void recalculateEstimatedSize() {
private void recalculateAndImproveEstimatedSize(int improve) {
int itemCount = getCellCount();
int cacheCount = itemSizeCache.size();
boolean keepRatio = ((cacheCount > 0) && !Double.isInfinite(this.absoluteOffset));
double estSize = estimatedSize / itemCount;

int oldIndex = computeCurrentIndex();
double oldOffset = computeViewportOffset(getPosition());
int added = 0;
while ((itemCount > itemSizeCache.size()) && (added < improve)) {
getOrCreateCellSize(itemSizeCache.size());
Expand All @@ -3035,7 +3092,22 @@ private void recalculateAndImproveEstimatedSize(int improve) {
cnt++;
}
}
this.estimatedSize = cnt == 0 ? 1d: tot * itemCount / cnt;
this.estimatedSize = cnt == 0 ? 1d : tot * itemCount / cnt;
estSize = estimatedSize / itemCount;

if (keepRatio) {
double newOffset = 0;
for (int i = 0; i < oldIndex; i++) {
double h = getCellSize(i);
if (h < 0) {
h = estSize;
}
newOffset += h;
}
this.absoluteOffset = newOffset + oldOffset;
adjustPosition();
}

}

private void resetSizeEstimates() {
Expand Down
Expand Up @@ -1834,6 +1834,7 @@ private boolean isAnchor(int index) {
sl.dispose();
}

@Ignore("JDK-8289909") // there is no guarantee that there will be 8 selected items (can be 7 as well)
@Test public void test_rt34407_up_up_down() {
final int items = 100;
listView.getItems().clear();
Expand Down

1 comment on commit b8302f6

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.