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.

Sign up for GitHub

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

8185887: TableRowSkinBase fails to correctly virtualize cells in horizontal direction #1644

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

8185887: TableRowSkinBase fails to correctly virtualize cells in horizontal direction #1644

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e811572
8185887: TableRowSkinBase fails to correctly virtualize cells in hori…
Maran23 Aug 11, 2024
680161f
8185887: Reset disclosureNodeDirty in updateDisclosureNodeAndGraphic()
Maran23 Dec 9, 2024
b6b38e4
Merge branch 'master' of https://github.com/openjdk/jfx into 8185887-…
Maran23 Dec 9, 2024
5a08985
Merge branch 'master' of https://github.com/openjdk/jfx into 8185887-…
Maran23 Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 21 additions & 14 deletions modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,22 +105,29 @@ private void setupTreeTableViewListeners() {
setupTreeTableViewListeners();
});
} else {
DoubleProperty fixedCellSizeProperty = tableView.fixedCellSizeProperty();
if (fixedCellSizeProperty != null) {
registerChangeListener(fixedCellSizeProperty, e -> {
fixedCellSize = fixedCellSizeProperty.get();
fixedCellSizeEnabled = fixedCellSize > 0;
});
fixedCellSize = fixedCellSizeProperty.get();
fixedCellSizeEnabled = fixedCellSize > 0;

// JDK-8144500:
// When in fixed cell size mode, we must listen to the width of the virtual flow, so
// that when it changes, we can appropriately add / remove cells that may or may not
// be required (because we remove all cells that are not visible).
registerChangeListener(tableView.fixedCellSizeProperty(), e -> {
VirtualFlow<TableRow<T>> virtualFlow = getVirtualFlow();
if (virtualFlow != null) {
registerChangeListener(virtualFlow.widthProperty(), e -> tableView.requestLayout());
unregisterChangeListeners(virtualFlow.widthProperty());
}

updateCachedFixedSize();
});

updateCachedFixedSize();
}
}

