Fix for #7833 - Test for considering using addingFakeSpaces in textMove op compares wrong width/height #7834
Conversation
| @@ -1459,10 +1459,11 @@ var PartialEvaluator = (function PartialEvaluatorClosure() { | |||
| var isSameTextLine = !textState.font ? false : | |||
| ((textState.font.vertical ? args[0] : args[1]) === 0); | |||
| advance = args[0] - args[1]; | |||
| var relativeAdvance = advance - textContentItem.lastAdvanceWidth - textContentItem.lastAdvanceHeight; | |||
There was a problem hiding this comment.
Looks like it's diff that we are calculating below. Also shall we calculate advance this way (instead of introducing relative advance)? Does it require resetting textContentItem.lastAdvanceWidth/lastAdvanceHeight to different value?
There was a problem hiding this comment.
Also, would it make sense to adjust the OPS.setTextMatrix case similarly case as well?
|
@lightman76 Can you introduce a 'text' test case? (or at least point to PDF to verify the fix)? |
|
The PDF file is in #7833. I have edited the PR description to include this so that GitHub will close the issue automatically if this is merged. As for testing, I think we need something like https://github.com/mozilla/pdf.js/pull/5783/files#diff-7776f1033c4a5613443defe1ecb39256. I wonder if we will see the difference with reference tests. |
A standard |
|
Ok - I've updated to move the diff calculation earlier in the OPS.moveText block and removed the relativeAdvance. This also resolved the linting check that failed. As far as OPS.setTextMatrix @Snuffleupagus mentioned in the review of the earlier version of the pull request, I'm pretty new to the guts of the PDF spec, but at first glance, it would appear it suffers from a similar comparison issue. From the same review of the earlier version of the pr by @yurydelendik "Does it require resetting textContentItem.lastAdvanceWidth/lastAdvanceHeight to different value?": Again, I'm pretty new to this but I think you are right, because if there's a series of textMove and showText commands it will continue appending to the same textContentItem since the moveText branch that calls addFakeSpaces then skips flushing the current textContentItem. It seems to me (a bit sleep deprived at the moment though) that this branch doesn't account for the moveText command then in the textContentItem - seems it should add to lastAdvanceWidth/Height and/or the width/height for where this move was supposed to be. I haven't dug in enough to determine if it should just be accounting for the width of the fake spaces inserted or for the full "diff". If anyone is familiar with this code and can help me wrap my head around this, would appreciate any insight. Otherwise will need to look at this with a clearer head, which probably may not happen until next week as I'm traveling right now. |
| @@ -1459,17 +1459,17 @@ var PartialEvaluator = (function PartialEvaluatorClosure() { | |||
| var isSameTextLine = !textState.font ? false : | |||
| ((textState.font.vertical ? args[0] : args[1]) === 0); | |||
| advance = args[0] - args[1]; | |||
| diff = (args[0] - textContentItem.lastAdvanceWidth) - | |||
There was a problem hiding this comment.
It seems like the advance variable above this line is not used anymore, so we can probably remove it.
There was a problem hiding this comment.
good catch. I've updated pr to remove that.
|
I've updated the setTextMatrix to use the diff (distance from last rendered text to the new position) for comparing to the min/max space to consider adding fake spaces. I've also added some test PDFs and test cases in the api_spec.js that exercise the addFakeSpaces cases for the moveText and setTextMatrix cases. These tests failed prior to these changes and now pass. Just saw I have some linter errors - will fix those up and update. |
Instead of adding a large number of test files, how about adding just one PDF file with multiple pages instead? Edit: Also, please don't forget to squash the commits, see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits. |
|
Ah - hadn't thought of that. Will consolidate the test PDFs... |
There was a problem hiding this comment.
I've added a few inline nits, and also a suggestion on how you can make the unit-tests a bit nicer.
A hopefully useful piece of advice, since I noticed that you've got a merge commit above, is to use git rebase rather than git merge when updating a patch series.
Please also remember to squash the commits, see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits.
| @@ -1492,18 +1491,20 @@ var PartialEvaluator = (function PartialEvaluatorClosure() { | |||
| // Optimization to treat same line movement as advance. | |||
| advance = textState.calcTextLineMatrixAdvance( | |||
| args[0], args[1], args[2], args[3], args[4], args[5]); | |||
| if(advance !== null) { | |||
There was a problem hiding this comment.
Nit: missing space between if and (, i.e. this line should read
if (advance !== null) {.
| @@ -40,6 +40,7 @@ | |||
| !issue7544.pdf | |||
| !issue7598.pdf | |||
| !issue7665.pdf | |||
| !issue7833*.pdf | |||
There was a problem hiding this comment.
Nit: you can remove the * now.
| @@ -890,6 +890,141 @@ describe('api', function() { | |||
| done.fail(reason); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('adds whitespace', function() { | |||
There was a problem hiding this comment.
Nit: this isn't a terribly descriptive name for a test, how about something like e.g. the following instead?
gets text content - check insertion of fake spaces (issue 7833)
| @@ -890,6 +890,141 @@ describe('api', function() { | |||
| done.fail(reason); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('adds whitespace', function() { | |||
There was a problem hiding this comment.
Now that you use one PDF file, I think that you should be able to make the unit-test a lot more efficient by only loading the PDF file once, and by using a helper function for getting the text. E.g. something like this: https://gist.github.com/Snuffleupagus/152e9450f568fb03dc4a78e1c86d3aff
|
Thanks for the feedback. Have a deadline coming up later this week. After that will make the suggested changes. |
lightman76
force-pushed
the
text-move-space-fix
branch
from
b374b7e
to
1a2bade
Compare
|
Ok - updated the tests as you suggested - hadn't used the beforeAll before. Much cleaner - thanks! Let me know if there are any other changes needed or other tests I should add around this. Thanks! |
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/ef8920bc03b9afb/output.txt |
From: Bot.io (Linux)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/e7123fe3d16e848/output.txt |
From: Bot.io (Windows)FailedFull output at http://107.22.172.223:8877/ef8920bc03b9afb/output.txt Total script time: 25.36 mins
Image differences available at: http://107.22.172.223:8877/ef8920bc03b9afb/reftest-analyzer.html#web=eq.log |
From: Bot.io (Linux)FailedFull output at http://107.21.233.14:8877/e7123fe3d16e848/output.txt Total script time: 25.93 mins
Image differences available at: http://107.21.233.14:8877/e7123fe3d16e848/reftest-analyzer.html#web=eq.log |
|
@lightman76 Really sorry for the delay with this PR, I think it fell off the radar for a bit! For information about running tests locally, please refer to https://github.com/mozilla/pdf.js/wiki/Contributing and more specifically https://github.com/mozilla/pdf.js/wiki/Contributing#4-run-lint-and-testing. |
…around when addFakeSpaces was triggered. Fixed some lint errors.
lightman76
force-pushed
the
text-move-space-fix
branch
from
1a2bade
to
708a361
Compare
|
/botio test |
|
I've fixed some linting errors that popped up. I'm not having any luck getting the image tests to run locally on my machine (OS X). I tried to generate the reference images from master, however i'm getting some failures there, so it looks like I can't generate the master set then. I tried to see if I could kick off a re-run here with bot.io, but appears it won't listen to me. Going to try to get my linux box setup to build this and see if I have better luck with the image tests there. |
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 0 Live output at: http://107.22.172.223:8877/4623faa81a98589/output.txt |
From: Bot.io (Linux)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 0 Live output at: http://107.21.233.14:8877/98cd36772f75a2f/output.txt |
From: Bot.io (Windows)FailedFull output at http://107.22.172.223:8877/4623faa81a98589/output.txt Total script time: 25.68 mins
Image differences available at: http://107.22.172.223:8877/4623faa81a98589/reftest-analyzer.html#web=eq.log |
From: Bot.io (Linux)FailedFull output at http://107.21.233.14:8877/98cd36772f75a2f/output.txt Total script time: 26.20 mins
Image differences available at: http://107.21.233.14:8877/98cd36772f75a2f/reftest-analyzer.html#web=eq.log |
|
Thanks. I'll take a look at those image failures now that this branch is up to date and see what's going on there. |
@lightman76 There are some intermittent failures on mac osx. (you can copy/move generated "tmp" master images for "ref" folder for workaround fix the issue) |
Or disable all the FBF tests temporarily. |
|
Hey all - just wanted let you know I haven't forgotten about this. Been sick and have a major deadline coming up for another project. Will get this back up to date and figure out the remaining issues hopefully in a few weeks. Most of the tests that were failing the screenshots I think are fine, but the text layer positioning is different from the previous version (generally aligns better I think). However there are 2 or 3 that are way off. I need to dig into those PDFs to understand which operations are used and why that's happening. |
|
Awesome! Thank you for letting us know. |
|
No more work has been done here and it's not in a mergeable state now, so I'm closing this. However, the associated issue will remain open with a reference to this PR so the work can be continued later. |
|
@lightman76 Thanks for your hard work on this issue.! Would love to see this fixed! |
The arguments of the textMove (Td) operator are relative to the last text positioning operator. Therefor to determine the remaining unused width to check for wether to consider adding spaces, need to look at the difference in the parameter and the lastAdvanceWidth/Height of the current textContentItem.
Fixes #7833.