Allow for unbinding of events in web application #8525

curiosity26 · 2017-06-13T21:19:06Z

Allows for single page apps to unbind listeners when DOM element is destroyed in #8524

yurydelendik · 2017-06-13T21:39:36Z

web/app.js

+
+    eventBus.off('resize', webViewerResize);
+    eventBus.off('hashchange', webViewerHashchange);
+    eventBus.off('beforeprint', this.beforePrint.bind(this));


.bind() returns different function every time it is called, but eventBus.on and .off require the same method to be used.

yurydelendik · 2017-06-13T21:40:48Z

web/app.js

+    window.removeEventListener('click', webViewerClick);
+    window.removeEventListener('keydown', webViewerKeyDown);
+
+    window.removeEventListener('resize', function windowResize() {


the methods windowResize is scoped only here and was not used for addEventListener.

yurydelendik

The off() and removeEventListener() methods shall use the exact same methods that where used in on() and addEventListener().

curiosity26 · 2017-06-13T22:29:54Z

Thanks Yury for the feedback. I'll make sure I cleanup those event listeners. I must be a little brain dead today because as I look at it now, it's glaringly obvious.

Snuffleupagus · 2017-06-14T08:39:49Z

web/app.js

@@ -1330,6 +1330,78 @@ var PDFViewerApplication = {
      });
    }
  },
+
+  unbindEvents: function pdfViewBindEvents() {


Since we support ES6 now, please use unbindEvents() { instead here (and similar in the method signature below as well).

Snuffleupagus · 2017-06-14T08:40:28Z

web/app.js

@@ -1330,6 +1330,78 @@ var PDFViewerApplication = {
      });
    }
  },
+
+  unbindEvents: function pdfViewBindEvents() {
+    var eventBus = this.eventBus;


Please use let instead of var when adding new code (applies elsewhere in the patch as well).

Snuffleupagus · 2017-06-14T08:40:48Z

web/app.js

+    eventBus.off('find', webViewerFind);
+    eventBus.off('findfromurlhash', webViewerFindFromUrlHash);
+    if (typeof PDFJSDev === 'undefined' || PDFJSDev.test('GENERIC')) {
+        eventBus.off('fileinputchange', webViewerFileInputChange);


Please correct the indentation, since the standard in PDF.js is two spaces for indentation (applies in multiple places below as well).

yurydelendik · 2017-06-14T13:40:10Z

web/app.js

@@ -1300,34 +1309,44 @@ var PDFViewerApplication = {
  bindWindowEvents: function pdfViewBindWindowEvents() {
    var eventBus = this.eventBus;

+    // Window Resize
+    this._boundEvents.windowResize = (function () {


let's prefer ES6 syntax here, e.g.:

this._boundEvents.windowResize = () => { this.dispatch('resize'); };

yurydelendik · 2017-06-14T13:42:39Z

web/app.js

+      this.dispatch('afterprint');
+    }).bind(eventBus);
+    // Window Change
+    this._boundEvents.windowChange = (function (evt) {


this code shall not be present in non-GENERIC build, can we keep this._boundEvents assignments interleaved with addEventListener

yurydelendik · 2017-06-14T13:43:52Z

web/app.js

    }
+
+    delete this._boundEvents.windowResize;


Don't delete, just reset _boundEvents this._boundEvents = {}; at the end.

P.S. Or individual assignments this._boundEvents.windowHashChange = null;

Snuffleupagus · 2017-06-14T13:45:24Z

web/app.js

+   * Hold Event Listeners bound to an object here to be unbound later
+   * @type {{}}
+   * @private
+   */


I'm not sure that we need such a verbose comment here (or a comment at all really).

Snuffleupagus · 2017-06-14T13:45:56Z

web/app.js

+  bindWindowEvents() {
+    let eventBus = this.eventBus;
+
+    // Window Resize


These comments doesn't seem to add much value here!?

Snuffleupagus · 2017-06-14T13:47:46Z

web/app.js

+    // Window Resize
+    this._boundEvents.windowResize = (function () {
+      this.dispatch('resize');
+    }).bind(eventBus);


Please don't use bind needlessly; I think the following ought to work instead:

this._boundEvents.windowResize = () => { eventBus.dispatch('resize'); };

eventListener not eventBus?

Snuffleupagus · 2017-06-14T13:48:48Z

web/app.js

-          fileInput: evt.target,
-        });
-      });
+        eventBus.off('fileinputchange', webViewerFileInputChange);


Here, and elsewhere, please remember to use two spaces when indenting.

Snuffleupagus · 2017-06-14T13:51:38Z

Also, please remember to squash the commits when addressing review comments; see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits for information on this matter.

Snuffleupagus · 2017-06-14T14:18:35Z

web/app.js

-          fileInput: evt.target,
-        });
+    };
+    this._boundEvents.windowChange = (evt) => {


#8525 (comment) still appears to be unresolved in the latest version of the patch.

Snuffleupagus · 2017-06-14T14:19:15Z

web/app.js

+        delete this._boundEvents.windowChange;
+    }
+
+    delete this._boundEvents.windowResize;


