Refactor annotation display layer code to use classes #6770

timvandermeij · 2015-12-17T15:10:47Z

This patch refactors the annotation display layer code to use classes. This work makes it easier to add new annotation types and improves readability and testability of the code. Feedback is appreciated.

Most of this patch is moving existing code. The following structural additions have been made:

Introduction of AnnotationElementFactory, which is based on the code in src/core/annotation.js, to handle returning the correct annotation element type object.
Introduction of AnnotationElement, a base class for other annotation element types that is responsible for creating a generic empty container.
Introduction of LinkAnnotationElement, responsible for rendering a link annotation element. Contains existing code, but _bindLink and _bindNamedAction have been moved to separate methods where they were previously inlined.
Introduction of TextAnnotationElement, responsible for rendering a text annotation element. Contains existing code, but _toggle, _show and _hide have been moved to separate methods where they were previously inlined.
Introduction of WidgetAnnotationElement, responsible for rendering a widget annotation element. Contains existing code with _setTextStyle as a method here instead of an overall function before.

Aside from the above, JSDoc comments have been added.

I have tested this with a set of 23 PDF files containing different annotation types, where Link, Text and Widget are covered. I found no difference in behavior, rendering or printing before and after the patch.

Snuffleupagus · 2015-12-18T10:02:04Z

src/display/annotation_layer.js

+     * @private
+     * @memberof AnnotationElement
+     */
+    _makeContainer: function AnnotationElement_makeContainer() {


Nit: I think that calling this _createContainer instead feels like better English :)

(Please note: I've only looked through the diff quickly so far, and I still want to go through the code more thoroughly before signing off on it. I'll try to do that during the weekend.)

Done in the new commit.

Snuffleupagus · 2015-12-18T10:18:25Z

I have tested this with a set of 23 PDF files containing different annotation types, where Link, Text and Widget are covered. I found no difference in behavior, rendering or printing before and after the patch.

@timvandermeij Would you be able to look into #6673 next, since having more complete test-coverage for annotations would mean that testing e.g. refactoring PRs like this one would be so much easier?

yurydelendik · 2015-12-18T20:28:03Z

src/display/annotation_layer.js

