Complete rewrite of the annotation layer #5218

timvandermeij · 2014-08-20T12:59:29Z

My apologies in advance for the wall of text!

This PR aims to provide a drop-in replacement for the current annotation code. In my opinion the current code is not structured enough, incomplete, not documented enough and it lacks basic testing. My goals with this PR are to provide a solid framework to create and present annotations in which we can easily implement new annotation types. It provides the necessary abstraction to do this.

The basis of this PR is the base class Annotation. It contains all information a basic annotation should have. Specific annotation types, such as Link or Text, will extend this base class with their own specific methods (see src/core/annotation_types/link_annotation.js for example). This is not only a nice way to work with annotations, but it's also what the PDF specification recommends.

The existing AnnotationUtils class and web/page_view.js have been rewritten to work with this new framework, but they have also been simplified a lot because most logic is now where it belongs: either in the annotation object itself or in annotation_handler.js. I wanted to avoid having styles for example being applied in many places, which is what happens in the current code. Doing so makes sure that for example page_view.js only deals with printing the annotations and not also with styling them anymore.

Aside from rewriting the current annotation layer for the sake of readability, structure and maintainability, I also felt that the current implementation was incomplete at some points and almost none of it was documented. What I did was read the entire part of the PDF specification on annotations and made an exhaustive overview of all features the annotation layer should have. This has been done so we know exactly what we do and do not support currently. This can be found in the implementation overview document (see below).

After having made the abovementioned overview, I have also implemented some new features that were missing in the previous implementation layer. Examples are:

Implemented full support for radii in a Border dictionary.
Implemented full support for colorspaces used by annotations.
Implemented full support for the 'underline' border style.
Implemented partial support for the 'dashed' border style (because of CSS limitations we can also show the default dashed border style, but it's better than showing nothing).
Improved handling of GoToR link types.

Finally, after rewriting everything and adding new features, I decided to fix the lack of tests. I have added unit tests for practically all features of the annotation layer that are currently implemented.

This is still a work in progress. The to do list can be found in the implementation overview document (see below).

I have prepared a ZIP file with eight test documents for the Link annotation and the implementation overview document as referenced above. This file can be downloaded from http://www39.zippyshare.com/v/28118637/file.html.

Rendering of LinkAnnotation3.pdf seems better to me than in Reader (because we actually use the border width as per spec and Reader seems to ignore this), but is OK to do this differently?

CodingFabian · 2014-08-20T13:13:57Z

src/core/annotation.js

+    this.date = null;
+    this.rectangle = [0, 0, 0, 0];
+    this.borderStyle = null;
+    this.color = [0, 0, 0];


as written on the other ticket i think color should be null.
The color dictionary is optional. if an annotation does not contain it, the pdf spec says that the color shall be transparent. right now it is initialized with black.

CodingFabian · 2014-08-20T13:26:30Z

great work! I had a few inline comments. This new implementation now supports less annotations than the current one, right?

timvandermeij · 2014-08-20T13:28:52Z

@CodingFabian Thank you for the review comments! I'll fix them soon. Right now this implementation only supports the Link annotation, but we need to at least add the annotation types supported in the current implementation before we can land this. It's part of my to do list which can now be found in the PR description.

CodingFabian · 2014-08-20T13:32:30Z

i cannot judge on the OO style you used, because I primarily develop Java. the actual logic code looks fine for me. as you can see in the overall diff its actually quite similar to the existing code.

yurydelendik · 2014-08-20T13:45:25Z

i cannot judge on the OO style you used, because I primarily develop Java.

Yes, you can. AnnotationLayer and AnnotationHandler bad abstractions here, closest pattern that can be used here is a Factory. Which mean appendToOperatorList does not belong here.

I would just ask to postpone splitting classes in their own files -- that will delay landing of this patch since we have to make this decision project wide (and probably it will involve not just placing code into separate file, but code format and global scope usage).

CodingFabian · 2014-08-20T14:29:54Z

thats exactly something I did not want to judge because i neither know JS nor pdfjs best ractices for that :)

Snuffleupagus · 2014-08-20T20:48:16Z

src/core/annotation.js

+     * Set the annotation ID.
+     *
+     * @access public
+     * @param int id  the annotation ID


Nit: the type is usually enclosed in curly braces, i.e. {int}. Compare with e.g. api.js#L175-L196.
The same applies for all the type definitions in this PR :-)

Snuffleupagus · 2014-08-31T14:21:32Z

src/core/annotation.js

+    this.name = null;
+    this.date = null;
+    this.rectangle = null;
+    this.borderStyle = new AnnotationBorderStyle();


Does every kind of Annotation contain a Border entry? If not, you might want to initialize this lazily instead.

To initialize it lazily, you remove this line and instead add e.g. the following method to the prototype:

get borderStyle() { return shadow(this, 'borderStyle', new AnnotationBorderStyle()); },

yurydelendik · 2014-12-18T19:10:51Z

blocked by #5552

yurydelendik · 2014-12-18T22:36:04Z

web/page_view.js

+      if (data.action && data.action.type === 'GoToR') {
+        link.href = PDFView.getAnchorUrl(data.action.url + '#' +
+                                         escape(destination));
+        if (data.action.newWindow) {


It's better to avoid inclusion of the new functionality into a "refactoring" PR. We need the separate PR for that.

@yurydelendik

Currently, `src/core/core.js` uses the `fromRef` method on an `Annotation` object to obtain the right annotation type object (such as `LinkAnnotation` or `TextAnnotation`). That method in turn uses a method `getConstructor` to find out which annotation type object must be returned. Aside from the fact that there is currently a lot of code to achieve this, these methods should not be part of the base `Annotation` class at all. Creation of annotation object should be done by a factory (as also recommended by @yurydelendik at mozilla#5218 (comment)) that handles finding out the correct annotation type object and returning it. This patch implements this separation of concerns. Doing this allows us to also simplify the code quite a bit and to make it more readable. Additionally, we are now able to get rid of the hardcoded array of supported annotation types. The factory takes care of checking the annotation types and falls back to returning the base annotation type (and issuing a warning, which the current code also does not do well) when an annotation type is unsupported. I have manually tested this commit with 20 test PDFs with different annotation types, such as /Link, /Text, /Widget, /FileAttachment and /FreeText. All render identically before and after the patch, and unsupported annotation types are now properly indicated with a warning in the console.

@yurydelendik

Currently, `src/core/core.js` uses the `fromRef` method on an `Annotation` object to obtain the right annotation type object (such as `LinkAnnotation` or `TextAnnotation`). That method in turn uses a method `getConstructor` to find out which annotation type object must be returned. Aside from the fact that there is currently a lot of code to achieve this, these methods should not be part of the base `Annotation` class at all. Creation of annotation object should be done by a factory (as also recommended by @yurydelendik at mozilla#5218 (comment)) that handles finding out the correct annotation type object and returning it. This patch implements this separation of concerns. Doing this allows us to also simplify the code quite a bit and to make it more readable. Additionally, we are now able to get rid of the hardcoded array of supported annotation types. The factory takes care of checking the annotation types and falls back to returning the base annotation type (and issuing a warning, which the current code also does not do well) when an annotation type is unsupported. I have manually tested this commit with 20 test PDFs with different annotation types, such as /Link, /Text, /Widget, /FileAttachment and /FreeText. All render identically before and after the patch, and unsupported annotation types are now properly indicated with a warning in the console.

@yurydelendik

Currently, `src/core/core.js` uses the `fromRef` method on an `Annotation` object to obtain the right annotation type object (such as `LinkAnnotation` or `TextAnnotation`). That method in turn uses a method `getConstructor` to find out which annotation type object must be returned. Aside from the fact that there is currently a lot of code to achieve this, these methods should not be part of the base `Annotation` class at all. Creation of annotation object should be done by a factory (as also recommended by @yurydelendik at mozilla#5218 (comment)) that handles finding out the correct annotation type object and returning it. This patch implements this separation of concerns. Doing this allows us to also simplify the code quite a bit and to make it more readable. Additionally, we are now able to get rid of the hardcoded array of supported annotation types. The factory takes care of checking the annotation types and falls back to returning the base annotation type (and issuing a warning, which the current code also does not do well) when an annotation type is unsupported. I have manually tested this commit with 20 test PDFs with different annotation types, such as /Link, /Text, /Widget, /FileAttachment and /FreeText. All render identically before and after the patch, and unsupported annotation types are now properly indicated with a warning in the console.

@yurydelendik

Currently, `src/core/core.js` uses the `fromRef` method on an `Annotation` object to obtain the right annotation type object (such as `LinkAnnotation` or `TextAnnotation`). That method in turn uses a method `getConstructor` to find out which annotation type object must be returned. Aside from the fact that there is currently a lot of code to achieve this, these methods should not be part of the base `Annotation` class at all. Creation of annotation object should be done by a factory (as also recommended by @yurydelendik at mozilla#5218 (comment)) that handles finding out the correct annotation type object and returning it. This patch implements this separation of concerns. Doing this allows us to also simplify the code quite a bit and to make it more readable. Additionally, we are now able to get rid of the hardcoded array of supported annotation types. The factory takes care of checking the annotation types and falls back to returning the base annotation type (and issuing a warning, which the current code also does not do well) when an annotation type is unsupported. I have manually tested this commit with 20 test PDFs with different annotation types, such as /Link, /Text, /Widget, /FileAttachment and /FreeText. All render identically before and after the patch, and unsupported annotation types are now properly indicated with a warning in the console.

yurydelendik · 2015-11-10T20:47:56Z

@timvandermeij can we revive this PR?

timvandermeij · 2015-11-10T23:15:35Z

@yurydelendik Yes, I hope to work on this again soon. The goal is to split this PR into multiple smaller PRs. Actually a lot of this PR is already merged by #5585, #6273, #6239, #6232, #6214 and more listed here: https://github.com/mozilla/pdf.js/pulls?utf8=%E2%9C%93&q=is%3Aclosed+is%3Apr+annotation

I hope to get the last bits of this PR into separate PRs soon such that we can close this one. I have set this to work in progress such that I constantly get reminded to finish this PR, and fortunately a lot has already been done by the above PRs, which are essentially self-contained parts of this PR for easier review.

mitar · 2015-11-10T23:20:38Z

It would be really great if also #5283 could be addressed. :-)

timvandermeij · 2015-11-10T23:29:20Z

This PR is more about refactoring the current code, but it should make the implementation of such an API easier.

This patch makes it possible to set and get all possible flags that the PDF specification defines. Even though we do not support all possible annotation types and not all possible annotation flags yet, this general framework makes it easy to access all flags for each annotation such that annotation type implementations can use with this information. We add constants for all possible annotation flags such that we do not need to hardcode the flags in the code anymore. The `isViewable()` and `isPrintable()` methods are now much easier to read. Additionally, unit tests have been added to ensure correct behavior. This is another part of mozilla#5218.

This patch makes it possible to set and get all possible flags that the PDF specification defines. Even though we do not support all possible annotation types and not all possible annotation flags yet, this general framework makes it easy to access all flags for each annotation such that annotation type implementations can use this information. We add constants for all possible annotation flags such that we do not need to hardcode the flags in the code anymore. The `isViewable()` and `isPrintable()` methods are now easier to read. Additionally, unit tests have been added to ensure correct behavior. This is another part of mozilla#5218.

timvandermeij · 2015-12-21T16:02:33Z

Most parts of this patch are now present in the master branch. The remaining bits are either not necessary anymore due to other rewrites or need to be reconsidered in a separate PR, so I'm closing this one as completed.

timvandermeij added 4-annotations labels Aug 20, 2014

CodingFabian reviewed Aug 20, 2014
View reviewed changes

CodingFabian mentioned this pull request Aug 20, 2014

Improves rendering performance of annotation layers. #5214

Merged

timvandermeij force-pushed the annotation-layer branch 6 times, most recently from 43bf4fa to 76bd337 Compare August 20, 2014 20:27

Snuffleupagus reviewed Aug 20, 2014
View reviewed changes

timvandermeij force-pushed the annotation-layer branch 4 times, most recently from 90eba9f to 0ffb71d Compare August 21, 2014 21:32

timvandermeij mentioned this pull request Aug 22, 2014

Added video annotation #4376

Closed

timvandermeij force-pushed the annotation-layer branch 4 times, most recently from 1164613 to 751b9da Compare August 23, 2014 20:39

Snuffleupagus reviewed Aug 31, 2014
View reviewed changes

yurydelendik reviewed Dec 18, 2014
View reviewed changes

Snuffleupagus mentioned this pull request Dec 27, 2014

Annotation border styles #5585

Merged

timvandermeij mentioned this pull request Mar 2, 2015

Annotation Support Seems Very Brittle #5772

Closed

timvandermeij mentioned this pull request Jul 26, 2015

Refactor annotation code to use a factory #6273

Merged

xlc mentioned this pull request Nov 16, 2015

Issue about rendering PDF annotation/comment #6564

Closed

timvandermeij mentioned this pull request Nov 16, 2015

Basic AcroForms input controls rendering #6172

Closed

timvandermeij mentioned this pull request Nov 21, 2015

Refactor annotation flags code #6672

Merged

timvandermeij mentioned this pull request Nov 23, 2015

Refactor annotation data passing #6680

Closed

timvandermeij mentioned this pull request Dec 2, 2015

[api-minor] Move annotation DOM manipulation logic to src/display/annotation_layer.js #6714

Merged

timvandermeij mentioned this pull request Dec 15, 2015

Implement constants for all annotation types #6757

Merged

timvandermeij closed this Dec 21, 2015

timvandermeij deleted the annotation-layer branch December 21, 2015 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complete rewrite of the annotation layer #5218

Complete rewrite of the annotation layer #5218

timvandermeij commented Aug 20, 2014

CodingFabian Aug 20, 2014

CodingFabian commented Aug 20, 2014

timvandermeij commented Aug 20, 2014

CodingFabian commented Aug 20, 2014

yurydelendik commented Aug 20, 2014

CodingFabian commented Aug 20, 2014

Snuffleupagus Aug 20, 2014

Snuffleupagus Aug 31, 2014

Snuffleupagus Aug 31, 2014

yurydelendik commented Dec 18, 2014

yurydelendik Dec 18, 2014

yurydelendik commented Nov 10, 2015

timvandermeij commented Nov 10, 2015

mitar commented Nov 10, 2015

timvandermeij commented Nov 10, 2015

timvandermeij commented Dec 21, 2015

Complete rewrite of the annotation layer #5218

Complete rewrite of the annotation layer #5218

Conversation

timvandermeij commented Aug 20, 2014

CodingFabian Aug 20, 2014

Choose a reason for hiding this comment

CodingFabian commented Aug 20, 2014

timvandermeij commented Aug 20, 2014

CodingFabian commented Aug 20, 2014

yurydelendik commented Aug 20, 2014

CodingFabian commented Aug 20, 2014

Snuffleupagus Aug 20, 2014

Choose a reason for hiding this comment

Snuffleupagus Aug 31, 2014

Choose a reason for hiding this comment

Snuffleupagus Aug 31, 2014

Choose a reason for hiding this comment

yurydelendik commented Dec 18, 2014

yurydelendik Dec 18, 2014

Choose a reason for hiding this comment

yurydelendik commented Nov 10, 2015

timvandermeij commented Nov 10, 2015

mitar commented Nov 10, 2015

timvandermeij commented Nov 10, 2015

timvandermeij commented Dec 21, 2015