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

More minor issues #38

Open
faceless2 opened this issue Nov 3, 2022 · 12 comments
Open

More minor issues #38

faceless2 opened this issue Nov 3, 2022 · 12 comments

Comments

@faceless2
Copy link
Collaborator

faceless2 commented Nov 3, 2022

We've just pulled the latest model and incorporated it so here's some feedback on the changes over the last couple of weeks. Almost all minor, so all in one issue if that's OK.

CertSeedValue.tsv

SignaturePolicyHashAlgorithm has the possible-values in the default-value column

PaperMetaData.tsv

fn:Eval((@ECC>=0) && (((@Symbology==PDF417) && (@ECC<=8)) || (@Symbology==QRCode) && (@ECC<=3))) is troubling because we need the precedence rules betwen && and || to implement. I think you need brackets around the final two, after the ||, to remove ambiguity and match the grammar description which requires them.

ArrayOfCIDGlyphMetricsW.tsv and ArrayOfCIDGlyphMetricsW2.tsv

Key is *, required is true - but the guide says

If "Key" column contains ASTERISK (as a wildcard), then "Required" field must be FALSE

XObjectImage.tsv

You've got a few special tests that include fn:IsPresent(ImageMask) - well, I'm sorry to say I have a file here with /ImageMask false and it's failing. I think the test needs to be rephrased to test if ImageMask=true, which I don't believe is implied by IsPresent based on its description (my emphasis):

asserts that the current row (key or array element) must be present in a PDF if key is present, or when the expression expr is true.

ActionGoToE.tsv

F is marked as required, but it's not:

(Optional) The root document of the target relative to the root document of the source. If this entry is absent, the source and target share the same root document.

Target.tsv

N is required if fn:IsRequired((@R==C) && fn:InMap(trailer::Catalog::Names::EmbeddedFiles)), and is supposed to be disallowed if the inverse condition exist. But the
special case is
[fn:Not(fn:IsPresent((@R!=C) || fn:Not(fn:InMap(trailer::Catalog::Names::EmbeddedFiles))))]
which I think is wrong (and also makes my head hurt). Shouldn't this be
fn:Not(fn:IsPresent((@R==C) && fn:InMap(trailer::Catalog::Names::EmbeddedFiles)))

Also, P is "Required if the value of R is C and the target is associated with a file attachment annotation; otherwise, it shall be absent)", but the nuance of "associated with a file attachment annotation" is not captured. Neither is the "otherwise shall be absent" bit. A has similar requirements, except here "Required" is just set to false.

ArrayOfPages.tsv

You've got a new special-case test here, [fn:Eval(fn:IsPresent(*::SeparationInfo))] - this is a weird test in the spec, which seems to require that EVERY child has SeparationInfo. As I understand the wildcard it would be interpreted as AT LEAST ONE child has SeparationInfo.

Finally, can I just say thank you for merging StandardLayoutAttributesBLSE and friends, which had caused all sorts of grief. New approach is 👍.

petervwyatt added a commit that referenced this issue Nov 3, 2022
petervwyatt added a commit that referenced this issue Nov 3, 2022
@petervwyatt
Copy link
Member

Thanks for the very comprehensive and detailed report!
I've fixed a number of these and also enhanced the grammar validation for wildcards to ensure Required==FALSE (I thought that test was already in place but it wasn't - and I reply on the validator when editing).
More fixes soon...

@petervwyatt
Copy link
Member

Re Standard*Attributes* - see also PDF Errata #226 so this may not be 100% finalized yet.

@petervwyatt
Copy link
Member

petervwyatt commented Nov 3, 2022

Re IsPresent - see also discussion in PDF Errata #6. I'm trying to pick up the finer nuances of wording in the PDF standard. "presence" is clearly a file format requirement, not a processor requirement. Also "presence" alone does NOT require that the key value needs to be the correct type - so if /ImageMask 12.34 or /ImageMask (JunkString) means that ImageMask is present. So explicitly testing for a specific value is not what the spec currently says...

My documentation of the predicates and/or the model itself may not be 100% consistent with these ideas ATM...

@petervwyatt
Copy link
Member

Re predicates on * in ArrayOfPages.tsv: my intention is slightly different and possibly poorly explained.

The * in the "Key" field matches anything as a key or array index (no issues there), but all other fields then apply to each instance of the match. Predicate parsing/naming rules for referencing a key mean that I need to use the * to also match against "the current instance" rather than meaning some other arbitrary key/array index. So a bit clumsy...

@faceless2
Copy link
Collaborator Author

faceless2 commented Nov 3, 2022

then apply to each instance of the match.

sigh, I was worried you'd say that.

Up until now a "path" in a predicate has only had to return a single path - even for wildcards, because fn:IsPresent(a::*) only has to return the first matching item. However fn:IsPresent(*::a) means changing it to return more than one, and that opens a can of worms. While I'm sure you'd never do anything like fn:Eval(*::@a + 1), it would be possible. Could this "match more than one thing" functionality perhaps be limited to IsPresent?

... IsPresent relating to ImageMask

The specific issue I was hitting is /ImageMask false /Mask [8 8] - the text for Mask says "If ImageMask is true, this entry shall not be present", so it refers explicitly to the value, not the presence of the key. ImageMask is not true, it should be allowed but we are failing because the model tests fn:IsPresent(ImageMask).The text is slightly different for Intent ("This value is ignored if ImageMask is true.") and Decode ("If ImageMask is true, the..."), but in all cases it is specifically referring to the value of ImageMask, not its presence.

I do take the point that it's complicated, in particular for Target where it does refer to presence. I realise that's a considerably more complicated phrasing

@petervwyatt
Copy link
Member

petervwyatt commented Nov 3, 2022

Let me review the Target wording - I likely copied from elsewhere that used "... is present" phrasing and didn't notice the use of explicit values. If the ISO spec says "... is true" then I do want Arlington to reflect that directly.
It also likely indicates that wording improvements are required!

@faceless2
Copy link
Collaborator Author

The ImageMask stuff I was on about is in XObjectImage.tsv, but that's great - thanks. It's really shaping up I think, we've audited 800 or so docs as a quick sample and have identified many issues that are low-hanging fruit we can repair fully or semi-automatically, which is very pleasing.

@petervwyatt
Copy link
Member

Re Target:

P is "Required if the value of R is C and the target is associated with a file attachment annotation; otherwise, it shall be absent)", but the nuance of "associated with a file attachment annotation" is not captured. Neither is the "otherwise shall be absent" bit. A has similar requirements, except here "Required" is just set to false.

Can you think of an elegant way to capture "associated with a file attachment annotation" - I'm thinking the fn:PageProperty predicate is not sufficiently explanatory and needs to be replaced with ???. Maybe a highly specialized predicate fn:AssociatedWithAnnot(@P, FileAttachment) and not try to get to a far more specific expression that looks something like @P::Annot[@A] ...::@Subtype==FileAttachment or @P::Annot[@NM==@A] ...::@Subtype==FileAttachment?

To stop false triggering on the insufficient/partial condition, I'll set Required==FALSE for now on P and A until a better solution is identified.

@faceless2
Copy link
Collaborator Author

faceless2 commented Nov 3, 2022

When I filed the issue I'd assumed you'd run with fn:AssociatedWithAnnot(). Targets can be chained so you might be referring an a file annotation inside an embedded document! You'd definitely need a custom predicate for that one.

EDIT: in fact that's a pretty important point now I think about it. Whether target "is located in the EmbeddedFiles name tree" (for "N") or "is associated with a file attachment annotation" (for "R"), you've got no way to determine either of these without a custom predicate because you're potentially talking about a completely different PDF to the current one.

Even with a custom predicate, that's suddenly a very expensive operation - if you're linking to a grandchild, you potentially have to extract and parse the embedded file that is the intermediate step to validate this.

@petervwyatt
Copy link
Member

Summarizing remaining unresolved topics in this issue since I won't get to revisit for at least a week or 2 due to ISO meetings:

  • XObjectImage.tsv - check wording of ImageMask requirements between "... is present" and "... is true/false" and ensure predicates match spec wording precisely even if they are somewhat different. Maybe create a PDF Errata if this is the case.

  • Target.tsv - update predicates to capture the missing requirements around "... the target is associated with a file attachment annotation" for N, P and A keys. Will introduce a new predicate(s) and remove fn:PageProperty

  • ArrayOfPages.tsv - need to improve documentation around the use of * in predicates matching "current row" rather than "every row"

If I missed anything please say so!

petervwyatt added a commit that referenced this issue Dec 20, 2022
@plaisted
Copy link

plaisted commented Mar 5, 2023

@petervwyatt I'm confused by some of the IsPresent usage as well and maybe "Special Cases" in general. The description says:

For a single parameter: asserts that the current row (key or array element) must be present in a PDF if key is present, or when the expression expr is true.

That doesn't seem to match its actual usage, for example in 3DUnits.tsv used under Required column:

fn:IsRequired(fn:IsPresent(TSm) || fn:IsPresent(TSn))

This seems like IsPresent() just returns a boolean if the key is present, and fn:IsRequired actually does the asserts that the current row (key or array element) must be present in a PDF if key is present

I see other uses as well in special cases eg. [fn:IsMeaningful(fn:IsPresent(MS) && fn:IsPresent(P))] where it just appears to be returning a bool.

But some other specials cases examples don't seem consistent:

  • [fn:Eval(fn:IsPresent(*::SeparationInfo))] - wrapped in eval I assume we check the expression and want a True result, which seems to match the pdf spec (ArrayOfPages.tsv)
  • [fn:Eval(fn:Not(fn:IsPresent(EPSG)))] - wrapped in eval with, assume want true response, so this ensures EPSG is not present if WKT is (GeographicCoordinateSystem.tsv)
  • [fn:Not(fn:IsPresent(Vertices))] - not wrapped in eval, is this effectively shorthand for fn:Eval(fn:Not()) and we should evaluate the expression and want the result to be false? (AnnotPolygon.tsv)
  • [fn:Not(fn:IsRequired(fn:IsPresent(IT) && fn:SinceVersion(2.0,(@IT!=Stamp))))] - this one I don't really follow at all, Name is already marked not required so if this is trying to set IsRequired() to false it wouldn't actually do anything. Or does this override the Required column and it's required for conditions that don't match? And why would this go under special cases instead of Required? (AnnotStamp.tsv)

edit:

Another expression that seems odd: [fn:Eval(@A==fn:PageProperty(@P,Annots::@NM))]. I haven't noticed anywhere else accessing a property from a list (does this return a list of the values?), and then == between a single value and a list (assuming we really want some sort of contains functionality) seems inconsistent as well. Would [fn:Eval(fn:ContainsOne(fn:PageProperty(@P,Annots), @A == *::@Nm)] or some variation thereof would be more appropriate? If not extra documentation around how to handle this scenario would be helpful (eg. == for value and list means contains).

@petervwyatt
Copy link
Member

I'm kind of regretting the redundancy of fn:Eval and you have highlighted a few places where it needs to be added to be consistent. Thanks.

Your other comments need further consideration...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants