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

Fetch and Cache previous row heights #701

Merged
merged 13 commits into from
Dec 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions docs/public_api/api_reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,16 @@ Scrolls the table to given horizontal offset.
function(scrollX: number)
```

#### updateRowHeights(firstUpdatedRowIndex)
In case of variable row heights the FDT asks only once for the row heights before the current visible rows (by calling `rowHeightGetter()`) and it caches those heights.
If any of the row heights changes meantime, the user should call `updateRowHeights(firstUpdatedRowIndex)` in order for the new row heights to be updated
starting with the ```firstUpdatedRowIndex```
If the method is called without passing the ```firstUpdatedRowIndex``` it updates all the row heights
```ts
function(firstUpdatedRowIndex: number)
```


## Types
#### Column
```ts
Expand Down
19 changes: 19 additions & 0 deletions src/FixedDataTable.js
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,25 @@ class FixedDataTable extends React.Component {
*/
scrollTop: PropTypes.number,

/**
* By default in case of variable rows heights,
* when table uses `scrollTop` it makes an optimization
* in order to not ask for all the rows heights, it uses the cached stored heights,
* that are initialized to the constant `rowHeight` property.
* Those heights are updated with the actual value (requested using `rowHeightGetter`)
* only when the rows becomes visible.
* So when the `scrollTop` is incremented step by step, the actual displayed row is exact,
* but when the `scrollTop` is set to a far position, the actual displayed row is inexact
*
* E.g. : first row height = 30, but the rest of the rows height = 500px,
* and the constant `rowheight` property = 30,
* when scrollTop changes from 0 to 5000, the displayed first row instead of being 11 is 57
*
* By setting ```isVerticalScrollExact``` to true, when trying to scroll to ```scrollTop``` position, the table will consider
* the exact row heights, so the offset of the displayed rows will be correct
*/
isVerticalScrollExact: PropTypes.bool,

/**
* Index of row to scroll to.
*/
Expand Down
12 changes: 10 additions & 2 deletions src/FixedDataTableContainer.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@ import Scrollbar from './plugins/Scrollbar';
import ScrollContainer from './plugins/ScrollContainer';
import { FixedDataTableContext } from './FixedDataTableContext';
import { createApi } from './api';
import { initialize, propChange } from './reducers';
import { initialize, propChange, updateRowHeights } from './reducers';
import { polyfill as lifecycleCompatibilityPolyfill } from 'react-lifecycles-compat';
import { bindActionCreators } from 'redux';

class FixedDataTableContainer extends React.Component {
static defaultProps = {
Expand Down Expand Up @@ -106,7 +107,14 @@ class FixedDataTableContainer extends React.Component {
...this.props,
...this.reduxStore.getState(),
},
this.scrollActions
{
...this.scrollActions,
updateRowHeights: (firstUpdatedRowIndex) =>
bindActionCreators(
{ updateRowHeights },
this.reduxStore.dispatch
).updateRowHeights(firstUpdatedRowIndex),
}
);
}

Expand Down
2 changes: 2 additions & 0 deletions src/api/apiMethods.js
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,7 @@ const getApiMethodsSelector = () =>
};

const scrollToX = actions.scrollToX;
const updateRowHeights = actions.updateRowHeights;

return {
/** get element */
Expand All @@ -371,6 +372,7 @@ const getApiMethodsSelector = () =>

/** actions */
scrollToX,
updateRowHeights,
};
}
);
Expand Down
32 changes: 29 additions & 3 deletions src/reducers/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,14 @@ import columnStateHelper from './columnStateHelper';
import computeRenderedRows from './computeRenderedRows';
import Scrollbar from '../plugins/Scrollbar';
import { createSlice, original } from '@reduxjs/toolkit';
import updateRowHeight from './updateRowHeight';

/**
* @typedef {{
* rowBufferSet: IntegerBufferSet,
* rowOffsetIntervalTree: PrefixIntervalTree,
* storedHeights: !Array.<number>
* storedHeights: !Array.<number>,
* rowUntilOffsetsAreExact: number
* }}
*/
const InternalState = {};
Expand Down Expand Up @@ -128,6 +130,7 @@ function createInternalState() {
rowBufferSet: new IntegerBufferSet(),
rowOffsetIntervalTree: null, // PrefixIntervalTree
storedHeights: [],
rowUntilOffsetsAreExact: 0,
};
}

