[editor] Add an Ink editor #14989

calixteman · 2022-06-04T21:38:54Z

Approximate the drawn curve by a set of Bezier curves in using
js code from https://github.com/soswow/fit-curves.
The code has been slightly modified in order to make the linter
happy.

Snuffleupagus · 2022-06-04T21:42:25Z

web/images/toolbarButton-inkMode.svg

@@ -0,0 +1,9 @@
+<?xml version='1.0' encoding='utf-8'?>


Duplicate file, please remove it.

Snuffleupagus · 2022-06-04T21:46:42Z

src/display/editor/bezier_approx.js

@@ -0,0 +1,677 @@
+/**


First of all, that's a lot of code needed to support adding of Ink Annotations.

Secondly, it looks like this is available through NPM so can we please use that instead of "dumping" this code into the PDF.js repository (since that's not very maintainable)?

First of all, that's a lot of code needed to support adding of Ink Annotations.

Yes it is but it helps to make the drawing nicer and it helps to reduce the size of the final InkList when the annotation is saved.

Do you have an example on how to use a node package in pdf.js ?

Just including a Node.js package in a build is trivial, but that unfortunately doesn't take the gulp server mode into account. However that should be possible to solve, but it's possibly non-trivial to implement; note e.g. the following development-specific build tasks.

In order to be able to move forward with this PR a bit more quickly, I'd accept that we temporarily add this code to the PDF.js library assuming the following:

The new file is placed in its own directory in https://github.com/mozilla/pdf.js/tree/master/external/, and not inside of the src/-folder.

The commented-out code in the file is removed, since it seems unnecessary to include that.

The automated comment in [editor] Add an Ink editor #14989 (review) is fixed.

An issue is opened, which tracks removing that and using the npm package instead.

src/display/editor/bezier_approx.js

web/toolbar.js

Snuffleupagus · 2022-06-06T11:27:52Z

web/viewer.html

@@ -270,6 +270,9 @@
                  <button id="editorFreeText" class="toolbarButton" title="Add FreeText Annotation" role="radio" aria-checked="false" tabindex="32" data-l10n-id="editor_free_text">
                    <span data-l10n-id="editor_free_text_label">FreeText Annotation</span>
                  </button>
+                  <button id="editorInk" class="toolbarButton" title="Add Ink Annotation" role="radio" aria-checked="false" tabindex="32" data-l10n-id="editor_ink">


Please fix the duplicated tabindex.

Snuffleupagus · 2022-06-06T11:29:17Z

src/display/editor/ink.js

+    // https://en.wikipedia.org/wiki/De_Casteljau%27s_algorithm
+
+    // The first point is the last point of the previous Bezier curve
+    // so no need to push the firt point.


firt -> first

Snuffleupagus · 2022-06-08T14:39:42Z

It looks like gulp ci-test failed, i.e. what GitHub action runs, and I'd guess that you need to extend this line to fix that:

pdf.js/gulpfile.js

Line 1547 in 2fbf14a

["examples/node/domstubs.js", "web/*.js", "!web/{pdfjs,viewer}.js"],

calixteman · 2022-06-08T14:53:51Z

It looks like gulp ci-test failed, i.e. what GitHub action runs, and I'd guess that you need to extend this line to fix that:

pdf.js/gulpfile.js

Line 1547 in 2fbf14a

["examples/node/domstubs.js", "web/*.js", "!web/{pdfjs,viewer}.js"],

I added external/fit_curve/fit_curve.js but it doesn't work because external is a the same level as display in lib-legacy.
So changing ../../../external into ../../external is ok but then gulp server isn't.

Snuffleupagus · 2022-06-08T16:00:15Z

I added external/fit_curve/fit_curve.js but it doesn't work because external is a the same level as display in lib-legacy.
So changing ../../../external into ../../external is ok but then gulp server isn't.

I suppose that you can temporarily move it back its original position, in the src/display/editor/ folder, but that makes fixing the follow-up issue even more important (in my opinion).

src/display/editor/fit_curve/fit_curve.js

Snuffleupagus · 2022-06-08T19:43:00Z

/botio test

pdfjsbot · 2022-06-08T19:43:02Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2022-06-08T19:43:02Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2022-06-08T20:09:11Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 26.15 mins

Font tests: Passed
Unit tests: FAILED
Integration Tests: Passed
Regression tests: FAILED

  different ref/snapshot: 52

Image differences available at: http://54.241.84.105:8877/cfa5839bc6b302c/reftest-analyzer.html#web=eq.log

pdfjsbot · 2022-06-08T20:11:56Z

From: Bot.io (Windows)

Failed

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

Total script time: 28.90 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: FAILED
Regression tests: FAILED

  different ref/snapshot: 1

Image differences available at: http://54.193.163.58:8877/fc0148520bfe7e2/reftest-analyzer.html#web=eq.log

Snuffleupagus

r=me, with comments addressed; thank you!

Snuffleupagus · 2022-06-09T14:57:49Z

src/display/editor/ink.js

+      // At some point this editor has been removed and
+      // we're rebuilting it, hence we must add it to its
+      // parent.


Spelling, grammar, and line-breaking nits:

Suggested change

// At some point this editor has been removed and

// we're rebuilting it, hence we must add it to its

// parent.

// At some point this editor was removed and we're rebuilding it,

// hence we must add it to its parent.

Snuffleupagus · 2022-06-09T14:59:42Z

src/display/editor/ink.js

+    super.remove();
+
+    // Destroy the canvas.
+    this.canvas.width = this.canvas.heigth = 0;


Do you also want to set this.canvas = null; to remove it completely?

Snuffleupagus · 2022-06-09T15:00:51Z

src/display/editor/ink.js

+    this.div.classList.add("editing");
+
+    if (this.width) {
+      // This editor has been created in using copy (ctrl+c).


Grammar nits:

Suggested change

// This editor has been created in using copy (ctrl+c).

// This editor was created by copying (ctrl+c).

Also, please update the two similar sentences in https://github.com/mozilla/pdf.js/blob/master/src/display/editor/freetext.js while you're at it (I missed those previously).

Snuffleupagus · 2022-06-09T15:11:27Z

src/display/editor/ink.js

+        this.setDimensions(rect.width, rect.height);
+      }
+    });
+    observer.observe(this.div);


Is the observer removed automatically when the current InkEditor-instance is removed, or do we (somehow) need to do that manually?

Snuffleupagus · 2022-06-09T15:26:12Z

src/display/editor/ink.js

+      this.#redraw();
+    }
+
+    const observer = new ResizeObserver(entries => {


This does not require any changes here, however:

According to https://developer.mozilla.org/en-US/docs/Web/API/ResizeObserver#browser_compatibility, this isn't available in all Safari versions that we currently "support".

Given first of all that this isn't even enabled by default and secondly that our Safari-support is listed as partial, this isn't a big issue as far as I'm concerned.
Hence I'd suggest that we just add the following code:

diff --git a/web/app_options.js b/web/app_options.js index 243aa9e11..d75277c56 100644 --- a/web/app_options.js +++ b/web/app_options.js @@ -38,6 +38,12 @@ if (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) { compatibilityParams.maxCanvasPixels = 5242880; } })(); + + (function checkResizeObserver() { + if (typeof ResizeObserver === "undefined") { + compatibilityParams.annotationEditorEnabled = false; + } + })(); } const OptionKind = { @@ -309,6 +315,9 @@ if ( kind: OptionKind.VIEWER, }; + defaultOptions.annotationEditorEnabled.compatibility = + compatibilityParams.annotationEditorEnabled; + defaultOptions.renderer.kind += OptionKind.PREFERENCE; } else if (PDFJSDev.test("CHROME")) { defaultOptions.disableTelemetry = {

- Approximate the drawn curve by a set of Bezier curves in using js code from https://github.com/soswow/fit-curves. The code has been slightly modified in order to make the linter happy.

calixteman · 2022-06-09T17:36:58Z

/botio unittest

pdfjsbot · 2022-06-09T17:37:00Z

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

pdfjsbot · 2022-06-09T17:37:00Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/56b61bb15724963/output.txt

pdfjsbot · 2022-06-09T17:40:12Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 3.19 mins

Unit Tests: FAILED

pdfjsbot · 2022-06-09T17:43:45Z

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/56b61bb15724963/output.txt

Total script time: 6.74 mins

Unit Tests: Passed

calixteman · 2022-06-09T18:23:48Z

/botio integrationtest

pdfjsbot · 2022-06-09T18:23:49Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2022-06-09T18:23:50Z

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

pdfjsbot · 2022-06-09T18:28:20Z

From: Bot.io (Linux m4)

Success

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

Total script time: 4.50 mins

Integration Tests: Passed

pdfjsbot · 2022-06-09T18:31:29Z

From: Bot.io (Windows)

Success

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

Total script time: 7.65 mins

Integration Tests: Passed

calixteman marked this pull request as draft June 4, 2022 21:39

Snuffleupagus reviewed Jun 4, 2022

View reviewed changes

Snuffleupagus added the editor label Jun 4, 2022

github-advanced-security bot found potential problems Jun 4, 2022

View reviewed changes

src/display/editor/bezier_approx.js Fixed Show fixed Hide fixed

Snuffleupagus reviewed Jun 6, 2022

View reviewed changes

Snuffleupagus linked an issue Jun 6, 2022 that may be closed by this pull request

Add support for drawing #14971

Closed

calixteman force-pushed the ink1 branch 2 times, most recently from 1ff2d70 to c2b66dd Compare June 8, 2022 14:26

calixteman marked this pull request as ready for review June 8, 2022 14:28

marco-c mentioned this pull request Jun 8, 2022

Support moving elements around #14974

Closed

calixteman force-pushed the ink1 branch from c2b66dd to 365af8b Compare June 8, 2022 15:49

calixteman force-pushed the ink1 branch from 365af8b to 691b653 Compare June 8, 2022 16:11

github-advanced-security bot found potential problems Jun 8, 2022

View reviewed changes

src/display/editor/fit_curve/fit_curve.js Fixed Show resolved Hide resolved

calixteman force-pushed the ink1 branch from 691b653 to eba8c5d Compare June 8, 2022 16:44

calixteman requested a review from Snuffleupagus June 9, 2022 14:25

Snuffleupagus approved these changes Jun 9, 2022

View reviewed changes

[editor] Add an Ink editor

c161a86

- Approximate the drawn curve by a set of Bezier curves in using js code from https://github.com/soswow/fit-curves. The code has been slightly modified in order to make the linter happy.

calixteman force-pushed the ink1 branch from eba8c5d to c161a86 Compare June 9, 2022 17:36

calixteman merged commit 61a6534 into mozilla:master Jun 9, 2022

calixteman deleted the ink1 branch June 9, 2022 19:13

ZeroXClem mentioned this pull request Aug 12, 2024

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

Closed

earthywh mentioned this pull request Sep 24, 2024

[Snyk] Upgrade pdfjs-dist from 2.6.347 to 2.16.105 earthywh/filestash#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[editor] Add an Ink editor #14989

[editor] Add an Ink editor #14989

calixteman commented Jun 4, 2022

Snuffleupagus Jun 4, 2022

Snuffleupagus Jun 4, 2022

calixteman Jun 4, 2022

calixteman Jun 5, 2022

Snuffleupagus Jun 6, 2022 •

edited

Loading

Snuffleupagus Jun 6, 2022

Snuffleupagus Jun 6, 2022

Snuffleupagus commented Jun 8, 2022 •

edited

Loading

calixteman commented Jun 8, 2022

Snuffleupagus commented Jun 8, 2022

Snuffleupagus commented Jun 8, 2022

pdfjsbot commented Jun 8, 2022

pdfjsbot commented Jun 8, 2022

pdfjsbot commented Jun 8, 2022

pdfjsbot commented Jun 8, 2022

Snuffleupagus left a comment

Snuffleupagus Jun 9, 2022

Snuffleupagus Jun 9, 2022

Snuffleupagus Jun 9, 2022

Snuffleupagus Jun 9, 2022

Snuffleupagus Jun 9, 2022

Snuffleupagus Jun 9, 2022

calixteman commented Jun 9, 2022

pdfjsbot commented Jun 9, 2022

pdfjsbot commented Jun 9, 2022

pdfjsbot commented Jun 9, 2022

pdfjsbot commented Jun 9, 2022

calixteman commented Jun 9, 2022

pdfjsbot commented Jun 9, 2022

pdfjsbot commented Jun 9, 2022

pdfjsbot commented Jun 9, 2022

pdfjsbot commented Jun 9, 2022

	// This editor has been created in using copy (ctrl+c).
	// This editor was created by copying (ctrl+c).

[editor] Add an Ink editor #14989

[editor] Add an Ink editor #14989

Conversation

calixteman commented Jun 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

Snuffleupagus Jun 6, 2022 • 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 8, 2022 • edited Loading

calixteman commented Jun 8, 2022

Snuffleupagus commented Jun 8, 2022

Snuffleupagus commented Jun 8, 2022

pdfjsbot commented Jun 8, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 8, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 8, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Jun 8, 2022

From: Bot.io (Windows)

Failed

Snuffleupagus left a 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 9, 2022

pdfjsbot commented Jun 9, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 9, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 9, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Jun 9, 2022

From: Bot.io (Windows)

Success

calixteman commented Jun 9, 2022

pdfjsbot commented Jun 9, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 9, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 9, 2022

From: Bot.io (Linux m4)

Success

pdfjsbot commented Jun 9, 2022

From: Bot.io (Windows)

Success

Snuffleupagus Jun 6, 2022 •

edited

Loading

Snuffleupagus commented Jun 8, 2022 •

edited

Loading