Annotations: move operator list addition logic to src/core/document.js
#8072
Conversation
timvandermeij
force-pushed
the
annotation-append-operator-list
branch
from
1b246df
to
f663be3
Compare
|
/botio-linux preview |
|
/botio test |
There was a problem hiding this comment.
Sorry about the delay here!
My initial reaction was that this feels like a lot of code to be putting in the Page.getOperatorList method. However, I'm not sure that moving this annotation code to a helper method in the Page "class" would be much better, considering the number of parameters that you'd still need to pass around then.
Since I cannot really argue with the intent of this patch, I suppose that I'm fine with it :-)
However, I've got one (sort of related) suggestion here though: Would it now perhaps make sense to add a helper function for checking the viewability/printability of annotations (e.g. like below)?
diff --git a/src/core/document.js b/src/core/document.js
index 9217710..4b9b988 100644
--- a/src/core/document.js
+++ b/src/core/document.js
@@ -71,6 +71,11 @@ var Page = (function PageClosure() {
var DEFAULT_USER_UNIT = 1.0;
var LETTER_SIZE_MEDIABOX = [0, 0, 612, 792];
+ function isAnnotationRenderable(annotation, intent) {
+ return (intent === 'display' && annotation.viewable) ||
+ (intent === 'print' && annotation.printable);
+ }
+
function Page(pdfManager, xref, pageIndex, pageDict, ref, fontCache) {
this.pdfManager = pdfManager;
this.pageIndex = pageIndex;
@@ -284,8 +289,7 @@ var Page = (function PageClosure() {
var i, ii;
var opListPromises = [];
for (i = 0, ii = annotations.length; i < ii; i++) {
- if ((intent === 'display' && annotations[i].viewable) ||
- (intent === 'print' && annotations[i].printable)) {
+ if (isAnnotationRenderable(annotations[i], intent)) {
opListPromises.push(annotations[i].getOperatorList(
partialEvaluator, task, renderInteractiveForms));
}
@@ -347,13 +351,9 @@ var Page = (function PageClosure() {
var annotations = this.annotations;
var annotationsData = [];
for (var i = 0, n = annotations.length; i < n; ++i) {
- if (intent) {
- if (!(intent === 'display' && annotations[i].viewable) &&
- !(intent === 'print' && annotations[i].printable)) {
- continue;
- }
+ if (!intent || isAnnotationRenderable(annotations[i], intent)) {
+ annotationsData.push(annotations[i].data);
}
- annotationsData.push(annotations[i].data);
}
return annotationsData;
},
timvandermeij
force-pushed
the
annotation-append-operator-list
branch
2 times, most recently
from
9c6ee0b
to
1a85896
Compare
I thought about that as well and came to the same conclusion as you did. In the new patch I managed to remove one more line that wasn't really necessary. In total we're adding around 15 lines to
It most certainly does! I added it to the new patch. This makes the patch size |
|
/botio-linux preview |
| for (i = 0, ii = opLists.length; i < ii; i++) { | ||
| pageOpList.addOpList(opLists[i]); | ||
| } | ||
| pageOpList.addOp(OPS.endAnnotations, []); |
There was a problem hiding this comment.
Nit: For improved readability, considering the importance of flushing the operatorList, I think that it'd be a good idea to add a new line after the annotation OPS here.
There was a problem hiding this comment.
Fixed in the new commit.
src/core/document.js
Outdated
| // Collect the operator list promises for the annotations. Each promise | ||
| // is resolved with the complete operator list for a single annotation. | ||
| var i, ii; | ||
| var opListPromises = []; |
There was a problem hiding this comment.
Nit: I you want to offset the (tiny) increase in diff size from the previous comment, I think that you could simply do var i, ii, opListPromises = []; here instead since all these variables are sort of temporary anyway :-)
There was a problem hiding this comment.
Fixed in the new commit.
Ideally, the `Annotation` class should not have anything to do with the page's operator list. How annotations are added to the page's operator list is logic that belongs in `src/core/document.js` instead where the operator list is constructed. Moreover, some comments have been added to clarify the intent of the code.
timvandermeij
force-pushed
the
annotation-append-operator-list
branch
from
1a85896
to
0739f90
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/9e6b3b52c977444/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/9e6b3b52c977444/output.txt Total script time: 2.19 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/a0e2843f64163a8/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 1 Live output at: http://54.215.176.217:8877/0b8790c920e1197/output.txt |
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/0b8790c920e1197/output.txt Total script time: 21.66 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/a0e2843f64163a8/output.txt Total script time: 26.38 mins
|
…operator-list Annotations: move operator list addition logic to `src/core/document.js`
Ideally, the
Annotationclass should not have anything to do with the page's operator list. How annotations are added to the page's operator list is logic that belongs insrc/core/document.jsinstead where the operator list is constructed.Moreover, some comments have been added to clarify the intent of the code.