[Editor] Add some telemetry to know how often the editing features are used (bug 1782254)

[calixteman]

No description provided.

[Snuffleupagus]

I can't help but wonder if we could perhaps reduce the amount of code needed, primarily in the src/display/editor/-folder, in order pass the necessary editor-type onto the viewer?

More specifically, in addition to the code that you're adding to handle saving/printing, we might be able to use the AnnotationStorage along these lines:

diff --git a/src/display/annotation_storage.js b/src/display/annotation_storage.js
index 6b558f8f0..eb52a4823 100644
--- a/src/display/annotation_storage.js
+++ b/src/display/annotation_storage.js
@@ -31,6 +31,8 @@ class AnnotationStorage {
     // can have undesirable effects.
     this.onSetModified = null;
     this.onResetModified = null;
+
+    this.onAnnotationEditor = null;
   }
 
   /**
@@ -100,6 +102,13 @@ class AnnotationStorage {
     if (modified) {
       this.#setModified();
     }
+
+    if (
+      value instanceof AnnotationEditor &&
+      typeof this.onAnnotationEditor === "function"
+    ) {
+      this.onAnnotationEditor(value.constructor._type);
+    }
   }
 
   /**
diff --git a/web/app.js b/web/app.js
index 8b4e6265e..fd05a99aa 100644
--- a/web/app.js
+++ b/web/app.js
@@ -1734,6 +1734,12 @@ const PDFViewerApplication = {
         delete this._annotationStorageModified;
       }
     };
+    annotationStorage.onAnnotationEditor = typeStr => {
+      this.externalServices.reportTelemetry({
+        type: "editing",
+        data: { type: typeStr },
+      })
+    };
   },
 
   setInitialView(

[calixteman]

I can't help but wonder if we could perhaps reduce the amount of code needed, primarily in the src/display/editor/-folder, in order pass the necessary editor-type onto the viewer?

More specifically, in addition to the code that you're adding to handle saving/printing, we might be able to use the AnnotationStorage along these lines:

diff --git a/src/display/annotation_storage.js b/src/display/annotation_storage.js
index 6b558f8f0..eb52a4823 100644
--- a/src/display/annotation_storage.js
+++ b/src/display/annotation_storage.js
@@ -31,6 +31,8 @@ class AnnotationStorage {
     // can have undesirable effects.
     this.onSetModified = null;
     this.onResetModified = null;
+
+    this.onAnnotationEditor = null;
   }
 
   /**
@@ -100,6 +102,13 @@ class AnnotationStorage {
     if (modified) {
       this.#setModified();
     }
+
+    if (
+      value instanceof AnnotationEditor &&
+      typeof this.onAnnotationEditor === "function"
+    ) {
+      this.onAnnotationEditor(value.constructor._type);
+    }
   }
 
   /**
diff --git a/web/app.js b/web/app.js
index 8b4e6265e..fd05a99aa 100644
--- a/web/app.js
+++ b/web/app.js
@@ -1734,6 +1734,12 @@ const PDFViewerApplication = {
         delete this._annotationStorageModified;
       }
     };
+    annotationStorage.onAnnotationEditor = typeStr => {
+      this.externalServices.reportTelemetry({
+        type: "editing",
+        data: { type: typeStr },
+      })
+    };
   },
 
   setInitialView(

It's definitely more clever than my solution but it adds a kind of invisible dependency between annotationStorage and externalServices, is it really something we want ?

[Snuffleupagus]

but it adds a kind of invisible dependency between annotationStorage and externalServices,

To me it's not that much different from the pre-existing callback-functions available in the AnnotationStorage class, and in a way the new hasAnnotationEditors method (which we still need of course) also introduces a kind-of dependency here.
The advantage of the suggested approach, at least in my opinion, is that it's smaller and easier to overview which hopefully means that it's less likely to break with any future changes to the Editing-functionality.

[Snuffleupagus]

/botio integrationtest

[pdfjsbot]

From: Bot.io (Linux m4)

---

Received

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

Live output at: http://54.241.84.105:8877/f665a041f21edaf/output.txt

[pdfjsbot]

From: Bot.io (Windows)

---

Received

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

Live output at: http://54.193.163.58:8877/73c591736ec6dde/output.txt

[pdfjsbot]

From: Bot.io (Linux m4)

---

Success

Full output at http://54.241.84.105:8877/f665a041f21edaf/output.txt

Total script time: 4.67 mins

• Integration Tests: Passed

[pdfjsbot]

From: Bot.io (Windows)

---

Failed

Full output at http://54.193.163.58:8877/73c591736ec6dde/output.txt

Total script time: 10.09 mins

• Integration Tests: FAILED