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

Jump to bottom

fix(react-grid): correct calculating start index of loading row for Infinite Scrolling #2256

Merged
merged 11 commits into from
Aug 27, 2019
Merged

fix(react-grid): correct calculating start index of loading row for Infinite Scrolling #2256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f96c158
Fix bug
LazyLahtak Aug 21, 2019
78d22eb
Correct tests
LazyLahtak Aug 21, 2019
6769228
Update API
LazyLahtak Aug 21, 2019
43f978d
Refactor
LazyLahtak Aug 22, 2019
11af8be
Fix
LazyLahtak Aug 22, 2019
672a0e8
Correct calculation and add test
LazyLahtak Aug 23, 2019
4878aef
Update API
LazyLahtak Aug 23, 2019
9fb6479
Refactor
LazyLahtak Aug 23, 2019
37d1f00
Refactor
LazyLahtak Aug 26, 2019
89301ed
Refactor
LazyLahtak Aug 27, 2019
bdce541
Merge branch 'master' into virtual-table-index-bug
LazyLahtak Aug 27, 2019
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
71 changes: 62 additions & 9 deletions packages/dx-grid-core/src/plugins/virtual-table-state/helpers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ describe('VirtualTableState helpers', () => {
const newInterval = createInterval(200, 500);
const virtualRows = createVirtualRows(loadedInterval);

expect(calculateRequestedRange(virtualRows, newInterval, 310, pageSize))
expect(calculateRequestedRange(virtualRows, newInterval, pageSize))
.toEqual({ start: 400, end: 500 });
});

Expand All @@ -170,7 +170,7 @@ describe('VirtualTableState helpers', () => {
const newInterval = createInterval(100, 400);
const virtualRows = createVirtualRows(loadedInterval);

expect(calculateRequestedRange(virtualRows, newInterval, 310, pageSize))
expect(calculateRequestedRange(virtualRows, newInterval, pageSize))
.toEqual({ start: 100, end: 200 });
});

Expand All @@ -179,7 +179,7 @@ describe('VirtualTableState helpers', () => {
const newInterval = createInterval(400, 700);
const virtualRows = createVirtualRows(loadedInterval);

expect(calculateRequestedRange(virtualRows, newInterval, 580, pageSize))
expect(calculateRequestedRange(virtualRows, newInterval, pageSize))
.toEqual({ start: 400, end: 700 });
});

Expand All @@ -188,8 +188,8 @@ describe('VirtualTableState helpers', () => {
const newInterval = createInterval(100, 400);
const virtualRows = createVirtualRows(loadedInterval);

expect(calculateRequestedRange(virtualRows, newInterval, 270, pageSize))
.toEqual({ start: 100, end: 300 });
expect(calculateRequestedRange(virtualRows, newInterval, pageSize))
.toEqual({ start: 100, end: 400 });
});
});

Expand All @@ -199,7 +199,7 @@ describe('VirtualTableState helpers', () => {
const newInterval = createInterval(200, 500);
const virtualRows = createVirtualRows(loadedInterval);

expect(calculateRequestedRange(virtualRows, newInterval, 300, pageSize))
expect(calculateRequestedRange(virtualRows, newInterval, pageSize))
.toEqual({ start: 200, end: 500 });
});

Expand All @@ -208,7 +208,7 @@ describe('VirtualTableState helpers', () => {
const newInterval = createInterval(200, 500);
const virtualRows = createVirtualRows(loadedInterval);

expect(calculateRequestedRange(virtualRows, newInterval, 399, pageSize))
expect(calculateRequestedRange(virtualRows, newInterval, pageSize))
.toEqual({ start: 200, end: 500 });
});

Expand All @@ -217,7 +217,7 @@ describe('VirtualTableState helpers', () => {
const newInterval = createInterval(1000, 1200);
const virtualRows = createVirtualRows(loadedInterval);

expect(calculateRequestedRange(virtualRows, newInterval, 1180, pageSize))
expect(calculateRequestedRange(virtualRows, newInterval, pageSize))
.toEqual({ start: 1000, end: 1200 });
});
});
Expand All @@ -228,10 +228,63 @@ describe('VirtualTableState helpers', () => {
const virtualRows = createVirtualRows(loadedInterval);

it('should return prev, current and next pages', () => {
expect(calculateRequestedRange(virtualRows, newInterval, 1170, pageSize))
expect(calculateRequestedRange(virtualRows, newInterval, pageSize))
.toEqual({ start: 1000, end: 1300 });
});
});

