[api-minor] Move annotation DOM manipulation logic to src/display/annotation_layer.js #6714
Conversation
Naming it this way makes more sense when compared to the core annoatation code. That code also uses `data` for the annotation properties.
timvandermeij
force-pushed
the
annotation-web-to-src
branch
from
e6a4bb2
to
89e7500
Compare
Additionally simplify the div creation logic (it needs to happen only once, so it should not be in the for-loop) and remove/rename variables for shorter code.
timvandermeij
force-pushed
the
annotation-web-to-src
branch
from
89e7500
to
bce3214
Compare
| container.style.left = data.rect[0] + 'px'; | ||
| container.style.top = data.rect[1] + 'px'; | ||
|
|
||
| // Size |
There was a problem hiding this comment.
Let's remove trivial comment such as "Size", "Border" or "Border color". If we can, let's add a sentence about what we are planing to do instead.
There was a problem hiding this comment.
Done in b1937e7, also for the existing border style code. The code is indeed very readable already without these comments.
|
|
||
| self.div = document.createElement('div'); | ||
| self.div.className = 'annotationLayer'; | ||
| self.pageDiv.appendChild(self.div); |
There was a problem hiding this comment.
We can create two functions "updateAnnotations" and "renderAnnotations" for PDFJS.AnnotationUtils that accept div, annotations, etc. and move logic of this if branches there. We can keep logic of the div creation and annotations.length check here.
There was a problem hiding this comment.
Done in 7e15034.
|
Looks good after comments are addressed. |
timvandermeij
changed the title
timvandermeij
force-pushed
the
annotation-web-to-src
branch
from
7e15034
to
edce8da
Compare
timvandermeij
changed the title
|
/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/b0ca424f1f902ed/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/b0ca424f1f902ed/output.txt Total script time: 0.81 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/6f2fc4f4d1c7ee3/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/5d2c0c873f5dd05/output.txt |
| PDFJS.AnnotationLayer.render(viewport, self.div, annotations, | ||
| self.pdfPage, self.linkService); | ||
| if (typeof mozL10n !== 'undefined') { | ||
| mozL10n.translate(self.div); |
There was a problem hiding this comment.
Does this line actually do the same thing as before, since the current code attempts to translate the individual annotation elements, and not the entire annotationLayerdiv?
There was a problem hiding this comment.
yes, but we don't want to move mozL10n dependency to the src/display/
There was a problem hiding this comment.
I have verified that mozL10n.translate() does work on the entire div too instead of only on individual annotation elements. I have tested this with PDFs with only one or multiple annotations per page and each annotation was properly translated. Therefore this seems to do the same thing as before. As Yury mentioned, it's nicer to keep localization in the web folder as it does not belong in the core code.
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/5d2c0c873f5dd05/output.txt Total script time: 19.23 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/6f2fc4f4d1c7ee3/output.txt Total script time: 20.16 mins
|
|
Merging with r=yurydelendik and r=me. |
[api-minor] Move annotation DOM manipulation logic to src/display/annotation_layer.js
Fixes #6689 (as the testing part is tracked in #6673). It is also another part of #5218.
I have tested this PR with a set of 20 PDF files with various kinds of annotations (Link, Text, Widget, et cetera) and found no changes in rendering, behavior or printing.
Note that I intentionally used multiple commits to make reviewing easier and to split renaming code and moving code as much as possible.