Skip to content

Commit

Permalink
fix(virtual-scroll): move views that are already attached instead of …
Browse files Browse the repository at this point in the history 
…inserting
  • Loading branch information
@kara
kara committed Mar 1, 2019
1 parent ae41a0a commit cc8a266
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 36 deletions.
54 changes: 38 additions & 16 deletions src/cdk/scrolling/virtual-for-of.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -293,12 +293,10 @@ export class CdkVirtualForOf<T> implements CollectionViewer, DoCheck, OnDestroy
adjustedPreviousIndex: number | null,
currentIndex: number | null) => {
if (record.previousIndex == null) { // Item added.
const view = this._getViewForNewItem();
this._viewContainerRef.insert(view, currentIndex!);
const view = this._insertViewForNewItem(currentIndex!);
view.context.$implicit = record.item;
} else if (currentIndex == null) { // Item removed.
this._cacheView(this._viewContainerRef.detach(adjustedPreviousIndex!) as
EmbeddedViewRef<CdkVirtualForOfContext<T>>);
this._cacheView(this._detachView(adjustedPreviousIndex !));
} else { // Item moved.
const view = this._viewContainerRef.get(adjustedPreviousIndex!) as
EmbeddedViewRef<CdkVirtualForOfContext<T>>;
Expand Down Expand Up @@ -343,18 +341,9 @@ export class CdkVirtualForOf<T> implements CollectionViewer, DoCheck, OnDestroy
}
}

/** Get a view for a new item, either from the cache or by creating a new one. */
private _getViewForNewItem(): EmbeddedViewRef<CdkVirtualForOfContext<T>> {
return this._templateCache.pop() || this._viewContainerRef.createEmbeddedView(this._template, {
$implicit: null!,
cdkVirtualForOf: this._cdkVirtualForOf,
index: -1,
count: -1,
first: false,
last: false,
odd: false,
even: false
});
/** Inserts a view for a new item, either from the cache or by creating a new one. */
private _insertViewForNewItem(index: number): EmbeddedViewRef<CdkVirtualForOfContext<T>> {
return this._insertViewFromCache(index) || this._createEmbeddedViewAt(index);
}

/** Update the computed properties on the `CdkVirtualForOfContext`. */
Expand All @@ -364,4 +353,37 @@ export class CdkVirtualForOf<T> implements CollectionViewer, DoCheck, OnDestroy
context.even = context.index % 2 === 0;
context.odd = !context.even;
}

/** Creates a new embedded view and moves it to the given index */
private _createEmbeddedViewAt(index: number): EmbeddedViewRef<CdkVirtualForOfContext<T>> {
const view = this._viewContainerRef.createEmbeddedView(this._template, {
$implicit: null!,
cdkVirtualForOf: this._cdkVirtualForOf,
index: -1,
count: -1,
first: false,
last: false,
odd: false,
even: false
});
if (index < this._viewContainerRef.length) {
this._viewContainerRef.move(view, index);
}
return view;
}

/** Inserts a recycled view from the cache at the given index. */
private _insertViewFromCache(index: number): EmbeddedViewRef<CdkVirtualForOfContext<T>>|null {
const cachedView = this._templateCache.pop();
if (cachedView) {
this._viewContainerRef.insert(cachedView, index);
}
return cachedView || null;
}

