Fetch and Cache previous row heights #701

daniela-mateescu · 2023-10-26T11:33:33Z

Fix for #700

Description

In case of variable row height, when scrolling to an exact Y position ensure that all the row heights before that Y position are exact. Avoid asking for the actual height for the ones asked before.

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.

karry08 · 2023-10-31T10:42:57Z

Hi thanks for the PR.
Since this is a change in core behaviour, We'll need to take bit more time to think through all scenarios in FDT that involve dynamic row heights.
Examples that may get affected by the change here:
Dynamic Row Heights.

We also need to add unit tests to make sure the scroll offsets are calculated precisely, and that rows are actually cached and not re-fetched.
Here is an example of a unit test for your reference - Scroll anchor unit test
Let us know if you need directions

site/examples/examplesPageStyle.less

src/reducers/scrollAnchor.js

karry08 · 2023-11-02T08:42:09Z

src/reducers/index.js

@@ -209,6 +212,18 @@ const slice = createSlice({
      state.scrolling = true;
      state.scrollX = scrollX;
    },
+    updateRowHeights(state) {


how about we pass an optional parameter updatedRowIndex, this way we wont need to set rowUntilOffsetsAreExact as 0 everytime.
For eg lets say there are 10000 rows and we have updated the height of 5000th row in that case we dont need to invalidate row heights and offsets of rows before 5000th row and we can simply set
state.getInternal().rowUntilOffsetsAreExact = updatedRowIndex //optional param and if this param is not passed then we can set it to 0
Furthermore if the current rowUntilOffsetsAreExact is less than the updatedRowIndex we dont need to change state (rowUntilOffsetsAreExact)

I think is ok. I have added this parameter

…ights when scrolling to a y position

daniela-mateescu · 2023-11-15T13:09:59Z

Hi thanks for the PR. Since this is a change in core behaviour, We'll need to take bit more time to think through all scenarios in FDT that involve dynamic row heights. Examples that may get affected by the change here: Dynamic Row Heights.

We also need to add unit tests to make sure the scroll offsets are calculated precisely, and that rows are actually cached and not re-fetched. Here is an example of a unit test for your reference - Scroll anchor unit test Let us know if you need directions

I have added two tests for the two cases mentioned above

daniela-mateescu · 2023-11-15T13:11:39Z

Please let us know if this new version it is ok and can be accepted.

karry08 · 2023-11-17T18:09:53Z

docs/public_api/api_reference.md

+starting with the ```firstUpdatedRowIndex```
+If the method is called without passing the ```firstUpdatedRowIndex``` it updates all the row heights 
+```ts
+function()


nit: function(firstUpdatedRowIndex: number)

Ok. Thank you. I have added this parameter

karry08 · 2023-11-17T18:16:05Z

Please let us know if this new version it is ok and can be accepted.

Thanks a lot for the changes, these look good just had a minor change suggestion and we also really appreciate you for adding the test
I will take one last look on monday and will then merge the changes and we'll make a new release with these changes
Thank you

pradeepnschrodinger · 2023-11-24T11:49:49Z

Hey, sorry for the delay.

@karry08 did some rounds of testing against various scenarios, and now we're of the opinion of feature flagging the changes:

This'll be a behavioral change for users which rely on lazy fetching row heights.
I don't think is a big deal though, because a lot of users might actually benefit from the scroll offset correction (eg: #700.
This affects scrollbar jump behavior.
Previously, the thumb jumped to the exact cursor position on mouse clicks. The scroll offset was inaccurate, but it still syncs up with the cursor somewhat.
But now that we are fetching all previous rows, the final scrollbar thumb position can appear at a very different spot.

@daniela-mateescu , @karry08 , we should probably bring a prop to control the new behavior.
Maybe something like fetchPreviousRows, which can be defaulted as false atleast until we figure out the scrollbar thumb issue.

on scrollTo() compute all the row height that were not computed before if isScrollTopExact

daniela-mateescu · 2023-11-28T11:07:02Z

Hi!

Thanks for your testing.
Indeed the scroll bar jump when clicking is a problem. This is caused by the fact that we compute the actual row heights only until the position where we want to scroll. But in order for the scrollbar to be at the correct position, relative to the entire scrollbar height, the entire scrollable height should be computed, so we need to compute all the rows heights.

I made this change, and now the scroll bar jump works ok.
I have also added a property isScrollTopExact, that enables this new logic added by us. By default is false, and works just it was initiallly designed: as an optimization it uses rowheight, instead of asking for the actual rowHeightGetter

Please let me know if you think that this solution is ok and can be accepted

cristian-spiescu · 2023-12-04T10:32:50Z

Hi guys. Any news? Thanks!

pradeepnschrodinger · 2023-12-05T09:54:00Z

src/reducers/index.js

+          .getInternal()
+          .rowOffsetIntervalTree.sumUntil(state.firstRowIndex) -
+        state.firstRowOffset;
+      const scrollAnchor = scrollTo(state, currentScrollY);


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

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

pradeepnschrodinger · 2023-12-05T10:19:27Z

Sorry for the lack of updates last week.

@karry08 is investigating on a trick that allows us to fetch just the previous rows but still keep the scroll bar in sync with the cursor.
This involves having FDT support % scroll offsets so that the scrollbar can use that for thumb click/jump scrolls.

As for the new changes in this PR, I see that the latest commit makes FDT fetch ALL rows at once.
While this ensures 100% accuracy and solves syncing issues, I have a small fear on whether this'll be too heavy for really large tables.
It might be nice if we can let the user choose between different offset accuracies / fetching strategies, maybe by having an enum prop.

Using the new changes as an example:
When isScrollTopExact is false or 'visible_rows' (the default), FDT only fetches visible rows.
When isScrollTopExact is true or 'all_rows' (ideal for synchronization between scrollable containers), FDT fetches ALL rows.
When isScrollTopExact is 'previous_rows', it's a balance between these two where all rows until the current visible row are fetched.

We're still discussing internally about this.
But I think we can go ahead with merging this PR with isScrollTop being a boolean right now, and later extending it to also an enum string to support the "fetch previous rows" mode.
@karry08 , @AmanGupta2708 , @karry08 , what do you guys think?

pradeepnschrodinger

LGTM with 1 suggestion!

And friendly ping on this for reviewers: @karry08 , @AmanGupta2708 , please see my previous comment.
If noone has concerns with the plan, I'll proceed with merging this.

pradeepnschrodinger · 2023-12-11T12:27:51Z

src/FixedDataTable.js

+     * By setting ```isScrollTopExact``` 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
+     */
+    isScrollTopExact: PropTypes.bool,


Let's rename this to isVerticalScrollExact since it deals with vertical scrolls in general, and not exclusive to scrollTop as before.

isVerticalScrollExact seems better to me. I changed and use this instead of` isScrollTopExact

pradeepnschrodinger

I was almost about to merge this, but then I realized the changes here no longer work as expected when controlled scrolling is OFF.
When I simply set isVerticalScrollExact to true in the DynamicRowHeightsExample , FDT does not fetch all row heights.
Apologies for finding this late :( .

@daniela-mateescu , could you take another took?

And thanks in advance for your patience. I'll take another look on priority once the new changes are pushed in.
Please let me know if anything is unclear/tricky to solve.

pradeepnschrodinger · 2023-12-14T00:14:52Z

src/reducers/scrollAnchor.js

@@ -68,12 +68,23 @@ export function getScrollAnchor(state, newProps, oldProps) {
 *   changed: boolean,
 * }}
 */
-export function scrollTo(state, scrollY) {
+export function scrollTo(state, scrollY, isVerticalScrollExact) {


There are calls to this function in other parts of the codebase where the third param is unspecified.
Some examples (but not limited to):

fixed-data-table-2/src/reducers/index.js

Line 204 in 6bd7bb8

const scrollAnchor = scrollTo(state, scrollY);

fixed-data-table-2/src/actions/scrollActions.js

Line 59 in 6bd7bb8

scrollActions.scrollToY(scrollPos);

fixed-data-table-2/src/plugins/ScrollContainer.js

Line 31 in 6bd7bb8

this.state.scrollToY(scrollPos);

This has the risk that some workflows in the table (eg: uncontrolled scrolling) don't use exact row offsets even when isVerticalScrollExact is true.

There's two approaches that I can immediately think of:

The first one is of course pass the prop isVerticalScrollExact in all these usages as the third param.
But this can be a bit cumbersome cause some of the calls are nested within components.

The second one -- which I'm more inclined towards -- is to simply copy the isVerticalScrollExact prop to the redux store as part of "input" state (see setStateFromProps()) for similar cases.
Once this is done, this reducer can simply query state.isVerticalScrollExact, thus avoiding the need for the caller to pass it explicitly.

Hello!

Thank you for this observation. I have modified as you suggested in order to avoid passing isVerticalScrollExact but instead pass it via state. Now the row heights are computed also for the DynamicRowHeightExample.

Do you think it's ok now?

nice, that works.

pradeepnschrodinger

LGTM, thanks for the continously addressing the comments!

There's still an edge case where the initial scroll AND "jump to row" scroll actions don't use exact scroll offsets even with isVerticalScrollExact set as true.
I think this is because the scrollTo reducer doesn't get called in these scenarios.

You can reproduce this by simply checking out the DynamicRowHeights example, specifying isVerticalScrollExact={true}, and scrolling exactly once by clicking on the vertical scrollbar area.

However, I think we can track and solve the edge case with a separate issue/PR, especially because the new changes are feature flagged via isVerticalScrollExact.

I'm going ahead with merging the PR, but it would be nice to fix the edge case soon.
cc: @karry08

pradeepnschrodinger · 2023-12-19T16:50:45Z

Released with v2.0.5.

daniela-buzatu added 3 commits October 20, 2023 16:59

exactScrollTopInCaseOfVariableRowHeights + example

1183988

exactScrollTopInCaseOfVariableRowHeights

2d8f627

Remove exactScrollTopInCaseOfVariableRowHeights

cdf908c

cristian-spiescu mentioned this pull request Oct 26, 2023

When using variable row heights, setting scrollTop doesn't work well. Fix proposed #700

Closed

pradeepnschrodinger changed the title ~~Fix for https://github.com/schrodinger/fixed-data-table-2/issues/700~~ Fetch and Cache previous row heights Oct 27, 2023

pradeepnschrodinger requested review from pradeepnschrodinger, abhisheksingh2410, karry08 and AmanGupta2708 October 27, 2023 15:32

pradeepnschrodinger self-assigned this Oct 27, 2023

pradeepnschrodinger added the enhancement label Oct 27, 2023

karry08 reviewed Nov 2, 2023

View reviewed changes

site/examples/examplesPageStyle.less Outdated Show resolved Hide resolved

karry08 reviewed Nov 2, 2023

View reviewed changes

src/reducers/scrollAnchor.js Outdated Show resolved Hide resolved

karry08 reviewed Nov 2, 2023

View reviewed changes

src/reducers/scrollAnchor.js Outdated Show resolved Hide resolved

karry08 reviewed Nov 2, 2023

View reviewed changes

daniela-buzatu added 2 commits November 15, 2023 15:03

schrodinger#701 PR review + tests for caching and updating the row he…

1b4279f

…ights when scrolling to a y position

schrodinger#701 PR review + tests for caching and updating the row he…

9c8509d

…ights when scrolling to a y position

update documnetation for parameter updateRowHeights() method

413d448

karry08 reviewed Nov 17, 2023

View reviewed changes

daniela-buzatu added 2 commits November 20, 2023 10:32

Add firstUpdatedRowIndex parameter to updateRowHeights in documentation

a77028e

Remove a leftover console.log

feb17e9

pradeepnschrodinger assigned karry08 Nov 25, 2023

add isScrollTopExact property +

3a80d73

on scrollTo() compute all the row height that were not computed before if isScrollTopExact

Fix tests after isScrollTopExact addition

08a6595

pradeepnschrodinger reviewed Dec 5, 2023

View reviewed changes

Fix small review remark

61afc4a

pradeepnschrodinger approved these changes Dec 11, 2023

View reviewed changes

rename isScrollTopExact to isVerticalScrollExact

dac8d31

pradeepnschrodinger requested changes Dec 14, 2023

View reviewed changes

avoid passing isVerticalScrollExact as parameter, by adding it to state

e61fb46

pradeepnschrodinger approved these changes Dec 18, 2023

View reviewed changes

pradeepnschrodinger merged commit 23614b4 into schrodinger:master Dec 19, 2023
3 checks passed

daniela-mateescu mentioned this pull request Dec 20, 2023

Scroll to an exact position when clicking the scrollbar track doesn't work in case of variable row heights and isVerticalScrollExact = true #711

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fetch and Cache previous row heights #701

Fetch and Cache previous row heights #701

daniela-mateescu commented Oct 26, 2023 •

edited

Loading

karry08 commented Oct 31, 2023

karry08 Nov 2, 2023

daniela-mateescu Nov 15, 2023

daniela-mateescu commented Nov 15, 2023

daniela-mateescu commented Nov 15, 2023

karry08 Nov 17, 2023

daniela-mateescu Nov 20, 2023

karry08 commented Nov 17, 2023

pradeepnschrodinger commented Nov 24, 2023

daniela-mateescu commented Nov 28, 2023 •

edited

Loading

cristian-spiescu commented Dec 4, 2023

pradeepnschrodinger Dec 5, 2023

daniela-mateescu Dec 5, 2023

pradeepnschrodinger commented Dec 5, 2023

pradeepnschrodinger left a comment

pradeepnschrodinger Dec 11, 2023

daniela-mateescu Dec 11, 2023 •

edited

Loading

pradeepnschrodinger left a comment

pradeepnschrodinger Dec 14, 2023

daniela-mateescu Dec 18, 2023

pradeepnschrodinger Dec 18, 2023

pradeepnschrodinger left a comment

pradeepnschrodinger commented Dec 19, 2023

Fetch and Cache previous row heights #701

Fetch and Cache previous row heights #701

Conversation

daniela-mateescu commented Oct 26, 2023 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

karry08 commented Oct 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniela-mateescu commented Nov 15, 2023

daniela-mateescu commented Nov 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karry08 commented Nov 17, 2023

pradeepnschrodinger commented Nov 24, 2023

daniela-mateescu commented Nov 28, 2023 • edited Loading

cristian-spiescu commented Dec 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradeepnschrodinger commented Dec 5, 2023

pradeepnschrodinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniela-mateescu Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

pradeepnschrodinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradeepnschrodinger left a comment

Choose a reason for hiding this comment

pradeepnschrodinger commented Dec 19, 2023

daniela-mateescu commented Oct 26, 2023 •

edited

Loading

daniela-mateescu commented Nov 28, 2023 •

edited

Loading

daniela-mateescu Dec 11, 2023 •

edited

Loading