Handle non-integer Annotation border widths correctly (issue 14203) #14391
Conversation
|
This is my first time Pull request in my life.I apologize if there are any mistakes in this PR. |
There was a problem hiding this comment.
In addition to the comment, please write a better commit message (and PR title/description) since as-is it's really quite difficult to understand this patch without looking at both the code and the relevant issue. (The first line of the commit message should be a short summary of the changes, and then you should also include additional information such as e.g. links to the specification and a good description of what you're changing and why.)
Furthermore, please add an eq test for these changes such that we avoid future regressions.
Also, please make sure that you follow all of https://github.com/mozilla/pdf.js/wiki/Contributing#4-run-lint-and-testing locally to ensure that the patch passes all existing tests.
|
For the commit message, I'd suggest the following wording: |
KouWakai
force-pushed
the
annot-border-correct
branch
from
dd6ad7b to
f5a8dd8
Compare
KouWakai
force-pushed
the
annot-border-correct
branch
from
f5a8dd8 to
d9e0160
Compare
Snuffleupagus
changed the title
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/672384e03d797eb/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.193.163.58:8877/8adcceb2f041632/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/672384e03d797eb/output.txt Total script time: 22.82 mins
Image differences available at: http://54.241.84.105:8877/672384e03d797eb/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/8adcceb2f041632/output.txt Total script time: 42.46 mins
Image differences available at: http://54.193.163.58:8877/8adcceb2f041632/reftest-analyzer.html#web=eq.log |
|
Note that the test results above look good, despite the failures (those tests unfortunately fail intermittently). #14391 (comment) also asked for a new unit-test as well (since you're new to the code-base I'll make a suggestion to help speed this up), and I'd thus suggest changing pdf.js/test/unit/annotation_spec.js Lines 417 to 420 in 41dab8e to the following instead: const borderStyleInt = new AnnotationBorderStyle();
borderStyleInt.setWidth(3);
const borderStyleNum = new AnnotationBorderStyle();
borderStyleNum.setWidth(2.5);
expect(borderStyleInt.width).toEqual(3);
expect(borderStyleNum.width).toEqual(2.5); |
KouWakai
force-pushed
the
annot-border-correct
branch
from
d9e0160 to
702d2d7
Compare
The existing code appears to be wrong, since according to the PDF specification the border width of an Annotation only has to be a number and not specifically an integer. Please see: - https://www.adobe.com/content/dam/acom/en/devnet/pdf/pdfs/PDF32000_2008.pdf#page=392 - https://www.adobe.com/content/dam/acom/en/devnet/pdf/pdfs/PDF32000_2008.pdf#G11.2096210 - https://www.adobe.com/content/dam/acom/en/devnet/pdf/pdfs/PDF32000_2008.pdf#G6.1965562
KouWakai
force-pushed
the
annot-border-correct
branch
from
702d2d7 to
98158b6
Compare
|
Thank you for the suggestion and approved my commit. |
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.241.84.105:8877/a53d125d4ec684d/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/a53d125d4ec684d/output.txt Total script time: 4.50 mins Published |
|
Thank you for fixing this! |
|
/botio makeref |
From: Bot.io (Windows)ReceivedCommand cmd_makeref from @timvandermeij received. Current queue size: 1 Live output at: http://54.193.163.58:8877/b137b96a71a8f09/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_makeref from @timvandermeij received. Current queue size: 0 Live output at: http://54.241.84.105:8877/07c5b21596a63de/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/07c5b21596a63de/output.txt Total script time: 20.14 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/b137b96a71a8f09/output.txt Total script time: 38.75 mins
|
The border width is expected to be 3 but tracing Class Annotation, I found the width actually comes as float 2.99998 so when the width passed to Number.Isinteger in Class AnnotationBorderStyle:setWidth() the width gets lost.
Number.Isinteger makes sure that the value should be integer.Taking it into account,adding Math.round can solve this issue I think.