Add support, in Dict.merge, for merging of "sub"-dictionaries
#12296
Conversation
Snuffleupagus
force-pushed
the
mergeSubDicts
branch
2 times, most recently
from
df5a131 to
66e7e70
Compare
src/core/annotation.js
Outdated
| params.acroForm.get("DR") || | ||
| Dict.empty; | ||
|
|
||
| this.fieldResources = Dict.merge({ |
There was a problem hiding this comment.
In general I'm fine with this change but when saving a form, we copy the resources dictionary:
https://github.com/mozilla/pdf.js/blob/master/src/core/annotation.js#L1040
So at the end it'll increase the file size uselessly, especially if the pdf is well formated.
That lets me think that we don't need to add fieldResources when this one is coming from Acroform...
To avoid that, my idea, when fixing #12294, was to have a kind of lazy merge:
just put in the dict what we really need.
There was a problem hiding this comment.
So at the end it'll increase the file size uselessly, especially if the pdf is well formated.
Given that the resources are usually Refs, that should hopefully reduce the size-impact in general; however I do agree that avoiding this if at all possible would be preferable.
That lets me think that we don't need to add fieldResources when this one is coming from Acroform...
Assuming that you can detect this case easily, and implement it without mutating any existing Dicts, that could perhaps work (although I'm not entirely sure I understand exactly what you have in mind). Please explore this idea further!
(If you do need to merge Dicts, with sub-resources, this patch provides a correct way of doing so.)
To avoid that, my idea, when fixing #12294, was to have a kind of
lazymerge:
just put in the dict what we really need.
The problem was with how it was implemented, since you unfortunately added special-cases (and dare I say hacks) to the general Dict abstraction used throughout the code-base to satisfy just one very specific use-case.
Hence my objections, since that wasn't really maintainable and also would have meant mutating existing Dicts in a way that you absolutely shouldn't do.
I'll remove the src/core/annotation.js changes from this patch, but I think that the better Dict.merge functionality is probably generally useful to have regardless.
There was a problem hiding this comment.
So at the end it'll increase the file size uselessly, especially if the pdf is well formated.
Given that the resources are usually
Refs, that should hopefully reduce the size-impact in general; however I do agree that avoiding this if at all possible would be preferable.That lets me think that we don't need to add fieldResources when this one is coming from Acroform...
Assuming that you can detect this case easily, and implement it without mutating any existing
Dicts, that could perhaps work (although I'm not entire sure I understand exactly what you have in mind). Please explore this idea further!
I'll file a bug for that: my idea is to reduce the file size when saving.
(If you do need to merge
Dicts, with sub-resources, this patch provides a correct way of doing so.)To avoid that, my idea, when fixing #12294, was to have a kind of
lazymerge:
just put in the dict what we really need.The problem was with how it was implemented, since you unfortunately added special-cases (and dare I say hacks) to the general
Dictabstraction used throughout the code-base to satisfy just one very specific use-case.
Hence my objections, since that wasn't really maintainable and also would have meant mutating existingDicts in a way that you absolutely shouldn't do.
Agreed !
Maybe we could create an object LazyMergedDict (extends Dict) which will keep a list of dicts. When we make a get, we have a look in each dict until we find it and then we add the key+value in the dict.
Wdyt ? do you have other ideas ?
I'll remove the
src/core/annotation.jschanges from this patch, but I think that the betterDict.mergefunctionality is probably generally useful to have regardless.
Snuffleupagus
force-pushed
the
mergeSubDicts
branch
3 times, most recently
from
03e193e to
acfb420
Compare
|
/botio unittest |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/cc7a46f67a5fa05/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/a24a0da02b7b5e0/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/a24a0da02b7b5e0/output.txt Total script time: 3.83 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/cc7a46f67a5fa05/output.txt Total script time: 4.93 mins
|
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/9bffb5c01374a63/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/1c25c3098e559f2/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/9bffb5c01374a63/output.txt Total script time: 26.97 mins
Image differences available at: http://54.67.70.0:8877/9bffb5c01374a63/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/1c25c3098e559f2/output.txt Total script time: 29.24 mins
Image differences available at: http://54.215.176.217:8877/1c25c3098e559f2/reftest-analyzer.html#web=eq.log |
This allows for merging of dictionaries one level deeper than previously. This could be useful e.g. for /Resources dictionaries, where you want to e.g. merge their respective /Font dictionaries (and other) together rather than picking just the first one.
Snuffleupagus
force-pushed
the
mergeSubDicts
branch
from
acfb420 to
784a420
Compare
|
Thank you for implementing support for this! |
This allows for merging of dictionaries one level deeper than previously. This could be useful e.g. for /Resources dictionaries, where you want to e.g. merge their respective /Font dictionaries (and other) together rather than picking just the first one.