describe('reference index', () => {
// tslint:disable-next-line: max-line-length
it('should caclulate correct if reference index less than the start of a new interval', () => {
const loadedInterval = createInterval(100, 400);
const newInterval = createInterval(200, 500);
const virtualRows = createVirtualRows(loadedInterval);
const referenceIndex = 440;

expect(calculateRequestedRange(virtualRows, newInterval, pageSize, referenceIndex))
.toEqual({ start: 400, end: 500 });
});

// tslint:disable-next-line: max-line-length
it('should caclulate correct if reference index more than the start of a new interval and less than half of page', () => {
const loadedInterval = createInterval(100, 400);
const newInterval = createInterval(200, 500);
const virtualRows = createVirtualRows(loadedInterval);
const referenceIndex = 360;

expect(calculateRequestedRange(virtualRows, newInterval, pageSize, referenceIndex))
.toEqual({ start: 400, end: 500 });
});

// tslint:disable-next-line: max-line-length
describe('reference index more than the start of a new interval and less than half of page', () => {
const loadedInterval = createInterval(100, 400);
const newInterval = createInterval(200, 500);
const virtualRows = createVirtualRows(loadedInterval);
const referenceIndex = 320;

it('should caclulate correct if infinite scrolling', () => {
expect(calculateRequestedRange(virtualRows, newInterval, pageSize, referenceIndex, true))
.toEqual({ start: 300, end: 400 });
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we want rows 300-400 if they're loaded already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we try load next page (rows 400-500) width index less than page 350 (for pageSize === 100), we catch index bug. It loads nextPage instead curren when we fast scroll up.
We see at string 320, cache know about previous (200-300) and next page (400-500), and we load 400-500. Then, we get rows combination:

[200,
201,
...
300,
400,
401,
...
500,
401,
...
500]

For index less than start - pageSize/2 we need to shift loading interval.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it reproduce in non-infinite mode?

});

it('should caclulate correct if non-infinite scrolling', () => {
expect(calculateRequestedRange(virtualRows, newInterval, pageSize, referenceIndex, false))
.toEqual({ start: 400, end: 500 });
});
});
});

// tslint:disable-next-line: max-line-length
it('should caclulate correct in non-infinite scrolling', () => {
const loadedInterval = createInterval(100, 400);
const newInterval = createInterval(200, 500);
const virtualRows = createVirtualRows(loadedInterval);
const referenceIndex = 320;

expect(calculateRequestedRange(virtualRows, newInterval, pageSize, referenceIndex))
.toEqual({ start: 400, end: 500 });
});
});

describe('#rowToPageIndex', () => {
Expand Down
28 changes: 22 additions & 6 deletions packages/dx-grid-core/src/plugins/virtual-table-state/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { intervalUtil } from './utils';
import {
VirtualRows, Row, MergeRowsFn, CalculateRequestedRangeFn, Interval, GridViewport, GetRequestMeta,
VirtualRows, Row, MergeRowsFn, CalculateRequestedRangeFn,
Interval, GridViewport, GetRequestMeta, CorrectRangeFn,
} from '../../types';
import { PureComputed } from '@devexpress/dx-core';

Expand Down Expand Up @@ -42,13 +43,26 @@ export const mergeRows: MergeRowsFn = (
};
};

const correctRequestedRange: CorrectRangeFn = (calculatedRange, referenceIndex, pageSize) => {
const { start, end } = calculatedRange;

if (start - referenceIndex > pageSize / 2) {
return { start: start - pageSize, end: end - pageSize };
}
return { start, end };
};

export const calculateRequestedRange: CalculateRequestedRangeFn = (
virtualRows, newRange, pageSize,
virtualRows, newRange, pageSize, referenceIndex, isInfiniteScroll,
) => {
const loadedInterval = intervalUtil.getRowsInterval(virtualRows);
const isAdjacentPage = Math.abs(loadedInterval.start - newRange.start) < 2 * pageSize;
if (isAdjacentPage) {
return intervalUtil.difference(newRange, loadedInterval);
const calculatedRange = intervalUtil.difference(newRange, loadedInterval);
if (isInfiniteScroll && calculatedRange !== intervalUtil.empty) {
return correctRequestedRange(calculatedRange, referenceIndex, pageSize);
}
return calculatedRange;
}

// load 3 pages at once because a missing page will be loaded anyway
Expand Down Expand Up @@ -120,14 +134,16 @@ export const getForceReloadInterval: PureComputed<[VirtualRows, number, number],
};

export const getRequestMeta: GetRequestMeta = (
referenceIndex, virtualRows, pageSize, totalRowCount, forceReload,
referenceIndex, virtualRows, pageSize, totalRowCount, forceReload, isInfiniteScroll,
) => {
const actualBounds = forceReload
? getForceReloadInterval(virtualRows, pageSize!, totalRowCount)
: recalculateBounds(referenceIndex, pageSize!, totalRowCount);
const requestedRange = forceReload
? actualBounds
: calculateRequestedRange(virtualRows, actualBounds, pageSize!);
: calculateRequestedRange(
virtualRows, actualBounds, pageSize!, referenceIndex, isInfiniteScroll,
);

return { requestedRange, actualBounds };
};
Expand All @@ -138,7 +154,7 @@ export const needFetchMorePages: PureComputed<[VirtualRows, number, number], boo
const { start, end } = intervalUtil.getRowsInterval(virtualRows);
const loadCount = end - start;
const topTriggerIndex = start > 0 ? start + pageSize! : 0;
const bottomTriggerIndex = end - pageSize!;
const bottomTriggerIndex = Math.max(topTriggerIndex + pageSize, end - pageSize! * 1.5);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is a 1.5 value?

Copy link
Contributor Author

@LazyLahtak LazyLahtak Aug 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Half of page.
Sometimes difference between bottomTriggerIndex and topTriggerIndex more than one pageSize and we can get index, than no trigger update. This is index in interval between end - pageSize! and end - pageSize! * 1.5
For example, in master try after load press End button twice (lazy loading => infinite scrolling demo)
You will stuck at string 150 as last with bottomTriggerIndex === 150 and index ~ 140

Copy link
Contributor Author

@LazyLahtak LazyLahtak Aug 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Sometimes we may catch this bug not in beginning of table. It is quite difficult to catch, but can. This check fixes it.


if (loadCount <= 0) {
return false;
Expand Down
10 changes: 8 additions & 2 deletions packages/dx-grid-core/src/types/virtual-table-state.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,15 @@ export type MergeRowsFn = PureComputed<
>;

/** @internal */
export type CalculateRequestedRangeFn = PureComputed<[VirtualRows, Interval, number], Interval>;
export type CalculateRequestedRangeFn = PureComputed<
[VirtualRows, Interval, number, number, boolean], Interval
>;
/** @internal */
export type CorrectRangeFn = PureComputed<
[Interval, number, number]
>;
/** @internal */
export type GetRequestMeta = PureComputed<
[number, VirtualRows, number, number, boolean],
[number, VirtualRows, number, number, boolean, boolean],
{ requestedRange: Interval, actualBounds: Interval }
>;
4 changes: 2 additions & 2 deletions packages/dx-react-grid/src/plugins/virtual-table/virtual-table-state.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,11 @@ class VirtualTableStateBase extends React.PureComponent<VirtualTableStateProps,
{ referenceIndex, forceReload },
{ virtualRows }: Getters,
) => {
const { pageSize, totalRowCount } = this.props;
const { pageSize, totalRowCount, infiniteScrolling } = this.props;
const { requestedStartIndex } = this.state;
const actualVirtualRows = forceReload ? emptyVirtualRows : virtualRows;
const { requestedRange, actualBounds } = getRequestMeta(
referenceIndex, virtualRows, pageSize!, totalRowCount, forceReload,
referenceIndex, virtualRows, pageSize!, totalRowCount, forceReload, infiniteScrolling,
);

if (forceReload || shouldSendRequest(requestedRange, requestedStartIndex)) {
Expand Down