[Editor] Add the ability to make multiple selections (bug 1779582) #15200

calixteman · 2022-07-21T09:00:40Z

several editors can be selected/unselected using ctrl+click;
and then they can be copied, pasted, their properties can be changed.

Snuffleupagus · 2022-07-21T09:47:37Z

src/display/editor/editor.js

+    return this.#isEditing;
+  }
+
+  set isEditing(value) {


Please add a JSDoc-comment for the value parameter.

src/display/editor/freetext.js

Snuffleupagus · 2022-07-21T09:54:13Z

src/display/editor/tools.js

+  setSelected(editor) {
+    if (!this.#isMultipleSelection) {
+      if (this.#selectedEditors.has(editor)) {
+        if (this.#selectedEditors.size >= 2) {


Optional-nit:

Suggested change

if (this.#selectedEditors.size >= 2) {

if (this.#selectedEditors.size > 1) {

Snuffleupagus · 2022-07-21T09:55:08Z

src/display/editor/tools.js

+          }
+          this.#selectedEditors.clear();
+          this.#selectedEditors.add(editor);
+          this.#dispatchUpdateUI(editor.propertiesToUpdate);


Don't you also need a dispatchUpdateStates-call in this case too?

It's useless because the only value we could update is hasSelectedEditor but its value won't change since we already know that the #selectedEditors is non-empty, and after the removal it's still non-empty (we let the select editor inside).

src/display/editor/tools.js

Snuffleupagus · 2022-07-21T10:06:14Z

web/annotation_editor_layer_builder.css

-}
-
-.annotationEditorLayer .inkEditor:not(:focus) {
+.annotationEditorLayer .inkEditor.disabled:not(.selectedEditor) {


Is the last part actually necessary here, or does the following work (given the rule just below which override it when necessary)?

Suggested change

.annotationEditorLayer .inkEditor.disabled:not(.selectedEditor) {

.annotationEditorLayer .inkEditor.disabled {

Snuffleupagus · 2022-07-21T10:06:44Z

web/annotation_editor_layer_builder.css

+.annotationEditorLayer .background {
+  z-index: 0;


This was just removed, in another PR, so is this perhaps a rebase mistake?

Snuffleupagus · 2022-07-21T18:17:24Z

src/display/editor/editor.js

+
+  /**
+   * If true then the editor is currently edited.
+   * @return {boolean}


Looking at our other getters, elsewhere in the code-base, I believe that you want the following for things to work generally:

Suggested change

* @return {boolean}

* @type {boolean}

Snuffleupagus · 2022-07-21T18:18:12Z

test/integration/freetext_editor_spec.js

+          await page.keyboard.press("v");
+          await page.keyboard.up("Control");
+
+          // 0,1,3 are unselected and new paster editors are selected.


Nit: Did you mean the following?

Suggested change

// 0,1,3 are unselected and new paster editors are selected.

// 0,1,3 are unselected and new pasted editors are selected.

Snuffleupagus · 2022-07-21T18:19:22Z

test/integration/freetext_editor_spec.js

+          // Select that all the editors are correctly selected (and the focus
+          // didn't move on the body when the empty editor has been removed).


Nit: Did you mean the following?

Suggested change

// Select that all the editors are correctly selected (and the focus

// didn't move on the body when the empty editor has been removed).

// Check that all the editors are correctly selected (and the focus

// didn't move to the body when the empty editor was removed).

Snuffleupagus · 2022-07-21T18:20:52Z

test/integration/freetext_editor_spec.js

+            .withContext(`In ${browserName}`)
+            .toEqual([7]);
+
+          // Get the focus to 2 and check that only 2 is selected.


Nit: Did you mean the following?

Suggested change

// Get the focus to 2 and check that only 2 is selected.

// Set the focus to 2 and check that only 2 is selected.

Snuffleupagus · 2022-07-21T18:45:54Z

src/display/editor/tools.js

+
+  /**
+   * An editor just got a mousedown with ctrl key pressed.
+   * @param {boolean}} isMultiple


Suggested change

* @param {boolean}} isMultiple

* @param {boolean} isMultiple

Snuffleupagus · 2022-07-21T18:46:10Z

src/display/editor/annotation_editor_layer.js

+
+  /**
+   * An editor just got a mousedown with ctrl key pressed.
+   * @param {boolean}} isMultiple


Suggested change

* @param {boolean}} isMultiple

* @param {boolean} isMultiple

- several editors can be selected/unselected using ctrl+click; - and then they can be copied, pasted, their properties can be changed.

calixteman · 2022-07-21T20:54:44Z

/botio integrationtest

pdfjsbot · 2022-07-21T20:54:46Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2022-07-21T20:54:46Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/761cc3dfb7ee27b/output.txt

pdfjsbot · 2022-07-21T20:59:30Z

From: Bot.io (Linux m4)

Success

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

Total script time: 4.73 mins

Integration Tests: Passed

pdfjsbot · 2022-07-21T21:02:01Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/761cc3dfb7ee27b/output.txt

Total script time: 7.25 mins

Integration Tests: FAILED

calixteman added the editor label Jul 21, 2022

calixteman requested a review from Snuffleupagus July 21, 2022 09:00

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

Support selecting multiple drawn elements #15042

Closed

Snuffleupagus requested changes Jul 21, 2022

View reviewed changes

calixteman force-pushed the multiple_selection branch from 2fffc1a to 96feefb Compare July 21, 2022 15:06

calixteman requested a review from Snuffleupagus July 21, 2022 16:37

Snuffleupagus approved these changes Jul 21, 2022

View reviewed changes

[Editor] Add the ability to make multiple selections (bug 1779582)

d6b9ca4

- several editors can be selected/unselected using ctrl+click; - and then they can be copied, pasted, their properties can be changed.

calixteman force-pushed the multiple_selection branch from 96feefb to d6b9ca4 Compare July 21, 2022 20:54

calixteman merged commit 6138e16 into mozilla:master Jul 21, 2022

calixteman deleted the multiple_selection branch July 21, 2022 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Editor] Add the ability to make multiple selections (bug 1779582) #15200

[Editor] Add the ability to make multiple selections (bug 1779582) #15200

calixteman commented Jul 21, 2022

Snuffleupagus Jul 21, 2022

Snuffleupagus Jul 21, 2022

Snuffleupagus Jul 21, 2022

calixteman Jul 21, 2022

Snuffleupagus Jul 21, 2022

Snuffleupagus Jul 21, 2022

Snuffleupagus Jul 21, 2022

Snuffleupagus Jul 21, 2022 •

edited

Loading

Snuffleupagus Jul 21, 2022

Snuffleupagus Jul 21, 2022

Snuffleupagus Jul 21, 2022

Snuffleupagus Jul 21, 2022

calixteman commented Jul 21, 2022

pdfjsbot commented Jul 21, 2022

pdfjsbot commented Jul 21, 2022

pdfjsbot commented Jul 21, 2022

pdfjsbot commented Jul 21, 2022

	if (this.#selectedEditors.size >= 2) {
	if (this.#selectedEditors.size > 1) {

	.annotationEditorLayer .inkEditor.disabled:not(.selectedEditor) {
	.annotationEditorLayer .inkEditor.disabled {

	// 0,1,3 are unselected and new paster editors are selected.
	// 0,1,3 are unselected and new pasted editors are selected.

		// Select that all the editors are correctly selected (and the focus
		// didn't move on the body when the empty editor has been removed).

	// Get the focus to 2 and check that only 2 is selected.
	// Set the focus to 2 and check that only 2 is selected.

[Editor] Add the ability to make multiple selections (bug 1779582) #15200

[Editor] Add the ability to make multiple selections (bug 1779582) #15200

Conversation

calixteman commented Jul 21, 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

Snuffleupagus Jul 21, 2022 • edited Loading

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

calixteman commented Jul 21, 2022

pdfjsbot commented Jul 21, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jul 21, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Jul 21, 2022

From: Bot.io (Linux m4)

Success

pdfjsbot commented Jul 21, 2022

From: Bot.io (Windows)

Failed

Snuffleupagus Jul 21, 2022 •

edited

Loading