[
https://issues.apache.org/jira/browse/PDFBOX-2459?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14545924#comment-14545924
]
John Hewson edited comment on PDFBOX-2459 at 5/15/15 6:34 PM:
--------------------------------------------------------------
{quote}
I don't understand why all field operations should be local if it applies to
inherited attributes. Let's assume the value of a field is inherited so
multiple fields do share the same value if setting the value doesn't change the
inherited only the value for the local field changes but all other fields
inheriting the value will still have the old one.
{quote}
That's actually the correct behaviour, setting the value of a field should
_not_ cause that value to be propagated to the fields siblings. Otherwise it
will be impossible to fill in the individual fields.
{quote}
The alternative would be to iterate over the descendants of the parent from
which the value has been inherited and set the local value also. But now we end
up having only local values instead of inherited ones.
{quote}
There's no need to do that, we expose the parent node in the PD model. All
changes being local is fine, because the end user can access the parent node
and make local changes to that. Such changes will be inherited via the usual
mechanisms already in place.
{quote}
Now obviously there is an issue with always setting the inherited value if
there is one as one might to have a local value for a specific field. My
suggestion would be to make setting the inherited value the default and
optionally - with an additional parameter or so - change the local value.
{quote}
Yes, that's because setting inherited values _can't ever work_, because it
makes it impossible to fill out child fields, or to modify any attribute which
they happen to inherit. Worse still, any attempt to modify any inherited
attribute will clobber the sibling nodes by propagating the change upwards.
That would be a terrible default.
Let's assume that we switch that idea around and make local changes the
default, with setting the inherited value being an additional parameter: _That
still can't work_, because there can be an arbitrary number of parent fields in
the hierarchy, e.g. many levels of inheritance - who's to say which level the
user wants to modify? If your answer is "the closest ancestor which defines
that field" then that will make it impossible to modify any previously
inherited values for that field above that ancestor (e.g. imagine multiple "V"
values being overridden in an inheritance tree). The only plausible approach
would be to add an extra parameter which specifies exactly which level in the
tree the inherited field which we wish to modify is... but that's going to be
very cumbersome and is ultimately unnecessary as we already provide access to
the full parent/child tree via PD.
{quote}
So we should be able to change inherited and local values dependent on the use
case the user has. Assuming a change is always local is as incomplete as the
previous assumption that the change should always be applied to the inherited
value if there was one (if there wasn't an inherited value the old
implementation also used the local value).
{quote}
Local changes are _not_ an incomplete assumption: The user can simply navigate
the tree to find the desired parent node and then make _local_ changes to that
node, which will be inherited automatically (as is already the case). Currently
PDNonTerminalField does not provide a rich API for doing this (users must call
getCOSObject()) but there's no reason why we can't provide such functionality
there if it proves to be necessary (and I'm not sure it is).
The point is that propagating changes "back up" the inheritance tree _can't_
result in anything meaningful in the general case. All it does is clobbers
sibling nodes and results in changes which are local to tree branches while
still preventing general manipulation of the larger tree. An advanced user who
wishes to manipulate the field tree is much better served by walking the tree
themselves via PDNonTerminalField, making _local_ changes to that node, which
will be inherited as normal.
Prior to my change, the simple and obvious use case of setting a local value
for a field which initially inherits its value was impossible. Now setValue()
does exactly that, as one would expect.
was (Author: jahewson):
{quote}
I don't understand why all field operations should be local if it applies to
inherited attributes. Let's assume the value of a field is inherited so
multiple fields do share the same value if setting the value doesn't change the
inherited only the value for the local field changes but all other fields
inheriting the value will still have the old one.
{quote}
That's actually the correct behaviour, setting the value of a field should
_not_ cause that value to be propagated to the fields siblings. Otherwise it
will be impossible to fill in the individual fields.
{quote}
The alternative would be to iterate over the descendants of the parent from
which the value has been inherited and set the local value also. But now we end
up having only local values instead of inherited ones.
{quote}
There's no need to do that, we expose the parent node in the PD model. All
changes being local is fine, because the end user can access the parent node
and make local changes to that. Such changes will be inherited via the usual
mechanisms already in place.
{quote}
Now obviously there is an issue with always setting the inherited value if
there is one as one might to have a local value for a specific field. My
suggestion would be to make setting the inherited value the default and
optionally - with an additional parameter or so - change the local value.
{quote}
Yes, that's because setting inherited values _can't ever work_, because it
makes it impossible to fill out child fields, or to modify any attribute which
they happen to inherit. Worse still, any attempt to modify any inherited
attribute will clobber the sibling nodes by propagating the change upwards.
That would be a terrible default.
Let's assume that we switch that idea around and make local changes the
default, with setting the inherited value being an additional parameter: _That
still can't work_, because there can be an arbitrary number of parent fields in
the hierarchy, e.g. many levels of inheritance - who's to say which level the
user wants to modify? If your answer is "the closest ancestor which defines
that field" then that will make it impossible to modify any previously
inherited values for that field above that ancestor (e.g. imagine multiple "V"
values being overridden in an inheritance tree). The only plausible approach
would be to add an extra parameter which specifies exactly which level in the
tree the inherited field which we wish to modify is... but that's going to be
very cumbersome and is ultimately unnecessary as we already provide access to
the full parent/child tree via PD.
{quote}
So we should be able to change inherited and local values dependent on the use
case the user has. Assuming a change is always local is as incomplete as the
previous assumption that the change should always be applied to the inherited
value if there was one (if there wasn't an inherited value the old
implementation also used the local value).
{quote}
Local changes are _not_ an incomplete assumption: The user can simply navigate
the tree to find the desired parent node and then make _local_ changes to that
node, which will be inherited automatically (as is already the case). Currently
PDNonTerminalField does not provide a rich API for doing this (users must call
getCOSObject()) but there's no reason why we can't provide such functionality
there if it proves to be necessary (and I'm not sure it is).
The point is that propagating changes "back up" the inheritance tree _can't_
result in anything meaningful in the general case. All it does is clobbers
sibling nodes and results in changes which are local to tree branches while
still preventing general manipulation of the larger tree. An advanced user who
wishes to manipulate the field tree is much better served by walking the tree
themselves via PDNonTerminalField, making _local_ changes to that node, which
will be inherited as normal.
Prior to my change, the simple and obvious use case of setting a local value
for a field which inherits its value was impossible. Now setValue() does
exactly that, as one would expect.
> Share functionality between Page Tree and Field Tree
> ----------------------------------------------------
>
> Key: PDFBOX-2459
> URL: https://issues.apache.org/jira/browse/PDFBOX-2459
> Project: PDFBox
> Issue Type: Improvement
> Components: PDModel
> Affects Versions: 2.0.0
> Reporter: Maruan Sahyoun
> Assignee: John Hewson
> Priority: Minor
> Fix For: 2.0.0
>
> Attachments: 001511.pdf, 004324.pdf
>
>
> The PDFs page tree and AcroForms field tree share some common functionality
> e.g. resolving inheritable attributes, iterating through leafs and such which
> could be combined into a PDTree class.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