Expand Down Expand Up @@ -209,6 +212,22 @@ const slice = createSlice({
state.scrolling = true;
state.scrollX = scrollX;
},
updateRowHeights(state, action) {
const firstUpdatedRowIndex = action.payload || 0;
if (firstUpdatedRowIndex >= state.getInternal().rowUntilOffsetsAreExact) {
return;
}
// Invalidate all the previous computed row heights till the updated row
state.getInternal().rowUntilOffsetsAreExact = firstUpdatedRowIndex;
// Refresh the current scroll position according to the new row heights
const currentScrollY =
state
.getInternal()
.rowOffsetIntervalTree.sumUntil(state.firstRowIndex) -
state.firstRowOffset;
const scrollAnchor = scrollTo(state, currentScrollY);
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this pass the third param as props.useExactRowHeight or true ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. You are right. I have changed this. Thank you

computeRenderedRows(state, scrollAnchor);
},
},
});

Expand Down Expand Up @@ -256,6 +275,7 @@ function setStateFromProps(state, props) {
columnElements,
elementTemplates,
propsRevision: state.propsRevision + 1,
isVerticalScrollExact: props.isVerticalScrollExact,
});

// NOTE (pradeep): We pre-freeze these large collections to avoid
Expand Down Expand Up @@ -314,6 +334,12 @@ function setStateFromProps(state, props) {
}

const { reducer, actions } = slice;
export const { initialize, propChange, scrollEnd, scrollToX, scrollToY } =
actions;
export const {
initialize,
propChange,
scrollEnd,
scrollToX,
scrollToY,
updateRowHeights,
} = actions;
export default reducer;
14 changes: 14 additions & 0 deletions src/reducers/scrollAnchor.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,20 @@ export function scrollTo(state, scrollY) {
const { rowOffsetIntervalTree } = state.getInternal();
const { rowsCount } = rowSettings;

if (
state.rowSettings.rowHeightGetter != undefined &&
state.isVerticalScrollExact
) {
// In case of variable row height, ask for the actual heights of the rows before the scroll position.
// Only for the ones that were not asked before
let { rowUntilOffsetsAreExact } = state.getInternal();

while (rowUntilOffsetsAreExact < rowsCount) {
updateRowHeight(state, rowUntilOffsetsAreExact++);
}
state.getInternal().rowUntilOffsetsAreExact = rowsCount;
}

if (rowsCount === 0) {
return {
firstIndex: 0,
Expand Down
44 changes: 44 additions & 0 deletions test/reducers/scrollAnchor-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
__RewireAPI__,
getScrollAnchor,
} from '../../src/reducers/scrollAnchor';
import PrefixIntervalTree from '../../src/vendor_upstream/struct/PrefixIntervalTree';

describe('scrollAnchor', function () {
beforeEach(function () {
Expand Down Expand Up @@ -77,6 +78,49 @@ describe('scrollAnchor', function () {
changed: true,
});
});

describe('scrollTo if rowHeightGetter() is set', function () {
beforeEach(function () {
oldState.getInternal().rowOffsetIntervalTree =
PrefixIntervalTree.uniform(100, 50);
const storedHeights = new Array(100);
for (let idx = 0; idx < 100; idx++) {
storedHeights[idx] = 50;
}
oldState.getInternal().storedHeights = storedHeights;

oldState.rowSettings = {
rowsCount: 100,
rowHeightGetter: () => 100,
subRowHeightGetter: () => 0,
};
});
it('should ask for rowHeightGetter() if the row height were not computed before', function () {
oldState.getInternal().rowUntilOffsetsAreExact = 0;
oldState.isVerticalScrollExact = true;

let scrollAnchor = getScrollAnchor(oldState, { scrollTop: 300 }, {});
assert.deepEqual(scrollAnchor, {
firstIndex: 3,
firstOffset: 0,
lastIndex: undefined,
changed: true,
});
});

it('should use the cached row heights if they were computed before', function () {
oldState.getInternal().rowUntilOffsetsAreExact = 4;
oldState.isVerticalScrollExact = true;

let scrollAnchor = getScrollAnchor(oldState, { scrollTop: 300 }, {});
assert.deepEqual(scrollAnchor, {
firstIndex: 5,
firstOffset: 0,
lastIndex: undefined,
changed: true,
});
});
});
});

describe('scrollToRow', function () {
Expand Down