#8525 (comment) still appears to be unresolved in the latest version of the patch.

Snuffleupagus · 2017-06-14T14:20:21Z

web/app.js

+    window.removeEventListener('click', webViewerClick);
+    window.removeEventListener('keydown', webViewerKeyDown);
+    window.removeEventListener('resize',
+        this._boundEvents.windowResize);


Nit: More indentation issues here, and below.
(And please don't forget to squash the commits.)

Hold bound event listeners for later unbinding ES6 styling More ES6 styling and code cleanup Remove 4 space indents and remove delete statements.

timvandermeij · 2017-06-15T21:15:00Z

/botio-linux preview

pdfjsbot · 2017-06-15T21:15:00Z

From: Bot.io (Linux m4)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/59996c6877a990c/output.txt

pdfjsbot · 2017-06-15T21:17:14Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/59996c6877a990c/output.txt

Total script time: 2.22 mins

Published

timvandermeij · 2017-06-17T20:32:26Z

Thank you for the patch!

Allow for unbinding of events in web application

timvandermeij added the viewer label Jun 13, 2017

yurydelendik reviewed Jun 13, 2017

View reviewed changes

yurydelendik requested changes Jun 13, 2017

View reviewed changes

yurydelendik mentioned this pull request Jun 13, 2017

DOM events called multiple times for each occurrence of PDFJS.webViewerLoad() #8524

Closed

Snuffleupagus reviewed Jun 14, 2017

View reviewed changes

yurydelendik reviewed Jun 14, 2017

View reviewed changes

Snuffleupagus reviewed Jun 14, 2017

View reviewed changes

Allow for unbinding of events in web application

8326304

Hold bound event listeners for later unbinding ES6 styling More ES6 styling and code cleanup Remove 4 space indents and remove delete statements.

curiosity26 force-pushed the master branch 2 times, most recently from c60fd63 to 8326304 Compare June 15, 2017 14:05

yurydelendik approved these changes Jun 15, 2017

View reviewed changes

timvandermeij approved these changes Jun 17, 2017

View reviewed changes

timvandermeij merged commit 7215e9c into mozilla:master Jun 17, 2017

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#8525 from curiosity26/master

0eeb987

Allow for unbinding of events in web application

Snuffleupagus mentioned this pull request May 7, 2022

Stop bundling PDFViewerApplication.{unbindEvents, unbindWindowEvents} in MOZCENTRAL builds #14885

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for unbinding of events in web application #8525

Allow for unbinding of events in web application #8525

curiosity26 commented Jun 13, 2017

yurydelendik Jun 13, 2017

yurydelendik Jun 13, 2017

yurydelendik left a comment

curiosity26 commented Jun 13, 2017

Snuffleupagus Jun 14, 2017

Snuffleupagus Jun 14, 2017

Snuffleupagus Jun 14, 2017

yurydelendik Jun 14, 2017 •

edited

Loading

yurydelendik Jun 14, 2017

yurydelendik Jun 14, 2017 •

edited

Loading

Snuffleupagus Jun 14, 2017

Snuffleupagus Jun 14, 2017

Snuffleupagus Jun 14, 2017 •

edited

Loading

curiosity26 Jun 14, 2017

Snuffleupagus Jun 14, 2017

Snuffleupagus commented Jun 14, 2017

Snuffleupagus Jun 14, 2017

Snuffleupagus Jun 14, 2017

Snuffleupagus Jun 14, 2017

timvandermeij commented Jun 15, 2017

pdfjsbot commented Jun 15, 2017

pdfjsbot commented Jun 15, 2017

timvandermeij commented Jun 17, 2017

Allow for unbinding of events in web application #8525

Allow for unbinding of events in web application #8525

Conversation

curiosity26 commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik left a comment

Choose a reason for hiding this comment

curiosity26 commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented Jun 15, 2017

pdfjsbot commented Jun 15, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 15, 2017

From: Bot.io (Linux m4)

Success

Published

timvandermeij commented Jun 17, 2017

yurydelendik Jun 14, 2017 •

edited

Loading

yurydelendik Jun 14, 2017 •

edited

Loading

Snuffleupagus Jun 14, 2017 •

edited

Loading