private void updateCachedFixedSize() {
TableView<T> tableView = getSkinnable().getTableView();
if (tableView != null) {
fixedCellSize = tableView.getFixedCellSize();
fixedCellSizeEnabled = fixedCellSize > 0;

if (fixedCellSizeEnabled) {
VirtualFlow<TableRow<T>> virtualFlow = getVirtualFlow();
if (virtualFlow != null) {
registerChangeListener(virtualFlow.widthProperty(), e -> getSkinnable().requestLayout());
}
}
}
Expand Down
99 changes: 16 additions & 83 deletions modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
import javafx.collections.ObservableList;
import javafx.css.StyleOrigin;
import javafx.css.StyleableObjectProperty;
import javafx.geometry.Insets;
import javafx.geometry.Pos;
import javafx.scene.Node;
import javafx.scene.Parent;
Expand Down Expand Up @@ -95,11 +94,6 @@ public abstract class TableRowSkinBase<T,
*/
static final Map<TableColumnBase<?,?>, Double> maxDisclosureWidthMap = new WeakHashMap<>();

// Specifies the number of times we will call 'recreateCells()' before we blow
// out the cellsMap structure and rebuild all cells. This helps to prevent
// against memory leaks in certain extreme circumstances.
private static final int DEFAULT_FULL_REFRESH_COUNTER = 100;


/* *************************************************************************
* *
Expand All @@ -113,21 +107,13 @@ public abstract class TableRowSkinBase<T,
* efficiency we create cells for all columns, even if they aren't visible,
* and we only create new cells if we don't already have it cached in this
* map.
*
* Note that this means that it is possible for this map to therefore be
* a memory leak if an application uses TableView and is creating and removing
* a large number of tableColumns. This is mitigated in the recreateCells()
* function below - refer to that to learn more.
*/
WeakHashMap<TableColumnBase, Reference<R>> cellsMap;

// This observableArrayList contains the currently visible table cells for this row.
final List<R> cells = new ArrayList<>();

private int fullRefreshCounter = DEFAULT_FULL_REFRESH_COUNTER;

boolean isDirty = false;
boolean updateCells = false;

// FIXME: replace cached values with direct lookup - JDK-8277000
double fixedCellSize;
Expand All @@ -152,7 +138,7 @@ public TableRowSkinBase(C control) {
getSkinnable().setPickOnBounds(false);

recreateCells();
updateCells(true);
updateCells();

// init bindings
// watches for any change in the leaf columns observableArrayList - this will indicate
Expand All @@ -176,7 +162,7 @@ public TableRowSkinBase(C control) {
* *
**************************************************************************/

private void updateLeafColumns() {
void updateLeafColumns() {
isDirty = true;
getSkinnable().requestLayout();
}
Expand Down Expand Up @@ -289,7 +275,6 @@ protected ObjectProperty<Node> graphicProperty() {
// earlier rows to update themselves to take into account
// this increased indentation.
final VirtualFlow<C> flow = getVirtualFlow();
final int thisIndex = getSkinnable().getIndex();
for (int i = 0; i < flow.cells.size(); i++) {
C cell = flow.cells.get(i);
if (cell == null || cell.isEmpty()) continue;
Expand Down Expand Up @@ -318,10 +303,13 @@ protected ObjectProperty<Node> graphicProperty() {
int index = control.getIndex();
if (index < 0/* || row >= itemsProperty().get().size()*/) return;

VirtualFlow<C> virtualFlow = getVirtualFlow();
for (int column = 0, max = cells.size(); column < max; column++) {
R tableCell = cells.get(column);
TableColumnBase<T, ?> tableColumn = getTableColumn(tableCell);

width = snapSizeX(tableColumn.getWidth());

boolean isVisible = true;
if (fixedCellSizeEnabled) {
// we determine if the cell is visible, and if not we have the
Expand All @@ -333,7 +321,7 @@ protected ObjectProperty<Node> graphicProperty() {
// provided by the developer, and this means that we do not have
// to concern ourselves with the possibility that the height
// may be variable and / or dynamic.
isVisible = isColumnPartiallyOrFullyVisible(tableColumn);
isVisible = isColumnPartiallyOrFullyVisible(x, width, virtualFlow);

y = 0;
height = fixedCellSize;
Expand All @@ -342,12 +330,9 @@ protected ObjectProperty<Node> graphicProperty() {
}

if (isVisible) {
if (fixedCellSizeEnabled && tableCell.getParent() == null) {
if (tableCell.getParent() == null) {
getChildren().add(tableCell);
}
// Note: prefWidth() has to be called only after the tableCell is added to the tableRow, if it wasn't
// already. Otherwise, it might not have its skin yet, and its pref width is therefore 0.
width = tableCell.prefWidth(height);

// Added for RT-32700, and then updated for RT-34074.
// We change the alignment from CENTER_LEFT to TOP_LEFT if the
Expand Down Expand Up @@ -419,11 +404,7 @@ protected ObjectProperty<Node> graphicProperty() {
// This does not appear to impact performance...
tableCell.requestLayout();
} else {
width = tableCell.prefWidth(height);
if (fixedCellSizeEnabled) {
// we only add/remove to the scenegraph if the fixed cell
// length support is enabled - otherwise we keep all
// TableCells in the scenegraph
if (tableCell.getParent() != null) {
getChildren().remove(tableCell);
}
}
Expand Down Expand Up @@ -473,21 +454,9 @@ boolean isShowRoot() {
return true;
}

void updateCells(boolean resetChildren) {
// To avoid a potential memory leak (when the TableColumns in the
// TableView are created/inserted/removed/deleted, we have a 'refresh
// counter' that when we reach 0 will delete all cells in this row
// and recreate all of them.
if (resetChildren) {
if (fullRefreshCounter == 0) {
recreateCells();
}
fullRefreshCounter--;
}

void updateCells() {
// if clear isn't called first, we can run into situations where the
// cells aren't updated properly.
final boolean cellsEmpty = cells.isEmpty();
cells.clear();

final C skinnable = getSkinnable();
Expand Down Expand Up @@ -518,23 +487,7 @@ void updateCells(boolean resetChildren) {
cells.add(cell);
}

// update children of each row
if (fixedCellSizeEnabled) {
// we leave the adding / removing up to the layoutChildren method mostly, but here we remove any children
// cells that refer to columns that are removed or not visible.
List<Node> toRemove = new ArrayList<>();
for (Node cell : getChildren()) {
if (!(cell instanceof IndexedCell)) continue;
TableColumnBase<T, ?> tableColumn = getTableColumn((R) cell);
if (!getVisibleLeafColumns().contains(tableColumn)) {
toRemove.add(cell);
}
}
getChildren().removeAll(toRemove);
}
if (resetChildren || cellsEmpty) {
getChildren().setAll(cells);
}
getChildren().setAll(cells);
}

VirtualFlow<C> getVirtualFlow() {
Expand Down Expand Up @@ -630,12 +583,8 @@ VirtualFlow<C> getVirtualFlow() {

final void checkState() {
if (isDirty) {
updateCells(true);
updateCells();
isDirty = false;
updateCells = false;
} else if (updateCells) {
updateCells(false);
updateCells = false;
}
}

Expand All @@ -655,33 +604,18 @@ void setDirty(boolean dirty) {
* *
**************************************************************************/

private boolean isColumnPartiallyOrFullyVisible(TableColumnBase col) {
if (col == null || !col.isVisible()) return false;
private boolean isColumnPartiallyOrFullyVisible(double start, double width, VirtualFlow<C> virtualFlow) {
double end = start + width;

final VirtualFlow<?> virtualFlow = getVirtualFlow();
double scrollX = virtualFlow == null ? 0.0 : virtualFlow.getHbar().getValue();
double headerWidth = virtualFlow == null ? 0.0 : virtualFlow.getViewportBreadth();
double virtualFlowWidth = headerWidth + scrollX;

// work out where this column header is, and it's width (start -> end)
double start = 0;
final ObservableList<? extends TableColumnBase> visibleLeafColumns = getVisibleLeafColumns();
for (int i = 0, max = visibleLeafColumns.size(); i < max; i++) {
TableColumnBase<?,?> c = visibleLeafColumns.get(i);
if (c.equals(col)) break;
start += c.getWidth();
}
double end = start + col.getWidth();

// determine the width of the table
final Insets padding = getSkinnable().getPadding();
double headerWidth = getSkinnable().getWidth() - padding.getLeft() + padding.getRight();

return (start >= scrollX || end > scrollX) && (start < (headerWidth + scrollX) || end <= (headerWidth + scrollX));
return (start >= scrollX || end > scrollX) && (start < virtualFlowWidth || end <= virtualFlowWidth);
}

private void requestCellUpdate() {
updateCells = true;
getSkinnable().requestLayout();

// update the index of all children cells (RT-29849).
// Note that we do this after the TableRow item has been updated,
// rather than when the TableRow index has changed (as this will be
Expand Down Expand Up @@ -713,7 +647,6 @@ private void recreateCells() {
ObservableList<? extends TableColumnBase/*<T,?>*/> columns = getVisibleLeafColumns();

cellsMap = new WeakHashMap<>(columns.size());
fullRefreshCounter = DEFAULT_FULL_REFRESH_COUNTER;
getChildren().clear();

for (TableColumnBase col : columns) {
Expand Down
Loading