Implement support for square and circle annotations #8691

timvandermeij · 2017-07-23T22:42:31Z

The commit descriptions provide more information about the individual changes.

timvandermeij · 2017-07-23T22:55:22Z

/botio-linux preview

timvandermeij · 2017-07-23T22:59:46Z

/botio test

timvandermeij · 2017-07-31T20:55:57Z

Would somebody perhaps have time to review this patch? It will make it easier to implement more annotation types and it is the first step towards supporting annotations without appearance streams (since the invisible annotations that we draw over the canvas can also be made visible and styled in case of missing appearance streams more easily with this approach). Thank you!

timvandermeij · 2017-08-31T22:09:49Z

I have updated the patches to resolve conflicts and to convert the code to ES6 syntax after the recent introduction of this for the annotation layer.

yurydelendik

Looks good to me.

I'm thinking DOMSVGFactory must be created Abstract factory. This means that it will not have static member and be an object that we can pass around. It will have a nice effect: we can replace node polyfill with NodeSVGFactory and have it in the library.

yurydelendik · 2017-09-08T21:52:01Z

src/display/annotation_layer.js

+    // the borders outside the square by default. This behavior cannot be
+    // changed programmatically, so correct for that here.
+    let borderWidth = data.borderStyle.width;
+    let square = document.createElementNS(SVG_NS, 'svg:rect');


Add createElement to the DOMSVGFactory.

yurydelendik · 2017-09-08T21:53:12Z

src/display/annotation_layer.js

+    // changed programmatically, so correct for that here.
+    let borderWidth = data.borderStyle.width;
+    let square = document.createElementNS(SVG_NS, 'svg:rect');
+    square.setAttributeNS(null, 'x', borderWidth / 2);


I wonder if just setAttribute('x', boderWidth / 2); will work?

yurydelendik · 2017-09-08T21:55:42Z

src/display/dom_utils.js

@@ -109,6 +110,23 @@ class DOMCMapReaderFactory {
  }
 }

+class DOMSVGFactory {
+  static create(width, height) {
+    if (width <= 0 || height <= 0) {


If we will keep DOMSVGFactory internal to the library, assert() is preferred.

This patch provides a new unit tested factory for creating SVG containers and elements. This code is duplicated twice in the codebase, but with upcoming changes this would need to be duplicated even more. Moreover, consolidating this code in one factory allows us to replace it easily for e.g., supporting Node.js. Therefore, move this to a central place and update/ES6-ify the related code. Finally, we replace `setAttributeNS` with `setAttribute` because no namespace is provided.

timvandermeij · 2017-09-09T19:43:00Z

Thank you for the review! I incorporated all the improvements you mentioned and verified that everything still works. I like how the SVG factory now also handles element creation so we don't need to export the SVG_NS constant anymore and there is no more explicit document usage; it's all encapsulated in the factory now.

timvandermeij · 2017-09-09T19:45:02Z

/botio-linux preview

pdfjsbot · 2017-09-09T19:45:03Z

From: Bot.io (Linux m4)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/45058e3caa45d22/output.txt

pdfjsbot · 2017-09-09T19:47:21Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/45058e3caa45d22/output.txt

Total script time: 2.30 mins

Published

timvandermeij · 2017-09-09T19:54:44Z

/botio test

pdfjsbot · 2017-09-09T19:54:45Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/c4624884bdc2cab/output.txt

pdfjsbot · 2017-09-09T19:54:45Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/1fbafd100330961/output.txt

pdfjsbot · 2017-09-09T20:11:20Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/c4624884bdc2cab/output.txt

Total script time: 16.58 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2017-09-09T20:24:21Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/1fbafd100330961/output.txt

Total script time: 29.59 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

timvandermeij · 2017-09-09T20:28:26Z

/botio makeref

pdfjsbot · 2017-09-09T20:28:26Z

From: Bot.io (Linux m4)

Received

Command cmd_makeref from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/5b15fef1f84edf0/output.txt

pdfjsbot · 2017-09-09T20:28:26Z

From: Bot.io (Windows)

Received

Command cmd_makeref from @timvandermeij received. Current queue size: 0

Live output at: http://54.215.176.217:8877/6ede4e8756048f5/output.txt

pdfjsbot · 2017-09-09T20:44:05Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/5b15fef1f84edf0/output.txt

Total script time: 15.63 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2017-09-09T20:56:16Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/6ede4e8756048f5/output.txt

Total script time: 27.81 mins

Lint: Passed
Make references: Passed
Check references: Passed

Node.js This patch fixes a regression from PR mozilla#8691 where we switched to using `setAttribute` instead of `setAttributeNS` if no namespace is provided.

…tations Implement support for square and circle annotations

Node.js This patch fixes a regression from PR mozilla#8691 where we switched to using `setAttribute` instead of `setAttributeNS` if no namespace is provided.

timvandermeij added the annotations label Jul 23, 2017

timvandermeij force-pushed the square-circle-annotations branch from fd74034 to ac226b9 Compare July 23, 2017 22:51

mozilla deleted a comment from pdfjsbot Jul 23, 2017

timvandermeij changed the title ~~Implement square and circle annotations~~ Implement support for square and circle annotations Jul 31, 2017

yurydelendik self-requested a review July 31, 2017 21:14

timvandermeij force-pushed the square-circle-annotations branch from ac226b9 to 0a16908 Compare August 31, 2017 22:00

yurydelendik approved these changes Sep 8, 2017

View reviewed changes

timvandermeij added 4 commits September 9, 2017 21:36

Implement support for square annotations

cace2e9

Implement support for circle annotations

44c116a

Implement reference tests for square and circle annotations

c04f9d6

timvandermeij force-pushed the square-circle-annotations branch from 0a16908 to c04f9d6 Compare September 9, 2017 19:42

mozilla deleted a comment from pdfjsbot Sep 9, 2017

timvandermeij merged commit 320779e into mozilla:master Sep 9, 2017

timvandermeij deleted the square-circle-annotations branch September 9, 2017 20:56

timvandermeij mentioned this pull request Sep 12, 2017

error running node pdf2svg.js #8902

Closed

timvandermeij mentioned this pull request Sep 12, 2017

Provide a stub for setAttribute in order to use the SVG back-end with Node.js #8904

Merged

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#8691 from timvandermeij/square-circle-anno…

11ec498

…tations Implement support for square and circle annotations

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for square and circle annotations #8691

Implement support for square and circle annotations #8691

timvandermeij commented Jul 23, 2017

timvandermeij commented Jul 23, 2017

timvandermeij commented Jul 23, 2017

timvandermeij commented Jul 31, 2017 •

edited

Loading

timvandermeij commented Aug 31, 2017 •

edited

Loading

yurydelendik left a comment •

edited

Loading

yurydelendik Sep 8, 2017

yurydelendik Sep 8, 2017

yurydelendik Sep 8, 2017

timvandermeij commented Sep 9, 2017 •

edited

Loading

timvandermeij commented Sep 9, 2017

pdfjsbot commented Sep 9, 2017

pdfjsbot commented Sep 9, 2017

timvandermeij commented Sep 9, 2017

pdfjsbot commented Sep 9, 2017

pdfjsbot commented Sep 9, 2017

pdfjsbot commented Sep 9, 2017

pdfjsbot commented Sep 9, 2017

timvandermeij commented Sep 9, 2017

pdfjsbot commented Sep 9, 2017

pdfjsbot commented Sep 9, 2017

pdfjsbot commented Sep 9, 2017

pdfjsbot commented Sep 9, 2017

Implement support for square and circle annotations #8691

Implement support for square and circle annotations #8691

Conversation

timvandermeij commented Jul 23, 2017

timvandermeij commented Jul 23, 2017

timvandermeij commented Jul 23, 2017

timvandermeij commented Jul 31, 2017 • edited Loading

timvandermeij commented Aug 31, 2017 • edited Loading

yurydelendik left a comment • edited Loading

Choose a reason for hiding this comment

yurydelendik Sep 8, 2017

Choose a reason for hiding this comment

yurydelendik Sep 8, 2017

Choose a reason for hiding this comment

yurydelendik Sep 8, 2017

Choose a reason for hiding this comment

timvandermeij commented Sep 9, 2017 • edited Loading

timvandermeij commented Sep 9, 2017

pdfjsbot commented Sep 9, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented Sep 9, 2017

From: Bot.io (Linux m4)

Success

Published

timvandermeij commented Sep 9, 2017

pdfjsbot commented Sep 9, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented Sep 9, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Sep 9, 2017

From: Bot.io (Linux m4)

Success

pdfjsbot commented Sep 9, 2017

From: Bot.io (Windows)

Success

timvandermeij commented Sep 9, 2017

pdfjsbot commented Sep 9, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented Sep 9, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Sep 9, 2017

From: Bot.io (Linux m4)

Success

pdfjsbot commented Sep 9, 2017

From: Bot.io (Windows)

Success

timvandermeij commented Jul 31, 2017 •

edited

Loading

timvandermeij commented Aug 31, 2017 •

edited

Loading

yurydelendik left a comment •

edited

Loading

timvandermeij commented Sep 9, 2017 •

edited

Loading