Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[api-minor] Implement creation/modification date for annotations #10771

Merged
merged 1 commit into from
May 6, 2019

Conversation

timvandermeij
Copy link
Contributor

@timvandermeij timvandermeij commented Apr 27, 2019

This includes the information in the core and display layers. The date parsing logic from the document properties is rewritten according to the specification and now includes unit tests.

Moreover, missing unit tests for the color of a popup annotation have been added.

Finally the styling of the popup is changed slightly to make the text a bit smaller (it's currently quite large in comparison to other viewers) and to make the drop shadow a bit more subtle. The former is done to be able to easily include the modification date in the popup similar to how other viewers do this.

@timvandermeij
Copy link
Contributor Author

/botio-linux preview

@timvandermeij
Copy link
Contributor Author

/botio test

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Apr 27, 2019

Looking briefly at the test results, a couple of things stand out:

  • The "title" now appears to be slightly smaller than the "contents", was that actually intentional?
  • There's a lot of, in my opinion too much, bottom padding/margin/etc. on both the "title" respectively the "contents" lines of the popups.
  • I'm assuming that the {{date}}, {{time}} strings wasn't intentional here :-)

@timvandermeij
Copy link
Contributor Author

timvandermeij commented Apr 28, 2019

The review comments have been addressed, but I haven't been able to find a way to make localization work in the test driver, which is why we see {{date}}, {{time}} in the reference images. It's supposed to be a fallback string in case the localization doesn't provide one, and the l10n provider then inserts the actual arguments there. It looks like we simply don't have this in the test driver since we only call render on a canvas and don't have a PDF viewer active.

I don't really see this as a blocker for this PR because we already cover the date parsing in the unit tests. However, a follow-up ticket to find a solution to this seems like a good idea because we never had this use-case before.

We also need to fix the CI since after a recent Node.js update the canvas library isn't installable anymore. The prebuilt file is not available due to an ABI change and some packages are missing on the CI to be able to compile it ourselves. It's also not possible to compile it ourselves because of this ABI change. If this doesn't get fixed upstream soon, we can switch to the most recent LTS version of Node.js for our builds instead, but I'd really prefer using the latest stable since that is likely to be what our contributors use too...

@timvandermeij timvandermeij force-pushed the annotation-dates branch 2 times, most recently from 775b2f8 to ba8bc71 Compare April 28, 2019 12:41
@mozilla mozilla deleted a comment from pdfjsbot May 4, 2019
@mozilla mozilla deleted a comment from pdfjsbot May 4, 2019
@mozilla mozilla deleted a comment from pdfjsbot May 4, 2019
@mozilla mozilla deleted a comment from pdfjsbot May 4, 2019
@mozilla mozilla deleted a comment from pdfjsbot May 4, 2019
@mozilla mozilla deleted a comment from pdfjsbot May 4, 2019
@timvandermeij
Copy link
Contributor Author

/botio lint

@timvandermeij
Copy link
Contributor Author

/botio-linux preview

@timvandermeij
Copy link
Contributor Author

/botio test

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks generally good; however I do have a couple of comments.

Really sorry about not getting to this PR sooner, since that may have caused you some unnecessary work!

src/core/annotation.js Outdated Show resolved Hide resolved
src/display/display_utils.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
web/pdf_document_properties.js Outdated Show resolved Hide resolved
l10n/en-US/viewer.properties Outdated Show resolved Hide resolved
@timvandermeij timvandermeij force-pushed the annotation-dates branch 2 times, most recently from a7eed22 to 940c3ee Compare May 5, 2019 12:44
This includes the information in the core and display layers. The
date parsing logic from the document properties is rewritten according
to the specification and now includes unit tests.

Moreover, missing unit tests for the color of a popup annotation have
been added.

Finally the styling of the popup is changed slightly to make the text a
bit smaller (it's currently quite large in comparison to other viewers)
and to make the drop shadow a bit more subtle. The former is done to be
able to easily include the modification date in the popup similar to how
other viewers do this.
@mozilla mozilla deleted a comment from pdfjsbot May 5, 2019
@mozilla mozilla deleted a comment from pdfjsbot May 5, 2019
@mozilla mozilla deleted a comment from pdfjsbot May 5, 2019
@mozilla mozilla deleted a comment from pdfjsbot May 5, 2019
@mozilla mozilla deleted a comment from pdfjsbot May 5, 2019
@mozilla mozilla deleted a comment from pdfjsbot May 5, 2019
@mozilla mozilla deleted a comment from pdfjsbot May 5, 2019
@mozilla mozilla deleted a comment from pdfjsbot May 5, 2019
@mozilla mozilla deleted a comment from pdfjsbot May 5, 2019
@mozilla mozilla deleted a comment from pdfjsbot May 5, 2019
@timvandermeij
Copy link
Contributor Author

/botio lint

@pdfjsbot
Copy link

pdfjsbot commented May 5, 2019

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/bc40373a6129263/output.txt

@pdfjsbot
Copy link

pdfjsbot commented May 5, 2019

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/f505a03ac5ca9f9/output.txt

@pdfjsbot
Copy link

pdfjsbot commented May 5, 2019

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/bc40373a6129263/output.txt

Total script time: 1.01 mins

  • Lint: Passed

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with passing tests; thank you!

@pdfjsbot
Copy link

pdfjsbot commented May 5, 2019

From: Bot.io (Windows)


Success

Full output at http://54.215.176.217:8877/f505a03ac5ca9f9/output.txt

Total script time: 2.84 mins

  • Lint: Passed

@timvandermeij
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

pdfjsbot commented May 5, 2019

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/5ddc264023b8a7f/output.txt

@pdfjsbot
Copy link

pdfjsbot commented May 5, 2019

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/bdc2565adbd998d/output.txt

@pdfjsbot
Copy link

pdfjsbot commented May 5, 2019

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/5ddc264023b8a7f/output.txt

Total script time: 17.59 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/5ddc264023b8a7f/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

pdfjsbot commented May 5, 2019

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/bdc2565adbd998d/output.txt

Total script time: 25.40 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/bdc2565adbd998d/reftest-analyzer.html#web=eq.log

@timvandermeij
Copy link
Contributor Author

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented May 6, 2019

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/43129aa8d71b6bb/output.txt

@pdfjsbot
Copy link

pdfjsbot commented May 6, 2019

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/43129aa8d71b6bb/output.txt

Total script time: 1.89 mins

Published

@timvandermeij timvandermeij changed the title Implement creation/modification date for annotations [api-minor] Implement creation/modification date for annotations May 6, 2019
@timvandermeij
Copy link
Contributor Author

/botio makeref

@pdfjsbot
Copy link

pdfjsbot commented May 6, 2019

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/b72fe8f076f1a9e/output.txt

@pdfjsbot
Copy link

pdfjsbot commented May 6, 2019

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/9e1e4a43740a46e/output.txt

@pdfjsbot
Copy link

pdfjsbot commented May 6, 2019

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/b72fe8f076f1a9e/output.txt

Total script time: 16.22 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@pdfjsbot
Copy link

pdfjsbot commented May 6, 2019

From: Bot.io (Windows)


Success

Full output at http://54.215.176.217:8877/9e1e4a43740a46e/output.txt

Total script time: 23.00 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@timvandermeij timvandermeij merged commit 83f6de3 into mozilla:master May 6, 2019
@timvandermeij timvandermeij deleted the annotation-dates branch May 6, 2019 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants