[Editor] - Add the ability to directly draw after selecting ink tool #15050

calixteman · 2022-06-16T16:19:49Z

Right now, we must select the tool, then click to select a page and
click to start drawing and it's a bit painful;
so just create a new ink editor when we're hovering a page without one.

Snuffleupagus

r=me, with comments addressed (mostly grammar nits) and passing tests; thank you!

Snuffleupagus · 2022-06-16T16:34:32Z

src/display/editor/annotation_editor_layer.js

+      // We want to have the ink editor covering all the page
+      // and no click to create it: it must be here when we start to draw.


Suggested change

// We want to have the ink editor covering all the page

// and no click to create it: it must be here when we start to draw.

// We want to have the ink editor covering all of the page without having

// to click to create it: it must be here when we start to draw.

Snuffleupagus · 2022-06-16T16:34:44Z

src/display/editor/annotation_editor_layer.js

+  }
+
+  /**
+   * mouseover callback.


Suggested change

* mouseover callback.

* Mouseover callback.

Snuffleupagus · 2022-06-16T16:35:52Z

src/display/editor/annotation_editor_layer.js

+      // The div is the target so there is no ink editor, hence we can
+      // create a new one.
+      // event.buttons === 0 is here to avoid adding a new ink editor
+      // when we drop and editor.


Did you actually mean the following?

Suggested change

// when we drop and editor.

// when we drop an editor.

Snuffleupagus · 2022-06-16T16:36:34Z

src/display/editor/annotation_editor_layer.js

+   * @param {boolean} mustExec - if true the command is executed after having
+   * been added.


Given that this is optional:

Suggested change

* @param {boolean} mustExec - if true the command is executed after having

* been added.

* @param {boolean} [mustExec] - If true the command is executed after having

* been added.

Snuffleupagus · 2022-06-16T16:37:39Z

src/display/editor/annotation_editor_layer.js

   */
-  addCommands(cmd, undo) {
-    this.#uiManager.addCommands(cmd, undo);
+  addCommands(cmd, undo, mustExec = true) {


Similar to another recent PR, can we please have the default value be false instead?
(Since having undefined => true feels quite surprising.)

Snuffleupagus · 2022-06-16T16:40:07Z

src/display/editor/ink.js

+    // This editor must be on top of the main ink
+    // editor.


Nit: Fits on one line.

Snuffleupagus · 2022-06-16T16:40:55Z

src/display/editor/ink.js

+    // Since it the last children there is no need to give
+    // a higher z-index.


Suggested change

// Since it the last children there is no need to give

// a higher z-index.

// Since it's the last child, there's no need to give it a higher z-index.

Snuffleupagus · 2022-06-16T16:41:53Z

src/display/editor/ink.js

+      // Since the ink editor covers all the page and we
+      // want to be able to select an other editor, we
+      // just put this one in background.


Suggested change

// Since the ink editor covers all the page and we

// want to be able to select an other editor, we

// just put this one in background.

// Since the ink editor covers all of the page and we want to be able

// to select another editor, we just put this one in the background.

Snuffleupagus · 2022-06-16T16:43:18Z

src/display/editor/tools.js

   */
-  addCommands(cmd, undo) {
-    this.#commandManager.add(cmd, undo);
+  addCommands(cmd, undo, mustExec = true) {


Similar to another recent PR, can we please have the default value be false instead?
(Since having undefined => true feels quite surprising.)

Snuffleupagus · 2022-06-16T16:47:14Z

src/display/editor/tools.js

   */
-  add(cmd, undo) {
+  add(cmd, undo, mustExec) {


Please mark the new parameter as optional with mustExec = falsem and also indicate that in the JSDoc (using * @param {boolean} [mustExec]).

- Right now, we must select the tool, then click to select a page and click to start drawing and it's a bit painful; - so just create a new ink editor when we're hovering a page without one.

calixteman · 2022-06-16T17:53:32Z

/botio unittest

pdfjsbot · 2022-06-16T17:53:34Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/6d64997aaa346b7/output.txt

pdfjsbot · 2022-06-16T17:53:34Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/fe0f6e31978ce32/output.txt

pdfjsbot · 2022-06-16T17:56:45Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/6d64997aaa346b7/output.txt

Total script time: 3.16 mins

Unit Tests: FAILED

pdfjsbot · 2022-06-16T17:59:50Z

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/fe0f6e31978ce32/output.txt

Total script time: 6.26 mins

Unit Tests: Passed

calixteman · 2022-06-16T18:22:57Z

/botio integrationtest

pdfjsbot · 2022-06-16T18:22:58Z

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/8735927338ade28/output.txt

pdfjsbot · 2022-06-16T18:22:58Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/30175c2f8a70079/output.txt

pdfjsbot · 2022-06-16T18:27:27Z

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/8735927338ade28/output.txt

Total script time: 4.47 mins

Integration Tests: Passed

pdfjsbot · 2022-06-16T18:30:17Z

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/30175c2f8a70079/output.txt

Total script time: 7.30 mins

Integration Tests: Passed

calixteman added the editor label Jun 16, 2022

calixteman requested a review from Snuffleupagus June 16, 2022 16:19

Snuffleupagus approved these changes Jun 16, 2022

View reviewed changes

[Editor] - Add the ability to directly draw after selecting ink tool

e7dc1ef

- Right now, we must select the tool, then click to select a page and click to start drawing and it's a bit painful; - so just create a new ink editor when we're hovering a page without one.

calixteman force-pushed the make_ink_better branch from bca75a9 to e7dc1ef Compare June 16, 2022 17:53

calixteman merged commit b868812 into mozilla:master Jun 16, 2022

calixteman deleted the make_ink_better branch June 16, 2022 18:35

ZeroXClem mentioned this pull request Aug 12, 2024

[Snyk] Upgrade pdfjs-dist from 2.9.359 to 2.16.105 ZeroXClem/metamesa#3

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Editor] - Add the ability to directly draw after selecting ink tool #15050

[Editor] - Add the ability to directly draw after selecting ink tool #15050

calixteman commented Jun 16, 2022

Snuffleupagus left a comment •

edited

Loading

Snuffleupagus Jun 16, 2022

Snuffleupagus Jun 16, 2022

Snuffleupagus Jun 16, 2022

Snuffleupagus Jun 16, 2022

Snuffleupagus Jun 16, 2022

Snuffleupagus Jun 16, 2022

Snuffleupagus Jun 16, 2022

Snuffleupagus Jun 16, 2022

Snuffleupagus Jun 16, 2022

Snuffleupagus Jun 16, 2022

calixteman commented Jun 16, 2022

pdfjsbot commented Jun 16, 2022

pdfjsbot commented Jun 16, 2022

pdfjsbot commented Jun 16, 2022

pdfjsbot commented Jun 16, 2022

calixteman commented Jun 16, 2022

pdfjsbot commented Jun 16, 2022

pdfjsbot commented Jun 16, 2022

pdfjsbot commented Jun 16, 2022

pdfjsbot commented Jun 16, 2022

		// We want to have the ink editor covering all the page
		// and no click to create it: it must be here when we start to draw.

		* @param {boolean} mustExec - if true the command is executed after having
		* been added.

		// Since it the last children there is no need to give
		// a higher z-index.

	// Since it the last children there is no need to give
	// a higher z-index.
	// Since it's the last child, there's no need to give it a higher z-index.

[Editor] - Add the ability to directly draw after selecting ink tool #15050

[Editor] - Add the ability to directly draw after selecting ink tool #15050

Conversation

calixteman commented Jun 16, 2022

Snuffleupagus left a comment • 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

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

calixteman commented Jun 16, 2022

pdfjsbot commented Jun 16, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 16, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 16, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Jun 16, 2022

From: Bot.io (Windows)

Success

calixteman commented Jun 16, 2022

pdfjsbot commented Jun 16, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 16, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 16, 2022

From: Bot.io (Linux m4)

Success

pdfjsbot commented Jun 16, 2022

From: Bot.io (Windows)

Success

Snuffleupagus left a comment •

edited

Loading