From 1ded729130ead6b11009f9995e2ff1087508150d Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 14 Feb 2019 18:13:23 +0100 Subject: [PATCH 1/2] Simplify the `updatetextlayermatches` event handling in `TextLayerBuilder` This implements the nice suggestion from https://github.com/mozilla/pdf.js/pull/10099#discussion_r219698000, which I at the time didn't think would work. I'll probably have to plead temporary insanity here, since it *should* have been totally obvious to me how this could be implemented. By simply not registering the event until the `textLayer` is actually rendered, removing the event on `cancel` works just fine. This patch also removes the `pagecancelled` event, given that it's no longer used anywhere in the code-base and that its implemention was flawed since it wasn't guaranteed to be called in *every* situation where rendering was actually cancelled. --- web/pdf_page_view.js | 10 -------- web/text_layer_builder.js | 53 ++++++++++++--------------------------- 2 files changed, 16 insertions(+), 47 deletions(-) diff --git a/web/pdf_page_view.js b/web/pdf_page_view.js index 3108d4c2c30d1..bd67134b66d7d 100644 --- a/web/pdf_page_view.js +++ b/web/pdf_page_view.js @@ -259,8 +259,6 @@ class PDFPageView { } cancelRendering(keepAnnotations = false) { - const renderingState = this.renderingState; - if (this.paintTask) { this.paintTask.cancel(); this.paintTask = null; @@ -276,14 +274,6 @@ class PDFPageView { this.annotationLayer.cancel(); this.annotationLayer = null; } - - if (renderingState !== RenderingStates.INITIAL) { - this.eventBus.dispatch('pagecancelled', { - source: this, - pageNumber: this.id, - renderingState, - }); - } } cssTransform(target, redrawAnnotations = false) { diff --git a/web/text_layer_builder.js b/web/text_layer_builder.js index e09819115b87a..6a59e5527c9f9 100644 --- a/web/text_layer_builder.js +++ b/web/text_layer_builder.js @@ -53,9 +53,7 @@ class TextLayerBuilder { this.textLayerRenderTask = null; this.enhanceTextSelection = enhanceTextSelection; - this._boundEvents = Object.create(null); - this._bindEvents(); - + this._onUpdateTextLayerMatches = null; this._bindMouse(); } @@ -109,6 +107,16 @@ class TextLayerBuilder { }, function (reason) { // Cancelled or failed to render text layer; skipping errors. }); + + if (!this._onUpdateTextLayerMatches) { + this._onUpdateTextLayerMatches = (evt) => { + if (evt.pageIndex === this.pageIdx || evt.pageIndex === -1) { + this._updateMatches(); + } + }; + this.eventBus.on('updatetextlayermatches', + this._onUpdateTextLayerMatches); + } } /** @@ -119,6 +127,11 @@ class TextLayerBuilder { this.textLayerRenderTask.cancel(); this.textLayerRenderTask = null; } + if (this._onUpdateTextLayerMatches) { + this.eventBus.off('updatetextlayermatches', + this._onUpdateTextLayerMatches); + this._onUpdateTextLayerMatches = null; + } } setTextContentStream(readableStream) { @@ -314,40 +327,6 @@ class TextLayerBuilder { this._renderMatches(this.matches); } - /** - * @private - */ - _bindEvents() { - const { eventBus, _boundEvents, } = this; - - _boundEvents.pageCancelled = (evt) => { - if (evt.pageNumber !== this.pageNumber) { - return; - } - if (this.textLayerRenderTask) { - console.error('TextLayerBuilder._bindEvents: `this.cancel()` should ' + - 'have been called when the page was reset, or rendering cancelled.'); - return; - } - // Ensure that all event listeners are cleaned up when the page is reset, - // since re-rendering will create new `TextLayerBuilder` instances and the - // number of (stale) event listeners would otherwise grow without bound. - for (const name in _boundEvents) { - eventBus.off(name.toLowerCase(), _boundEvents[name]); - delete _boundEvents[name]; - } - }; - _boundEvents.updateTextLayerMatches = (evt) => { - if (evt.pageIndex !== this.pageIdx && evt.pageIndex !== -1) { - return; - } - this._updateMatches(); - }; - - eventBus.on('pagecancelled', _boundEvents.pageCancelled); - eventBus.on('updatetextlayermatches', _boundEvents.updateTextLayerMatches); - } - /** * Improves text selection by adding an additional div where the mouse was * clicked. This reduces flickering of the content if the mouse is slowly From 599dcf5a44c52404cf54a5670283017b95bc3b4b Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 14 Feb 2019 18:47:32 +0100 Subject: [PATCH 2/2] Fix `{PDFPageView, PDFThumbnailView}.cancelRendering` to avoid visual artifacts when called directly Currently these methods are only used from the respective `reset` methods, and from `{BaseViewer, PDFThumbnailViewer}._cancelRendering` which only runs when the active document is closed. This patch changes `{PDFPageView, PDFThumbnailView}.cancelRendering` to *only* cancel any pending rendering operations, and doesn't attempt to reset e.g. the `renderingState`, since that causes visual glitches (duplicated canvases in the viewer) when called directly. Furthermore, unless you "know" what you're doing, the `{PDFPageView, PDFThumbnailView}.reset` methods are what *should* normally be used instead. --- web/pdf_page_view.js | 6 +++++- web/pdf_thumbnail_view.js | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/web/pdf_page_view.js b/web/pdf_page_view.js index bd67134b66d7d..a3d651a44d48d 100644 --- a/web/pdf_page_view.js +++ b/web/pdf_page_view.js @@ -154,6 +154,7 @@ class PDFPageView { reset(keepZoomLayer = false, keepAnnotations = false) { this.cancelRendering(keepAnnotations); + this.renderingState = RenderingStates.INITIAL; let div = this.div; div.style.width = Math.floor(this.viewport.width) + 'px'; @@ -258,12 +259,15 @@ class PDFPageView { this.reset(/* keepZoomLayer = */ true, /* keepAnnotations = */ true); } + /** + * PLEASE NOTE: Most likely you want to use the `this.reset()` method, + * rather than calling this one directly. + */ cancelRendering(keepAnnotations = false) { if (this.paintTask) { this.paintTask.cancel(); this.paintTask = null; } - this.renderingState = RenderingStates.INITIAL; this.resume = null; if (this.textLayer) { diff --git a/web/pdf_thumbnail_view.js b/web/pdf_thumbnail_view.js index 0144000fd083c..601ecad7a39cb 100644 --- a/web/pdf_thumbnail_view.js +++ b/web/pdf_thumbnail_view.js @@ -152,6 +152,7 @@ class PDFThumbnailView { reset() { this.cancelRendering(); + this.renderingState = RenderingStates.INITIAL; this.pageWidth = this.viewport.width; this.pageHeight = this.viewport.height; @@ -195,12 +196,15 @@ class PDFThumbnailView { this.reset(); } + /** + * PLEASE NOTE: Most likely you want to use the `this.reset()` method, + * rather than calling this one directly. + */ cancelRendering() { if (this.renderTask) { this.renderTask.cancel(); this.renderTask = null; } - this.renderingState = RenderingStates.INITIAL; this.resume = null; }