The Wayback Machine - https://web.archive.org/web/20171127200426/https://github.com/mozilla/pdf.js/pull/6714
Skip to content

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from Dec 15, 2015

Conversation

Projects
None yet
4 participants
Contributor

timvandermeij commented Dec 2, 2015

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.

timvandermeij added some commits Dec 2, 2015

Rename `initContainer(item)` to `getContainer(data)`
Naming it this way makes more sense when compared to the core
annoatation code. That code also uses `data` for the annotation
properties.
Move link creation logic to src/display/annotation_layer.js
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.
src/display/annotation_layer.js
+ container.style.left = data.rect[0] + 'px';
+ container.style.top = data.rect[1] + 'px';
+
+ // Size
@yurydelendik

yurydelendik Dec 15, 2015

Contributor

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.

@timvandermeij

timvandermeij Dec 15, 2015

Contributor

Done in b1937e7, also for the existing border style code. The code is indeed very readable already without these comments.

web/annotations_layer_builder.js
+
+ self.div = document.createElement('div');
+ self.div.className = 'annotationLayer';
+ self.pageDiv.appendChild(self.div);
@yurydelendik

yurydelendik Dec 15, 2015

Contributor

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.

Contributor

yurydelendik commented Dec 15, 2015

Looks good after comments are addressed.

@timvandermeij timvandermeij changed the title from Move most annotation rendering logic to src/display/annotation_layer.js to [api-minor] Move most annotation rendering logic to src/display/annotation_layer.js Dec 15, 2015

@timvandermeij timvandermeij changed the title from [api-minor] Move most annotation rendering logic to src/display/annotation_layer.js to [api-minor] Move annotation DOM manipulation logic to src/display/annotation_layer.js Dec 15, 2015

Contributor

timvandermeij commented Dec 15, 2015

/botio-linux preview

Collaborator

pdfjsbot commented Dec 15, 2015

From: Bot.io (Linux)


Received

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

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

Contributor

timvandermeij commented Dec 15, 2015

/botio test

Collaborator

pdfjsbot commented Dec 15, 2015

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/6f2fc4f4d1c7ee3/output.txt

Collaborator

pdfjsbot commented Dec 15, 2015

From: Bot.io (Windows)


Received

Command 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);
@Snuffleupagus

Snuffleupagus Dec 15, 2015

Member

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?

@yurydelendik

yurydelendik Dec 15, 2015

Contributor

yes, but we don't want to move mozL10n dependency to the src/display/

@timvandermeij

timvandermeij Dec 15, 2015

Contributor

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.

Collaborator

pdfjsbot commented Dec 15, 2015

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/5d2c0c873f5dd05/output.txt

Total script time: 19.23 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed
Collaborator

pdfjsbot commented Dec 15, 2015

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/6f2fc4f4d1c7ee3/output.txt

Total script time: 20.16 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed
Contributor

timvandermeij commented Dec 15, 2015

Merging with r=yurydelendik and r=me.

timvandermeij added a commit that referenced this pull request Dec 15, 2015

Merge pull request #6714 from timvandermeij/annotation-web-to-src
[api-minor] Move annotation DOM manipulation logic to src/display/annotation_layer.js

@timvandermeij timvandermeij merged commit 1b5940e into mozilla:master Dec 15, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@timvandermeij timvandermeij deleted the timvandermeij:annotation-web-to-src branch Dec 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can't perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.
Morty Proxy This is a proxified and sanitized view of the page, visit original site.