[Editor] Dispatch an event when some global states are changing (bug 1777695) #15130

calixteman · 2022-07-04T16:08:19Z

this way the context menu in Firefox can take into account what we
have in the clipboard, if an editor is selected, ...
when the user will click on a context menu item, an action will be
triggered, hence this patch adds what is required to handle it;
some tests will be added in the Firefox' patch.

Snuffleupagus · 2022-07-04T17:26:26Z

src/display/editor/editor.js

+    if (event.button === 2) {
+      // Avoid to focus this editor because of a right click.


Do we actually want to focus it for a middle-click, i.e. (usually) the scroll-wheel on a mouse, or do we perhaps want to avoid it receiving focus for event.button !== 0 instead?

Snuffleupagus · 2022-07-04T17:29:34Z

src/display/editor/tools.js

+    switch (details.name) {
+      case "undo":
+        this.undo();
+        break;
+      case "redo":
+        this.redo();
+        break;
+      case "cut":
+        this.cut();
+        break;
+      case "copy":
+        this.copy();
+        break;
+      case "paste":
+        this.paste();
+        break;
+      case "delete":
+        this.delete();
+        break;
+      case "selectAll":
+        this.selectAll();
+        break;
+    }


Given that the action-names seem to perfectly mimic the method-names, could we perhaps avoid a bit of duplication here?

Suggested change

switch (details.name) {

case "undo":

this.undo();

break;

case "redo":

this.redo();

break;

case "cut":

this.cut();

break;

case "copy":

this.copy();

break;

case "paste":

this.paste();

break;

case "delete":

this.delete();

break;

case "selectAll":

this.selectAll();

break;

}

this[details.name]?.();

Snuffleupagus · 2022-07-04T17:30:02Z

src/display/editor/tools.js

+   * @param {Object} details
+   */
+  #dispatchUpdateStates(details) {
+    const hasChange = Object.entries(details).some(


Nit:

Suggested change

const hasChange = Object.entries(details).some(

const hasChanged = Object.entries(details).some(

Snuffleupagus · 2022-07-04T17:30:27Z

src/display/editor/tools.js

@@ -325,6 +416,29 @@ class AnnotationEditorUIManager {
    });
  }

+  /**
+   * Set the editing state.
+   * It can be useful to temporarly disable it when the user is editing a


Spelling-nit:

Suggested change

* It can be useful to temporarly disable it when the user is editing a

* It can be useful to temporarily disable it when the user is editing a

Snuffleupagus · 2022-07-04T17:35:27Z

src/display/editor/tools.js

+      return Array.from(this.#allEditors.values()).every(editor =>
+        editor.isEmpty()
+      );


Nit: While it probably doesn't matter much, performance wise, it seems that this code will lead to essentially two loops and one (strictly speaking) unnecessary Array allocation.
Would it make sense to do something like the following instead?

Suggested change

return Array.from(this.#allEditors.values()).every(editor =>

editor.isEmpty()

);

for (const editor of this.#allEditors.values()) {

if (!editor.isEmpty()) {

return false;

}

}

return true;

Snuffleupagus · 2022-07-04T17:37:23Z

web/app.js

+    if (typeof PDFJSDev !== "undefined" && PDFJSDev.test("MOZCENTRAL")) {
+      eventBus._on(
+        "annotationeditorstateschanged",
+        annotationEditorStatesChanged


For consistency with all of the code above, please make this:

Suggested change

annotationEditorStatesChanged

webViewerAnnotationEditorStatesChanged

Snuffleupagus · 2022-07-04T17:39:49Z

web/app.js

@@ -3074,6 +3080,10 @@ function beforeUnload(evt) {
  return false;
 }

+function annotationEditorStatesChanged(data) {
+  PDFViewerApplication.externalServices.setEditorStates(data);


For documentation purposes, please add the following dummy-method to the DefaultExternalServices-class at the top of the file:

static updateEditorStates(data) { throw new Error("Not implemented: updateEditorStates"); }

Snuffleupagus · 2022-07-04T17:42:16Z

src/display/editor/tools.js

  constructor(eventBus) {
    this.#eventBus = eventBus;
+    this.#eventBus.on("editingaction", this.#boundOnEditingAction);


Please use the ._on-method for every "internal" use-case, since that helps ensure that the PDF.js code always receive events before any externally registered event listeners.

Suggested change

this.#eventBus.on("editingaction", this.#boundOnEditingAction);

eventBus._on("editingaction", this.#boundOnEditingAction);

Snuffleupagus · 2022-07-04T17:53:02Z

src/display/editor/tools.js

+      this.#eventBus.dispatch("annotationeditorstateschanged", {
+        source: this,
+        details,
+      });
+      Object.assign(this.#previousStates, details);


It seems, as far as I can tell, that the "shape" of the details-Object will differ quite a lot between call-sites. Is that an issue, and if so do we perhaps want to ensure that we always send consistent data here?

You could possibly avoid that by doing something like this:

Suggested change

this.#eventBus.dispatch("annotationeditorstateschanged", {

source: this,

details,

});

Object.assign(this.#previousStates, details);

this.#eventBus.dispatch("annotationeditorstateschanged", {

source: this,

details: Object.assign(this.#previousStates, details),

});

Snuffleupagus · 2022-07-04T18:01:03Z

web/firefoxcom.js

+  static setEditorStates(data) {
+    FirefoxCom.request("setEditorStates", data);


For consistency with existing methods, in the FirefoxExternalServices-class, maybe use:

Suggested change

static setEditorStates(data) {

FirefoxCom.request("setEditorStates", data);

static updateEditorStates(data) {

FirefoxCom.request("updateEditorStates", data);

Snuffleupagus · 2022-07-05T19:36:37Z

src/display/editor/editor.js

+   */
+  mousedown(event) {
+    if (event.button !== 0) {
+      // Avoid to focus this editor because of a right click.


Maybe tweak the comment slightly, now that the condition was updated above?

Suggested change

// Avoid to focus this editor because of a right click.

// Avoid to focus this editor because of a non-left click.

Snuffleupagus · 2022-07-05T19:49:32Z

src/display/editor/tools.js

  constructor(eventBus) {
    this.#eventBus = eventBus;
+    this.#eventBus._on("editingaction", this.#boundOnEditingAction);


It doesn't seem that anything is ever removing this listener, as far as I can tell?
That means that when multiple documents have been opened in the GENERIC viewer, there'll be a number of old (and stale) listeners still around. While "editingaction"-support will currently only be implemented in Firefox, this is nonetheless something that we should fix here to prevent any future issues.

Basically, I think that the simplest solution would be to add e.g. a destroy-method to this class and having it run:

this.#eventBus._off("editingaction", this.#boundOnEditingAction);

Then, in the viewer, remove line

pdf.js/web/base_viewer.js

Line 902 in a1ac1a6

this.#annotationEditorUIManager = null;

and instead add

if (this.#annotationEditorUIManager) { this.#annotationEditorUIManager.destroy(); this.#annotationEditorUIManager = null; }

just after line

pdf.js/web/base_viewer.js

Line 642 in a1ac1a6

}

- this way the context menu in Firefox can take into account what we have in the clipboard, if an editor is selected, ... - when the user will click on a context menu item, an action will be triggered, hence this patch adds what is required to handle it; - some tests will be added in the Firefox' patch.

Snuffleupagus · 2022-07-05T20:18:24Z

/botio integrationtest

pdfjsbot · 2022-07-05T20:18:25Z

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 1

Live output at: http://54.241.84.105:8877/5e859836d7f1c5a/output.txt

pdfjsbot · 2022-07-05T20:18:25Z

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 1

Live output at: http://54.193.163.58:8877/6684513ded1c454/output.txt

pdfjsbot · 2022-07-05T20:28:51Z

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/5e859836d7f1c5a/output.txt

Total script time: 4.74 mins

Integration Tests: Passed

pdfjsbot · 2022-07-05T20:36:07Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/6684513ded1c454/output.txt

Total script time: 8.30 mins

Integration Tests: FAILED

calixteman added the editor label Jul 4, 2022

calixteman requested a review from Snuffleupagus July 4, 2022 16:08

calixteman force-pushed the context_menu branch from 863c74f to 979327f Compare July 4, 2022 16:21

Snuffleupagus requested changes Jul 4, 2022

View reviewed changes

calixteman requested a review from Snuffleupagus July 5, 2022 15:43

calixteman force-pushed the context_menu branch from 979327f to 49a13c9 Compare July 5, 2022 15:43

Snuffleupagus reviewed Jul 5, 2022

View reviewed changes

calixteman requested a review from Snuffleupagus July 5, 2022 20:12

calixteman force-pushed the context_menu branch from 49a13c9 to ec0f9f6 Compare July 5, 2022 20:13

Snuffleupagus approved these changes Jul 5, 2022

View reviewed changes

Snuffleupagus merged commit bde4663 into mozilla:master Jul 5, 2022

Snuffleupagus mentioned this pull request Jul 8, 2022

Error during destruction of the Editing-instance #15149

Closed

marco-c linked an issue Jul 13, 2022 that may be closed by this pull request

Support copying and pasting by right click context menu #15041

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Editor] Dispatch an event when some global states are changing (bug 1777695) #15130

[Editor] Dispatch an event when some global states are changing (bug 1777695) #15130

calixteman commented Jul 4, 2022

Snuffleupagus Jul 4, 2022

Snuffleupagus Jul 4, 2022

Snuffleupagus Jul 4, 2022

Snuffleupagus Jul 4, 2022

Snuffleupagus Jul 4, 2022

Snuffleupagus Jul 4, 2022

Snuffleupagus Jul 4, 2022

Snuffleupagus Jul 4, 2022

Snuffleupagus Jul 4, 2022

Snuffleupagus Jul 4, 2022

Snuffleupagus Jul 5, 2022

Snuffleupagus Jul 5, 2022

Snuffleupagus commented Jul 5, 2022

pdfjsbot commented Jul 5, 2022

pdfjsbot commented Jul 5, 2022

pdfjsbot commented Jul 5, 2022

pdfjsbot commented Jul 5, 2022

		if (event.button === 2) {
		// Avoid to focus this editor because of a right click.

	const hasChange = Object.entries(details).some(
	const hasChanged = Object.entries(details).some(

	* It can be useful to temporarly disable it when the user is editing a
	* It can be useful to temporarily disable it when the user is editing a

	annotationEditorStatesChanged
	webViewerAnnotationEditorStatesChanged

	this.#eventBus.on("editingaction", this.#boundOnEditingAction);
	eventBus._on("editingaction", this.#boundOnEditingAction);

		static setEditorStates(data) {
		FirefoxCom.request("setEditorStates", data);

[Editor] Dispatch an event when some global states are changing (bug 1777695) #15130

[Editor] Dispatch an event when some global states are changing (bug 1777695) #15130

Conversation

calixteman commented Jul 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus commented Jul 5, 2022

pdfjsbot commented Jul 5, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jul 5, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Jul 5, 2022

From: Bot.io (Linux m4)

Success

pdfjsbot commented Jul 5, 2022

From: Bot.io (Windows)

Failed