-    style.fontFamily = fontFamily + fallbackName;
-  }
+/**
+ * @typedef {Object} AnnotationElementOptions


AnnotationElementParameters ?

Done in the new commit.

timvandermeij · 2015-12-19T00:29:52Z

Thank you both. I will address the comments soon. @Snuffleupagus, there is now #6780 for the regression testing, which should address your test coverage concern.

Snuffleupagus · 2015-12-19T11:31:47Z

src/display/annotation_layer.js

+        }, false);
+        image.addEventListener('mouseout', function image_mouseOutHandler() {
+          self._hide();
+        }, false);


I think that you can actually make all of the above event listeners one-liners, e.g. like this:

image.addEventListener('click', this._toggle.bind(this)); ...

Edit: You might not agree with this (for e.g. readability reasons), in that case I'm fine with keeping it as is, but I figured I'd mention it anyway.

Done in the new commit. Note that I needed to add the isBool check and import because with this oneliner the event object is now passed to the listener, where we first expected only to get a boolean value.

I'm really sorry that I didn't think this through properly before!
I believe that you should be able to get rid of the boolean check, and be able to simplify all the event listeners by doing:

image.addEventListener('click', this._toggle.bind(this)); image.addEventListener('mouseover', this._show.bind(this, false)); image.addEventListener('mouseout', this._hide.bind(this, false)); content.addEventListener('click', this._hide.bind(this, true));

I did not know that that was possible, but it works like a charm. The boolean checks and import have been removed and the event listener lines have been updated. I verified with a PDF file with text annotations that the behavior is still the same.

timvandermeij · 2015-12-19T18:36:50Z

/botio-linux preview

pdfjsbot · 2015-12-19T18:36:51Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/025192a848975ce/output.txt

pdfjsbot · 2015-12-19T18:37:37Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/025192a848975ce/output.txt

Total script time: 0.77 mins

Published

timvandermeij · 2015-12-19T18:39:44Z

/botio test

pdfjsbot · 2015-12-19T18:39:45Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/c4b1be218642960/output.txt

pdfjsbot · 2015-12-19T18:39:45Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/0adf36a9e4cf829/output.txt

pdfjsbot · 2015-12-19T18:59:54Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/0adf36a9e4cf829/output.txt

Total script time: 20.16 mins

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

pdfjsbot · 2015-12-19T19:00:23Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/c4b1be218642960/output.txt

Total script time: 20.63 mins

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

Refactor annotation display layer code to use classes

Snuffleupagus · 2015-12-20T12:04:56Z

Looks good, thank you for the patch!

Also, thanks for your continued efforts in improving both the annotation code and the related tests!

xlc · 2016-01-05T02:38:06Z

src/display/annotation_layer.js

-          warn('Unimplemented border style: beveled');
-          break;
+      default:
+        throw new Error('Unimplemented annotation type "' + subtype + '"');


What is the reason to throw error instead of warn it and just don't render anything? I don't think there are any code that catches this error so this error is a fatal error that will propagate to top level and stop the annotation rendering process. I think this is also the reason why #6818 failed (but I still prefer #6786 why of doing it)

This PR has just been refactoring, so it was like this already before the PR. I think the main reason is that we should just not render any HTML elements for unsupported annotation types. Note that this is not the main reason the hasHtml removal patch failed: that was because we tried to call render on a non-object because there is no annotation element class for unsupported annotation types. Of course, if you think improvements are possible, please open a separate issue or a PR.

timvandermeij added the annotations label Dec 17, 2015

timvandermeij assigned Snuffleupagus Dec 17, 2015

timvandermeij force-pushed the annotation-display-classes branch from 62e7c61 to 69d287f Compare December 17, 2015 15:17

Snuffleupagus reviewed Dec 18, 2015
View reviewed changes

timvandermeij force-pushed the annotation-display-classes branch from 69d287f to a85100e Compare December 18, 2015 20:17

yurydelendik reviewed Dec 18, 2015
View reviewed changes

Snuffleupagus reviewed Dec 19, 2015
View reviewed changes

timvandermeij force-pushed the annotation-display-classes branch 2 times, most recently from 716c92f to f48c6a6 Compare December 19, 2015 16:00

Refactor annotation display layer code to use classes

5b66ad6

timvandermeij force-pushed the annotation-display-classes branch from f48c6a6 to 5b66ad6 Compare December 19, 2015 18:33

Snuffleupagus added a commit that referenced this pull request Dec 20, 2015

Merge pull request #6770 from timvandermeij/annotation-display-classes

cfc0cc8

Refactor annotation display layer code to use classes

Snuffleupagus merged commit cfc0cc8 into mozilla:master Dec 20, 2015

Snuffleupagus mentioned this pull request Dec 20, 2015

Implement annotation layer regression testing #6780

Merged

timvandermeij deleted the annotation-display-classes branch December 20, 2015 12:38

timvandermeij mentioned this pull request Dec 20, 2015

Add video annotation support #5450

Closed

Snuffleupagus mentioned this pull request Dec 26, 2015

Implement support for Popup annotations #6792

Merged

xlc reviewed Jan 5, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor annotation display layer code to use classes #6770

Refactor annotation display layer code to use classes #6770

timvandermeij commented Dec 17, 2015

Snuffleupagus Dec 18, 2015

timvandermeij Dec 18, 2015

Snuffleupagus commented Dec 18, 2015

yurydelendik Dec 18, 2015

timvandermeij Dec 19, 2015

timvandermeij commented Dec 19, 2015

Snuffleupagus Dec 19, 2015

timvandermeij Dec 19, 2015

Snuffleupagus Dec 19, 2015

timvandermeij Dec 19, 2015

timvandermeij commented Dec 19, 2015

pdfjsbot commented Dec 19, 2015

pdfjsbot commented Dec 19, 2015

timvandermeij commented Dec 19, 2015

pdfjsbot commented Dec 19, 2015

pdfjsbot commented Dec 19, 2015

pdfjsbot commented Dec 19, 2015

pdfjsbot commented Dec 19, 2015

Snuffleupagus commented Dec 20, 2015

xlc Jan 5, 2016

timvandermeij Jan 5, 2016

Refactor annotation display layer code to use classes #6770

Refactor annotation display layer code to use classes #6770

Conversation

timvandermeij commented Dec 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus commented Dec 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented Dec 19, 2015

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

timvandermeij commented Dec 19, 2015

pdfjsbot commented Dec 19, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 19, 2015

From: Bot.io (Linux)

Success

Published

timvandermeij commented Dec 19, 2015

pdfjsbot commented Dec 19, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 19, 2015

From: Bot.io (Windows)

Received

pdfjsbot commented Dec 19, 2015

From: Bot.io (Windows)

Success

pdfjsbot commented Dec 19, 2015

From: Bot.io (Linux)

Success

Snuffleupagus commented Dec 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment