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

Remove ttp:version parameter attribute (#435). #606

Merged
merged 2 commits into from
Feb 12, 2018

Conversation

skynavga
Copy link
Collaborator

Closes #435.

@skynavga skynavga added this to the Editor's CR Work List milestone Jan 30, 2018
@skynavga skynavga self-assigned this Jan 30, 2018
@@ -13006,9 +12920,6 @@ example.</p>
<p>The <att>tts:textShadow</att> attribute is used to specify a style property that
defines one or more text shadow decorations to apply to glyphs that are selected for <loc href="#terms-glyph-area">glyph areas</loc> generated
by content flowed into a region.</p>
<p>If both <loc href="#style-attribute-textOutline"><att>tts:textOutline</att></loc> and <att>tts:textShadow</att> attributes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this related to the version attribute?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had previously thought that tts:textShadow (a TTML2 feature) would replace tts:textOutline (a TTML1 feature). We now understand it isn't a replacement (driven by version) but both apply independently.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how this was driven by version and how this is related to ttp:version but I think the change is good. Approving

@cconcolato
Copy link
Contributor

@cconcolato
Copy link
Contributor

@skynavga, just to make sure we have covered every aspect, I'm copying the results of your study of TTT and checking how this change affects them:

  • [ttv] select between version specific default profiles
    This seems covered by the use of TTML2 profiles as the default given that it is a superset of TTML1.

  • [ttv] select between TTML schemas associated with different specification versions (56 instances)
    Same as above. At some point, we had mentioned that a validator could choose the schema corresponding to the minimal profile that includes all required features. I think this is not needed for now.

  • [ttv] contextualize length expressions in terms of version (in order to support new units) (2 instances)
    Can you clarify how this is handled?

  • [ttv] enable deprecation warnings (1 instance)
    I think this is covered by the selection of a profile.

  • [ttpe] select use of initial value of tts:position versus tts:origin (1 instance)
    We fixed this with Consider aligning initial values of tts:position and tts:origin. #551

  • [ttx] select between tt element and region element for root of style inheritance (1 instance)
    This is covered by #546

  • [ttx] select between version specific transformation helper class methods used during ISD transformation processing (24 instances)
    This seems internal to your software.

@skynavga
Copy link
Collaborator Author

@cconcolato thanks for checking my TTT study; there are a few things I need to add/tweak to cover these points

Copy link
Contributor

@nigelmegitt nigelmegitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great pull request, approving as is.

One question in my mind, possibly for a later change: should we also remove @version from isd:isd?

@skynavga
Copy link
Collaborator Author

skynavga commented Jan 30, 2018

@nigelmegitt I reviewed @version on ISD vocabulary, and decided it was appropriate there, since we don't profile the ISD vocabulary usage via features from which we might infer the ISD version.

@nigelmegitt
Copy link
Contributor

@skynavga OK, I'm not sure about that but no objection from me to keep it in and moving on.

@skynavga skynavga merged commit 140fd8f into master Feb 12, 2018
@skynavga skynavga removed their assignment Feb 12, 2018
@skynavga skynavga deleted the issue-0435-remove-version-parameter branch March 9, 2018 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants