Update: Serially render thumbnail images (#901)

src/lib/ThumbnailsSidebar.js

@@ -49,6 +49,7 @@ class ThumbnailsSidebar {
         this.createThumbnailImage = this.createThumbnailImage.bind(this);
         this.generateThumbnailImages = this.generateThumbnailImages.bind(this);
         this.getThumbnailDataURL = this.getThumbnailDataURL.bind(this);
+        this.renderNextThumbnailImage = this.renderNextThumbnailImage.bind(this);
         this.requestThumbnailImage = this.requestThumbnailImage.bind(this);
         this.thumbnailClickHandler = this.thumbnailClickHandler.bind(this);
 
@@ -153,20 +154,29 @@ class ThumbnailsSidebar {
      * @param {Object} currentListInfo - VirtualScroller info object which contains startOffset, endOffset, and the thumbnail elements
      * @return {void}
      */
-    generateThumbnailImages({ items, startOffset }) {
-        if (!isFinite(startOffset) || startOffset < 0) {
-            return;
-        }
-
+    generateThumbnailImages({ items }) {
         this.currentThumbnails = items;
 
-        items.forEach((thumbnailEl, index) => {
-            if (thumbnailEl.classList.contains(CLASS_BOX_PREVIEW_THUMBNAIL_IMAGE_LOADED)) {
-                return;
-            }
+        // Serially renders the thumbnails one by one as needed
+        this.renderNextThumbnailImage();
+    }
 
-            this.requestThumbnailImage(index + startOffset, thumbnailEl);
-        });
+    /**
+     * Requests the next thumbnail image that needs rendering
+     *
+     * @return {void}
+     */
+    renderNextThumbnailImage() {
+        // Iterates over the current thumbnails and requests rendering of the first
+        // thumbnail it encounters that does not have an image loaded
+        const nextThumbnailEl = this.currentThumbnails.find(
+            (thumbnailEl) => !thumbnailEl.classList.contains(CLASS_BOX_PREVIEW_THUMBNAIL_IMAGE_LOADED)
+        );
+
+        if (nextThumbnailEl) {
+            const parsedPageNum = parseInt(nextThumbnailEl.dataset.bpPageNum, 10);
+            this.requestThumbnailImage(parsedPageNum - 1, nextThumbnailEl);
+        }
     }
 
     /**
@@ -202,8 +212,14 @@ class ThumbnailsSidebar {
     requestThumbnailImage(itemIndex, thumbnailEl) {
         requestAnimationFrame(() => {
             this.createThumbnailImage(itemIndex).then((imageEl) => {
-                thumbnailEl.appendChild(imageEl);
-                thumbnailEl.classList.add(CLASS_BOX_PREVIEW_THUMBNAIL_IMAGE_LOADED);
+                // Promise will resolve with null if create image request was already in progress
+                if (imageEl) {
+                    thumbnailEl.appendChild(imageEl);
+                    thumbnailEl.classList.add(CLASS_BOX_PREVIEW_THUMBNAIL_IMAGE_LOADED);
+                }
+
+                // After generating the thumbnail image, render the next one
+                this.renderNextThumbnailImage();
             });
         });
     }
@@ -212,19 +228,29 @@ class ThumbnailsSidebar {
      * Make a thumbnail image element
      *
      * @param {number} itemIndex - the item index for the overall list (0 indexed)
-     * @return {Promise} - promise reolves with the image HTMLElement
+     * @return {Promise} - promise reolves with the image HTMLElement or null if generation is in progress
      */
     createThumbnailImage(itemIndex) {
-        // If this page has already been cached, use it
-        if (this.thumbnailImageCache[itemIndex]) {
-            return Promise.resolve(this.thumbnailImageCache[itemIndex]);
+        const cacheEntry = this.thumbnailImageCache[itemIndex];
+
+        // If this thumbnail has already been cached, use it
+        if (cacheEntry && cacheEntry.image) {
+            return Promise.resolve(cacheEntry.image);
+        }
+
+        // If this thumbnail has already been requested, resolve with null
+        if (cacheEntry && cacheEntry.inProgress) {
+            return Promise.resolve(null);
         }
 
+        // Update the cache entry to be in progress
+        this.thumbnailImageCache[itemIndex] = { ...cacheEntry, inProgress: true };
+
         return this.getThumbnailDataURL(itemIndex + 1)
             .then(this.createImageEl)
             .then((imageEl) => {
                 // Cache this image element for future use
-                this.thumbnailImageCache[itemIndex] = imageEl;
+                this.thumbnailImageCache[itemIndex] = { inProgress: false, image: imageEl };
 
                 return imageEl;
             });

src/lib/VirtualScroller.js

@@ -289,6 +289,8 @@ class VirtualScroller {
             //  |--------------------|
             // newStartOffset    newEndOffset
             this.createItems(fragment, newStartOffset, newEndOffset);
+            // Delete all the current elements (if any)
+            this.deleteItems(this.listEl);
             this.listEl.appendChild(fragment);
         }
     }
@@ -316,11 +318,11 @@ class VirtualScroller {
      * Deletes elements of the 'ol'
      *
      * @param {HTMLElement} listEl - the `ol` element
-     * @param {number} start - start index
+     * @param {number} [start] - start index
      * @param {number} [end] - end index
      * @return {void}
      */
-    deleteItems(listEl, start, end) {
+    deleteItems(listEl, start = 0, end) {
         if (!listEl || start < 0 || end < 0) {
             return;
         }

src/lib/__tests__/ThumbnailsSidebar-test.js

@@ -140,35 +140,53 @@ describe('ThumbnailsSidebar', () => {
         });
     });
 
-    describe('generateThumbnailImage()', () => {
+    describe('renderNextThumbnailImage()', () => {
         beforeEach(() => {
             stubs.requestThumbnailImage = sandbox.stub(thumbnailsSidebar, 'requestThumbnailImage');
         });
 
-        it('should do nothing if startOffset is -1', () => {
-            thumbnailsSidebar.generateThumbnailImages({ items: [], startOffset: -1 });
+        // eslint-disable-next-line
+        const createThumbnailEl = (pageNum, contains) => {
+            return {
+                classList: {
+                    contains: () => contains
+                },
+                dataset: {
+                    bpPageNum: pageNum
+                }
+            };
+        };
+
+        it('should do nothing there are no current thumbnails', () => {
+            thumbnailsSidebar.currentThumbnails = [];
+            thumbnailsSidebar.renderNextThumbnailImage();
 
             expect(stubs.requestThumbnailImage).not.to.be.called;
         });
 
         it('should not request thumbnail images if thumbnail already contains image loaded class', () => {
-            stubs.contains = sandbox.stub().returns(true);
-
-            const items = [{ classList: { contains: stubs.contains } }];
+            const items = [createThumbnailEl(1, true)];
+            thumbnailsSidebar.currentThumbnails = items;
 
-            thumbnailsSidebar.generateThumbnailImages({ items, startOffset: 0 });
+            thumbnailsSidebar.renderNextThumbnailImage();
 
             expect(stubs.requestThumbnailImage).not.to.be.called;
         });
 
         it('should request thumbnail images if thumbnail does not already contains image loaded class', () => {
-            stubs.contains = sandbox.stub().returns(false);
+            const items = [createThumbnailEl(1, false)];
+            thumbnailsSidebar.currentThumbnails = items;
+            thumbnailsSidebar.renderNextThumbnailImage();
 
-            const items = [{ classList: { contains: stubs.contains } }];
+            expect(stubs.requestThumbnailImage).to.be.calledOnce;
+        });
 
-            thumbnailsSidebar.generateThumbnailImages({ items, startOffset: 0 });
+        it('should only request the first thumbnail that does not already contain an image loaded class', () => {
+            const items = [createThumbnailEl(1, true), createThumbnailEl(2, false), createThumbnailEl(3, false)];
+            thumbnailsSidebar.currentThumbnails = items;
+            thumbnailsSidebar.renderNextThumbnailImage();
 
-            expect(stubs.requestThumbnailImage).to.be.called;
+            expect(stubs.requestThumbnailImage).to.be.calledOnce;
         });
     });
 
@@ -206,7 +224,7 @@ describe('ThumbnailsSidebar', () => {
 
         it('should resolve immediately if the image is in cache', () => {
             const cachedImage = {};
-            thumbnailsSidebar.thumbnailImageCache = { 1: cachedImage };
+            thumbnailsSidebar.thumbnailImageCache = { 1: { image: cachedImage } };
 
             return thumbnailsSidebar.createThumbnailImage(1).then(() => {
                 expect(stubs.createImageEl).not.to.be.called;
@@ -215,12 +233,24 @@ describe('ThumbnailsSidebar', () => {
 
         it('should create an image element if not in cache', () => {
             const cachedImage = {};
-            thumbnailsSidebar.thumbnailImageCache = { 1: cachedImage };
+            thumbnailsSidebar.thumbnailImageCache = { 1: { image: cachedImage } };
             stubs.createImageEl.returns(cachedImage);
 
             return thumbnailsSidebar.createThumbnailImage(0).then((imageEl) => {
                 expect(stubs.createImageEl).to.be.called;
-                expect(thumbnailsSidebar.thumbnailImageCache[0]).to.be.eql(imageEl);
+                expect(thumbnailsSidebar.thumbnailImageCache[0].image).to.be.eql(imageEl);
+                expect(thumbnailsSidebar.thumbnailImageCache[0].inProgress).to.be.false;
+            });
+        });
+
+        it('should resolve with null if cache entry inProgress is true', () => {
+            const cachedImage = {};
+            thumbnailsSidebar.thumbnailImageCache = { 0: { inProgress: true } };
+            stubs.createImageEl.returns(cachedImage);
+
+            return thumbnailsSidebar.createThumbnailImage(0).then((imageEl) => {
+                expect(stubs.createImageEl).not.to.be.called;
+                expect(imageEl).to.be.null;
             });
         });
     });

src/lib/__tests__/VirtualScroller-test.js

@@ -187,7 +187,7 @@ describe('VirtualScroller', () => {
             });
             virtualScroller.renderItems();
 
-            expect(stubs.deleteItems).not.to.be.called;
+            expect(stubs.deleteItems).to.be.calledWith(curListEl);
             expect(stubs.createItems).to.be.calledWith(newListEl, 0, 10);
             expect(stubs.appendChild).to.be.called;
             expect(stubs.insertBefore).not.to.be.called;
@@ -202,7 +202,7 @@ describe('VirtualScroller', () => {
             });
             virtualScroller.renderItems(95);
 
-            expect(stubs.deleteItems).not.to.be.called;
+            expect(stubs.deleteItems).to.be.calledWith(curListEl);
             expect(stubs.createItems).to.be.calledWith(newListEl, 95, 99);
             expect(stubs.appendChild).to.be.called;
             expect(stubs.insertBefore).not.to.be.called;