Refactor annotation code to use a factory #6273
Conversation
timvandermeij
force-pushed
the
annotation-factory
branch
from
357227f
to
aa9d120
Compare
| return; | ||
| } | ||
|
|
||
| // Determine the annotation's subtype |
There was a problem hiding this comment.
Nit: Can you add a period at the end of the sentence, both here and below :)
There was a problem hiding this comment.
Done in the new commit.
timvandermeij
force-pushed
the
annotation-factory
branch
from
aa9d120
to
7f7e4ea
Compare
| var subtype = dict.get('Subtype'); | ||
| subtype = isName(subtype) ? subtype.name : ''; | ||
|
|
||
| // Determine the annotation's field type. |
There was a problem hiding this comment.
is this prepared for other annotations which have a field type? because i think this could be also done on demand on below switch and done only when actually needed
There was a problem hiding this comment.
Good catch! Only Widget annotations (for interactive forms) use field types and that is also how the current code uses it, so we might as well get it over there where it is really needed. Fixed in the new commit.
There was a problem hiding this comment.
By the way, the test suite contains annotation-tx.pdf to verify that this change still works as expected.
|
lgtm |
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.
timvandermeij
force-pushed
the
annotation-factory
branch
from
7f7e4ea
to
4f920ad
Compare
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/2976fbf6657adee/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/2976fbf6657adee/output.txt Total script time: 0.63 mins Published |
|
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/bfad4c828ed68e9/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://107.22.172.223:8877/0c61e98948fa15c/output.txt |
From: Bot.io (Windows)FailedFull output at http://107.22.172.223:8877/0c61e98948fa15c/output.txt Total script time: 2.98 mins
Image differences available at: http://107.22.172.223:8877/0c61e98948fa15c/reftest-analyzer.html#web=eq.log |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/bfad4c828ed68e9/output.txt Total script time: 18.92 mins
|
|
/botio-windows test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/004ce1855676f7b/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/004ce1855676f7b/output.txt Total script time: 18.58 mins
|
Refactor annotation code to use a factory
|
Improved code structure, and a net reduction in number of code lines, that's always nice :) |
| return new WidgetAnnotation(parameters); | ||
|
|
||
| default: | ||
| warn('Unimplemented annotation type "' + subtype + '", ' + |
There was a problem hiding this comment.
This message is shown when loading a document with highlights. Is this expected? Highlight annotations are rendered fine and it doesn't need any special implementation (unless there is plan to support interactive annotations).
pdf.worker.js:224 Warning: Unimplemented annotation type "Highlight", falling back to base annotation
There was a problem hiding this comment.
Yes, this is expected as it was not listed as supported in the original code. This patch just refactored the original code and I did not do further checks on the other annotation types yet. We will get to the other annotation types though, so if there is indeed nothing else to be done for Highlight annotations, then that will be changed.
Currently,
src/core/core.jsuses thefromRefmethod on anAnnotationobject to obtain the right annotation type object (such asLinkAnnotationorTextAnnotation). That method in turn uses a methodgetConstructorto 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
Annotationclass at all. Creation of annotation object should be done by a factory (as also recommended by @yurydelendik at #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.
@Snuffleupagus Would you be so kind again to review this? To test this, you can use all PDFs in issues with the
4-annotationlabel (both open and closed) and check rendering and the console before and after the patch. If you want even more test files, I can provide the 20 test PDFs I used to test this, but I think the4-annotationlabel contains enough PDFs to properly verify this. A sample PDF to see that warnings are now properly put in the console is http://mirrors.rit.edu/CTAN/macros/latex/contrib/pdfcomment/doc/example.pdf.