feat(virtual-scroll): fixed size virtual scroll #9316

mmalerba · 2018-01-10T00:48:20Z

No description provided.

crisbeto · 2018-01-10T17:53:59Z

src/cdk/scrolling/for-of.ts

+})
+export class CdkForOf<T> implements CollectionViewer, DoCheck, OnDestroy {
+  /** Emits when the rendered view of the data changes. */
+  viewChange = new Subject<Range>();


Rename this to viewChanged? Otherwise it could introduce an unintentional two-way binding if we decided to make this an output and add an input called view.

This has to be called viewChange because it's implementing CollectionViewer

crisbeto · 2018-01-10T18:01:23Z

src/cdk/scrolling/virtual-scroll-viewport.ts

+
+  /** The offset of the rendered portion of the data from the start. */
+  get renderedContentOffset() { return this._renderedContentOffset; }
+  set renderedContentOffset(offset) {


Add a type to the param?

crisbeto · 2018-01-10T18:03:17Z

src/cdk/scrolling/virtual-scroll-viewport.ts

+    Promise.resolve().then(() => {
+      this._viewportSize = this.orientation === 'horizontal' ?
+          this.elementRef.nativeElement.clientWidth : this.elementRef.nativeElement.clientHeight;
+      fromEvent(this.elementRef.nativeElement, 'scroll').subscribe(() => {


This one should be run outside the NgZone.

crisbeto · 2018-01-10T18:05:07Z

src/cdk/scrolling/virtual-scroll-viewport.ts

+  }
+
+  ngOnInit() {
+    Promise.resolve().then(() => {


Rather than wrapping it in a promise, have you tried running it in one of the later lifecycle hooks?

I tried, it seemed like they were not late enough and I had to use the promise

crisbeto · 2018-01-10T18:11:13Z

src/cdk/scrolling/for-of.ts

+    // Mark the removed indices so we can recycle their views.
+    changes.forEachRemovedItem(record => {
+      this._templateCache.push(previousViews[record.previousIndex!]);
+      delete previousViews[record.previousIndex!];


Using delete here won't reindex the array, is that intended? Otherwise it might be better to use splice to avoid some unintended side-effects. Same goes for the other delete usages.

Yes this is intentional. I need each element to be at its correct original index so it matches up with the index in the IterableChangeRecord. I then use delete to mark the view as having been handled in some way (cached or queued up for re-insertion).

Would it work to to null? That's generally much faster than using delete

crisbeto · 2018-01-10T18:12:06Z

src/cdk/scrolling/for-of.ts

+  }
+
+  /** Apply changes to the DOM. */
+  private _applyChanges(changes: IterableChanges<T> | null) {


This method is getting kinda long, consider splitting it into a few smaller ones?

Agreed, it does quite a lot.

CaerusKaru · 2018-01-10T22:07:14Z

src/cdk/collections/data-source.ts

@@ -7,8 +7,10 @@
 */

 import {Observable} from 'rxjs/Observable';
+import {of} from 'rxjs/observable/of';


Should be import {of as observableOf} from 'rxjs/observable/of'

jelbourn · 2018-01-11T18:28:17Z

src/cdk/collections/data-source.ts

+
+
+/** DataSource wrapper for a native array. */
+export class ArrayDataSource<T> implements DataSource<T> {


Move this to its own file?

jelbourn · 2018-01-11T18:28:48Z

src/cdk/collections/data-source.ts

+
+
+/** DataSource wrapper for a native array. */
+export class ArrayDataSource<T> implements DataSource<T> {


Maybe call this StaticArrayDataSource, since there's no way for it to emit once set?

jelbourn · 2018-01-11T18:30:30Z

src/cdk/scrolling/for-of.ts

+import {switchMap} from 'rxjs/operators/switchMap';
+import {Subject} from 'rxjs/Subject';
+import {CdkVirtualScrollViewport} from './virtual-scroll-viewport';
+


Should these changes go into cdk-experimental? That will give us a way to publish and get feedback without committing to a final API

jelbourn · 2018-01-11T18:31:41Z

src/cdk/scrolling/for-of.ts

+ * container.
+ */
+@Directive({
+  selector: '[cdkFor][cdkForOf]',


I'd propose [cdkVirtualFor][cdkVirtualForOf]. Without the extra term, there's nothing that communicates how this is different from ngFor.

(with that name propagated throughout the rest of the code)

jelbourn · 2018-01-11T18:34:58Z

src/cdk/scrolling/for-of.ts

+
+  get even(): boolean { return this.index % 2 === 0; }
+
+  get odd(): boolean { return !this.even; }


For the table we avoided making these getters (CdkCellOutletRowContext) to avoid the relatively large payload size they generate, especially since these fields would only need to be written once. Avoiding a concrete class makes it a bit smaller, too.

jelbourn · 2018-01-11T19:41:07Z

src/cdk/scrolling/virtual-scroll-viewport.ts

+  get totalContentSize() { return this._totalContentSize; }
+  set totalContentSize(size: number) {
+    if (this._totalContentSize != size) {
+      this._ngZone.run(() => {


Comment that explains why you need to re-enter the zone here?
(and elsewhere)

jelbourn · 2018-01-11T19:45:11Z

src/cdk/scrolling/virtual-scroll-viewport.ts

+
+  /** The total size of all content, including content that is not currently rendered. */
+  get totalContentSize() { return this._totalContentSize; }
+  set totalContentSize(size: number) {


These properties shouldn't need to be be getter/setters since they're not @Inputs for the component (could have an update method, which is more obviously side-effecty)

jelbourn · 2018-01-11T19:45:48Z

src/cdk/scrolling/virtual-scroll-viewport.ts

+  /** A stream that emits whenever the rendered range changes. */
+  renderedRangeStream: Observable<Range> = this._renderedRangeSubject.asObservable();
+
+  _renderedContentTransform: string;


Description?

jelbourn · 2018-01-11T19:47:34Z

src/cdk/scrolling/virtual-scroll-viewport.ts

+              @Inject(VIRTUAL_SCROLL_STRATEGY) private _scrollStrategy: VirtualScrollStrategy) {}
+
+  /** Connect a `CdkForOf` to this viewport. */
+  connect(forOf: CdkForOf<any>) {


This isn't formalized anywhere, but I feel that attach and detach are more appropriate for components that interact than connect/disconnect

jelbourn · 2018-01-11T19:48:13Z

src/cdk/scrolling/virtual-scroll-viewport.ts

+    }
+
+    this._connected = true;
+    forOf.dataStream.pipe(takeUntil(this._disconnectSubject)).subscribe(data => {


Add a comment that explains what's happening here?

googlebot · 2018-01-16T22:19:01Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

amcdnl · 2018-01-18T23:32:12Z

src/cdk/scrolling/virtual-for-of.ts

+
+  ngDoCheck() {
+    if (this._differ && this._needsUpdate) {
+      const changes = this._differ.diff(this._renderedItems);


This is really pricey, is there a more reactive way we can do this updating?

I think we could differentiate between needing an update due to scrolling and rendering a new portion of the list (can do something simpler) vs needing an update due to the underlying data changing (in which case I think we'd have to do this diff). I added a TODO to add some logic for that. Also added a TODO to calculate the minimum set of remove & inserts since I'm currently just removing all of the views and re-inserting them in the right order

amcdnl · 2018-01-18T23:39:02Z

src/cdk/scrolling/virtual-scroll-viewport.ts

+    if (this._renderedContentOffset != offset) {
+      this._ngZone.run(() => {
+        this._renderedContentOffset = offset;
+        this._renderedContentTransform = this.orientation === 'horizontal' ?


Might want to do browser prefix test here.

We only have to support back to IE11 (+ latest 2 versions of other browsers) so should be ok

amcdnl · 2018-01-18T23:39:39Z

src/cdk/scrolling/virtual-scroll-viewport.ts

+        this._renderedContentOffset = offset;
+        this._renderedContentTransform = this.orientation === 'horizontal' ?
+            `translateX(${offset}px)`: `translateY(${offset}px)`;
+        this._changeDetectorRef.markForCheck();


I'd consider just doing detectChanges to get it to run immediately.

I've added a TODO to investigate this. I'd like to talk to some folks on the core team who know more about change detection than me

amcdnl · 2018-01-18T23:40:00Z

src/cdk/scrolling/virtual-scroll-viewport.ts

+    if (!this._rangesEqual(this._renderedRange, range)) {
+      this._ngZone.run(() => {
+        this._renderedRangeSubject.next(this._renderedRange = range);
+        this._changeDetectorRef.markForCheck();


I'd consider just doing detectChanges to get it to run immediately.

amcdnl · 2018-01-18T23:40:13Z

src/cdk/scrolling/virtual-scroll-viewport.ts

+    if (this._totalContentSize != size) {
+      this._ngZone.run(() => {
+        this._totalContentSize = size;
+        this._changeDetectorRef.markForCheck();


I'd consider just doing detectChanges to get it to run immediately.

I thought (and I could definitely be wrong) that it was better to just mark for check so that angular can go ahead and do these updates, together with any other updates that need to happen, at the same time and avoid thrashing.

@jelbourn @crisbeto do you guys know which way is best for performance?

In other projects, I was running markForCheck in a lot of places, when I switched it to detectChanges I saw a huge perf jump when scrolling.

amcdnl · 2018-01-18T23:43:38Z

src/cdk/scrolling/virtual-scroll-viewport.ts

+  get renderedContentOffset(): number { return this._renderedContentOffset; }
+  set renderedContentOffset(offset: number) {
+    if (this._renderedContentOffset != offset) {
+      this._ngZone.run(() => {


I'd try to avoid running this in a zone.

The way its set up right now I need to, so the markForCheck will work, but I'll investigate calling detectChanges directly

amcdnl · 2018-01-18T23:47:48Z

src/cdk/scrolling/virtual-scroll-fixed-size.ts

+})
+export class CdkVirtualScrollFixedSize implements OnChanges {
+  /** The size of the items in the list. */
+  @Input() itemSize = 20;


It would be nice if you could make this accept a function too where a user could pass a callback that accepted the current item they were iterating over to return a custom height for that element. For example:

itemSize = (item) => item.type === 'description' ? 50 : 20;

Yep that's something I want to add in a future PR

amcdnl · 2018-01-18T23:49:08Z

src/cdk/scrolling/virtual-for-of.ts

+   *     Or null if no DOM elements are rendered for the given index.
+   * @throws If the given index is not in the rendered range.
+   */
+  measureClientRect(index: number): ClientRect | null {


You might want to add a window resize function to update dims on window resizes.

Responding to resize is a feature I plan to add, but not in this PR

amcdnl · 2018-01-18T23:53:56Z

src/cdk/scrolling/virtual-for-of.ts

+
+  /** Update the `CdkVirtualForOfContext` for all views. */
+  private _updateContext() {
+    for (let i = 0, len = this._viewContainerRef.length; i < len; i++) {


Given the sensitivity of perf here, you might want to consider a faster loop technique. https://jsperf.com/loops/33

mmalerba · 2018-02-15T22:02:40Z

Thanks for the feedback everyone. I've put a number of TODOs for things I didn't get to in this PR. I'm going to merge this into the virtual-scroll feature branch now, but there will be plenty more chances to comment before it gets merged to master.

* feat(virtual-scroll): fixed size virtual scroll * address some comments * change VirtualScrollStrategy interface a bit * address some more comments * fix lint & build

* feat(virtual-scroll): fixed size virtual scroll (#9316) * feat(virtual-scroll): fixed size virtual scroll * address some comments * change VirtualScrollStrategy interface a bit * address some more comments * fix lint & build * chore: move virtual-scroll to cdk-experimental (#9974) * virtual-scroll: simplify scroll listener logic (#10102) * virtual-scroll: only move views that need to be moved (#10099) * virtual-scroll: only move views that need to be moved * address comments * virtual-scroll: switch `throttleTime` to `sampleTime` (#10179) * virtual-scroll: switch throttleTime to sampleTime * add comment * virtual-scroll: allow user to pass `Observable<T[]>` (#10158) * virtual-scroll: rename `Range` to `ListRange` to avoid confusion with native `Range` (#10220) * virtual-scroll: add autosize scroll strategy (#10219) * rename fixed size virtual scroll directive * add autosize virtual scroll strategy * add item size estimator class * add logic for jumping rendered content based on scroll position * address comments * virtual-scroll: add `onContentRendered` hook to `VirtualScrollStrategy` (#10290) * virtual-scroll: add `onContentRendered` hook to `VirtualScrollStrategy` * address comemnts * virtual-scroll: add incremental scroll logic in `AutosizeVirtualScrollStrategy` (#10504) * virtual-scroll: add incremental scroll logic in `AutosizeVirtualScrollStrategy`. This still has a couple issues that need to be ironed out and it doesn't have the code for correcting the error between the predicted and actual scroll position. (See various TODOs for additional things that need work). * fix lint * address comments * address comments * fix bazel * fix devapp * fix lint * cleanup * virtual-scroll: rewrite offset in terms of "to-top" and fix a bug where items were removed too soon (#10986) * rewrite offsets to the end of the rendered content as offsets to the start * add some more autosize demos for testing * make sure not to remove too many items * address comments * virtual-scroll: address amcdnl's feedback (#10988) * rewrite offsets to the end of the rendered content as offsets to the start * add some more autosize demos for testing * make sure not to remove too many items * virtual-scroll: address amcdnl's feedback * virtual-scroll: fix updating when data changes and add trackBy demos (#11085) * virtual-scroll: add logic to correct the scroll position as user move… (#11137) * virtual-scroll: add logic to correct the scroll position as user moves toward the top * address comments * fix(scrolling): adds right to fix pushed content (#11192) * fix(scrolling): adds right to fix pushed content * chore: address feedback * chore: address pr feedback * virtual-scroll: add tests for fixed size items (#11255) * virtual-scroll: add tests for fixed size items * address comments * fix bug in fixed size strategy * fix bazel build * virtual-scroll: add tests for `cdkVirtualFor` logic (#11275) * merge fixed size test components into one * add tests for cdkVirtualFor logic * allow undefined to be explicitly passed as trackBy * fix bazel build * address comments * add some basic tests (#11295) * fix lint * virtual-scroll: add e2e tests for autosize scroll strategy (#11345) * set up virtual scroll page in e2e app * add gulp task for e2e:watch * add e2e tests for autosize * address comments * address comments * fix lint and tests * fix bad rebase * fix(scrolling): scrollbar jump when data change on scroll (#11193) * fix(scrolling): data change not updating size * chore: remove todo * chore: more performant fix for jump

angular-automatic-lock-bot · 2019-09-08T23:01:42Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

mmalerba added the pr: needs review label Jan 10, 2018

mmalerba assigned jelbourn Jan 10, 2018

mmalerba requested a review from jelbourn January 10, 2018 00:48

mmalerba requested review from andrewseguin and crisbeto as code owners January 10, 2018 00:48

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 10, 2018

crisbeto reviewed Jan 10, 2018

View reviewed changes

CaerusKaru reviewed Jan 10, 2018

View reviewed changes

jelbourn reviewed Jan 11, 2018

View reviewed changes

mmalerba force-pushed the virtual-scroll-7 branch from 0cb50f0 to b678af4 Compare January 16, 2018 22:18

mmalerba requested review from amcdnl, devversion and josephperrott as code owners January 16, 2018 22:18

googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Jan 16, 2018

mmalerba force-pushed the virtual-scroll-7 branch 3 times, most recently from 48c0a70 to 767916b Compare January 18, 2018 23:39

mmalerba requested review from kara and tinayuangao as code owners January 18, 2018 23:39

mmalerba force-pushed the virtual-scroll-7 branch from 767916b to fd0f947 Compare January 18, 2018 23:50

amcdnl reviewed Jan 18, 2018

View reviewed changes

mmalerba force-pushed the virtual-scroll-7 branch 4 times, most recently from c88df3e to 63c6051 Compare January 24, 2018 01:25

mmalerba added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Jan 29, 2018

mmalerba force-pushed the virtual-scroll-7 branch from 63c6051 to 7b5dc1e Compare February 15, 2018 19:25

mmalerba added 5 commits February 15, 2018 11:33

feat(virtual-scroll): fixed size virtual scroll

250eaed

address some comments

eb1db06

change VirtualScrollStrategy interface a bit

6277388

address some more comments

d3aa780

fix lint & build

33cb26f

mmalerba force-pushed the virtual-scroll-7 branch from 7b5dc1e to 33cb26f Compare February 15, 2018 22:00

mmalerba merged commit 012384c into angular:virtual-scroll Feb 15, 2018

andrewseguin added the target: minor This PR is targeted for the next minor release label Feb 20, 2018

mmalerba added pr: lgtm and removed pr: needs review labels May 11, 2018

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019



		/** DataSource wrapper for a native array. */
		export class ArrayDataSource<T> implements DataSource<T> {


		get even(): boolean { return this.index % 2 === 0; }

		get odd(): boolean { return !this.even; }

feat(virtual-scroll): fixed size virtual scroll #9316

feat(virtual-scroll): fixed size virtual scroll #9316

Uh oh!

Conversation

mmalerba commented Jan 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Jan 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!