/** Detaches the embedded view at the given index. */
private _detachView(index: number): EmbeddedViewRef<CdkVirtualForOfContext<T>> {
return this._viewContainerRef.detach(index) as
EmbeddedViewRef<CdkVirtualForOfContext<T>>;
}
}
45 changes: 25 additions & 20 deletions src/cdk/scrolling/virtual-scroll-viewport.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import {
NgZone,
TrackByFunction,
ViewChild,
ViewContainerRef,
ViewEncapsulation
} from '@angular/core';
import {async, ComponentFixture, fakeAsync, flush, inject, TestBed} from '@angular/core/testing';
Expand Down Expand Up @@ -488,49 +487,51 @@ describe('CdkVirtualScrollViewport', () => {

it('should trackBy value by default', fakeAsync(() => {
testComponent.items = [];
spyOn(testComponent.virtualForViewContainer, 'detach').and.callThrough();
spyOn<any>(testComponent.virtualForOf, '_detachView').and.callThrough();
finishInit(fixture);

testComponent.items = [0];
fixture.detectChanges();
flush();

expect(testComponent.virtualForViewContainer.detach).not.toHaveBeenCalled();
expect((testComponent.virtualForOf as any)._detachView).not.toHaveBeenCalled();

testComponent.items = [1];
fixture.detectChanges();
flush();

expect(testComponent.virtualForViewContainer.detach).toHaveBeenCalled();
expect((testComponent.virtualForOf as any)._detachView).toHaveBeenCalled();
}));

it('should trackBy index when specified', fakeAsync(() => {
testComponent.trackBy = i => i;
testComponent.items = [];
spyOn(testComponent.virtualForViewContainer, 'detach').and.callThrough();
spyOn<any>(testComponent.virtualForOf, '_detachView').and.callThrough();
finishInit(fixture);

testComponent.items = [0];
fixture.detectChanges();
flush();

expect(testComponent.virtualForViewContainer.detach).not.toHaveBeenCalled();
expect((testComponent.virtualForOf as any)._detachView).not.toHaveBeenCalled();

testComponent.items = [1];
fixture.detectChanges();
flush();

expect(testComponent.virtualForViewContainer.detach).not.toHaveBeenCalled();
expect((testComponent.virtualForOf as any)._detachView).not.toHaveBeenCalled();
}));

it('should recycle views when template cache is large enough to accommodate', fakeAsync(() => {
testComponent.trackBy = i => i;
const spy =
spyOn(testComponent.virtualForViewContainer, 'createEmbeddedView').and.callThrough();
const spy = spyOn<any>(testComponent.virtualForOf, '_createEmbeddedViewAt')
.and.callThrough();

finishInit(fixture);

// Should create views for the initial rendered items.
expect(testComponent.virtualForViewContainer.createEmbeddedView).toHaveBeenCalledTimes(4);
expect((testComponent.virtualForOf as any)._createEmbeddedViewAt)
.toHaveBeenCalledTimes(4);

spy.calls.reset();
triggerScroll(viewport, 10);
Expand All @@ -540,7 +541,8 @@ describe('CdkVirtualScrollViewport', () => {
// As we first start to scroll we need to create one more item. This is because the first item
// is still partially on screen and therefore can't be removed yet. At the same time a new
// item is now partially on the screen at the bottom and so a new view is needed.
expect(testComponent.virtualForViewContainer.createEmbeddedView).toHaveBeenCalledTimes(1);
expect((testComponent.virtualForOf as any)._createEmbeddedViewAt)
.toHaveBeenCalledTimes(1);

spy.calls.reset();
const maxOffset =
Expand All @@ -553,18 +555,21 @@ describe('CdkVirtualScrollViewport', () => {

// As we scroll through the rest of the items, no new views should be created, our existing 5
// can just be recycled as appropriate.
expect(testComponent.virtualForViewContainer.createEmbeddedView).not.toHaveBeenCalled();
expect((testComponent.virtualForOf as any)._createEmbeddedViewAt)
.not.toHaveBeenCalled();
}));

it('should not recycle views when template cache is full', fakeAsync(() => {
testComponent.trackBy = i => i;
testComponent.templateCacheSize = 0;
const spy =
spyOn(testComponent.virtualForViewContainer, 'createEmbeddedView').and.callThrough();
finishInit(fixture);
const spy = spyOn<any>(testComponent.virtualForOf, '_createEmbeddedViewAt')
.and.callThrough();

finishInit(fixture);

// Should create views for the initial rendered items.
expect(testComponent.virtualForViewContainer.createEmbeddedView).toHaveBeenCalledTimes(4);
expect((testComponent.virtualForOf as any)._createEmbeddedViewAt)
.toHaveBeenCalledTimes(4);

spy.calls.reset();
triggerScroll(viewport, 10);
Expand All @@ -574,7 +579,8 @@ describe('CdkVirtualScrollViewport', () => {
// As we first start to scroll we need to create one more item. This is because the first item
// is still partially on screen and therefore can't be removed yet. At the same time a new
// item is now partially on the screen at the bottom and so a new view is needed.
expect(testComponent.virtualForViewContainer.createEmbeddedView).toHaveBeenCalledTimes(1);
expect((testComponent.virtualForOf as any)._createEmbeddedViewAt)
.toHaveBeenCalledTimes(1);

spy.calls.reset();
const maxOffset =
Expand All @@ -587,7 +593,8 @@ describe('CdkVirtualScrollViewport', () => {

// Since our template cache size is 0, as we scroll through the rest of the items, we need to
// create a new view for each one.
expect(testComponent.virtualForViewContainer.createEmbeddedView).toHaveBeenCalledTimes(5);
expect((testComponent.virtualForOf as any)._createEmbeddedViewAt)
.toHaveBeenCalledTimes(5);
}));

it('should render up to maxBufferPx when buffer dips below minBufferPx', fakeAsync(() => {
Expand Down Expand Up @@ -831,7 +838,6 @@ function triggerScroll(viewport: CdkVirtualScrollViewport, offset?: number) {
class FixedSizeVirtualScroll {
@ViewChild(CdkVirtualScrollViewport) viewport: CdkVirtualScrollViewport;
@ViewChild(CdkVirtualForOf) virtualForOf: CdkVirtualForOf<any>;
@ViewChild(CdkVirtualForOf, {read: ViewContainerRef}) virtualForViewContainer: ViewContainerRef;

@Input() orientation = 'vertical';
@Input() viewportSize = 200;
Expand Down Expand Up @@ -882,7 +888,6 @@ class FixedSizeVirtualScroll {
})
class FixedSizeVirtualScrollWithRtlDirection {
@ViewChild(CdkVirtualScrollViewport) viewport: CdkVirtualScrollViewport;
@ViewChild(CdkVirtualForOf, {read: ViewContainerRef}) virtualForViewContainer: ViewContainerRef;

@Input() orientation = 'vertical';
@Input() viewportSize = 200;
Expand Down

0 comments on commit cc8a266

Please sign in to comment.