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 Script Event Type, use Represents instead #241

Merged
merged 12 commits into from
Sep 28, 2024

Conversation

nigelmegitt
Copy link
Contributor

@nigelmegitt nigelmegitt commented Sep 12, 2024

Closes #227 by removing Script Event Type, reusing Represents instead.

  • Merge registry table entries for eventType and <content-descriptor> (removing the eventType registry)
  • Extract definition of Represents into the Shared Properties section
  • Remove Script Event Type
  • Make Represents an optional property of Script Event and move example that was in Script Event Type up into Script Event under the Represents bullet
  • Add permitted #represents-div feature
  • Update class diagram to remove Script Event Type and add Represents to Script Event

Preview | Diff

@css-meeting-bot
Copy link
Member

The Timed Text Working Group just discussed Remove Script Event Type, use Represents instead w3c/dapt#241, and agreed to the following:

  • SUMMARY: Reviewers to consider the PR and if necessary comment regarding the requirements, in the issue.
The full IRC log of that discussion <nigel> Subtopic: Remove Script Event Type, use Represents instead #241
<nigel> github: https://github.com//pull/241
<nigel> Nigel: [shows pull request preview]
<nigel> .. Question about whether to formalise the relationship when a content descriptor is a subset of another one.
<nigel> Cyril: 2 comments, one editorial, one technical.
<nigel> .. 1. Editorial: there's no question on the intent, merging the fields is a good decision.
<nigel> .. Why did you choose to make it a Shared Property?
<nigel> .. It behaves very similarly to text language source and other attributes.
<nigel> .. 2. Technical: I think we need to better explain the inheritance model that goes with it.
<nigel> q+
<nigel> .. To me, I see Represents as similar to xml:lang or daptm:langSrc - you specify it somewhere and it
<nigel> .. applies to the whole tree, and if you specify it somewhere else it is possibly to narrow down the value
<nigel> .. for that part of the tree, a group of divs or one div.
<nigel> .. For example for a dubbing script a represents at the top level could say "dialog nonDialogSounds"
<nigel> .. and for specific events you would say this is a dialog or a nonDialogSound.
<nigel> .. We shouldn't be able to say something in represents at the top level and then contradict that at a lower level,
<nigel> .. e.g. by not having any nonDialogSounds in the body but claiming it at the root.
<nigel> ack nige
<nigel> Nigel: My thinking here is that there is no inheritance model
<nigel> .. You could make the inheritance model one where you replace an inherited value with one
<nigel> .. specified at a lower level, but additive inheritance would not work.
<nigel> .. Let's say I commission a transcript including dialog and nonDialog sounds, but the media
<nigel> .. has no nonDialogSounds, then it makes sense to have no Script Events that represent nonDialogSounds.
<nigel> [discussion of inheritance, inclusion and exclusion constraints]
<atai> q+
<nigel> Gary: Could the top level one be a default one?
<nigel> Nigel: Could make represents mandatory on Script Events
<nigel> Cyril: Can't have a single Script Event be visual and nonVisual at the same time.
<nigel> ack at
<nigel> Andreas: Why can't we apply the same inheritance rule as xml:lang, so that the lowest
<nigel> .. attribute in the tree defines what the event is, so it's not a restriction, it's just overriding what is above.
<nigel> .. It's a general rule, e.g. for namespaces, xml:lang and others.
<nigel> .. To make this restriction makes it complicated to validate.
<nigel> Nigel: It's a list, and it only applies to the element it is on.
<nigel> Cyril: In that case your idea to make it mandatory on Script Event is probably better.
<nigel> .. It would lead to some verbosity.
<nigel> Gary: An alternative is to have represents be a single item and have a documentRepresents with a list
<nigel> Cyril: Could do that
<nigel> Gary: Then you could do normal inheritance
<nigel> Cyril: It's like langSrc, where the document can have multiple source languages
<nigel> .. I realise that langSrc is just one value but I thought we discussed having multiple values
<nigel> Nigel: Need to close this due to time. Please add comments to the issue clarifying the requirements.
<nigel> Cyril: OK
<nigel> SUMMARY: Reviewers to consider the PR and if necessary comment regarding the requirements, in the issue.

@cconcolato
Copy link
Contributor

If verbosity was not a concern, we should say:

  1. a Script Event shall have a non-empty daptm:represents attribute and maybe, that contains a single value
  2. a Script Event shall not use a value that is not listed directly or indirectly (because it is a subset of a value listed) on the document-level daptm:represents. In the meeting @nigelmegitt said that there could be values listed at the top level that are not listed in script events, e.g. after editing or to indicate that the document tried to represent that dimension but could not find any content.

We could live with that. Alternatively, we could also specify that either:

  1. if daptm:represents is not present on a Script Event, it inherits the first value of the daptm:represents attribute on the document. It would behave like the default language.
  2. or introduce a daptm:defaultRepresents that is used to avoid the use of "first value".

BTW, the property is mandatory on the DAPT script but it shall also be non-empty.

@nigelmegitt
Copy link
Contributor Author

@cconcolato see also #227 (comment)

I'm happy to take the approach you're suggesting - it's one of the options I also considered:

  1. Make daptm:represents mandatory and non-empty and maybe single value on Script Events
  2. Define a sub-type hierarchy in the <content-descriptor> registry. I would prefer to constrain it to 2 levels to minimise implementation complexity, i.e. to prevent the need to generate a tree, which would happen if we permitted a value to be a sub-type of another value that is also a sub-type of a 3rd one, and so forth.

If verbosity is a concern, perhaps we could shorten the attribute name, e.g to repr which is an abbreviation for "represents" or "representation" used in Python and Rust.

@cconcolato
Copy link
Contributor

If verbosity is a concern, perhaps we could shorten the attribute name, e.g to repr which is an abbreviation for "represents" or "representation" used in Python and Rust.

The verbosity concern is not really about the length of the attribute name, but more about the repetition of the attribute in the case the document represents contains a single value that matches all the values on all the script events. But that concern is probably outweighed by the clear identification of what div matches a script event.

@nigelmegitt nigelmegitt force-pushed the issue-0227-change-script-event-type-to-represents branch from 3681360 to 76cdc27 Compare September 19, 2024 13:35
Copy link
Contributor

@cconcolato cconcolato left a comment

Choose a reason for hiding this comment

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

My general comments are:

  • technical: why is 'represents' optional on Script Events? If it remains optional, what should be its value when not specified? I would prefer making it mandatory.
  • technical: I would like to mandate that if a represents value is used at a Script Event level, it (or its corresponding top-level value, if the value is a sublevel) must be present at the document level.
  • editorial: it is not clear to me why we are making it a "shared property" like begin/end. Those are allowed on plenty of objects (Script Event, Mixing Instruction, Audio Recording). Represents is allowed on DAPT Script and Script Event. Language is allwoed on Script Event Description, Text (and also on Script Event because of inheritance). Text Language Source is allowed on DAPT Script, Script Event (by inheritance), Text. I think we should be consistent. My suggestion is maybe to remove the notion of "Shared properties", define a property in one place and allow it in other places by reference.

I added some other minor comments.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@nigelmegitt
Copy link
Contributor Author

This doesn't build properly because of speced/respec#4806

@nigelmegitt
Copy link
Contributor Author

@cconcolato I think all of your comments have now been resolved, please check.

examples/intro-script-with-gain.xml Outdated Show resolved Hide resolved
examples/intro-times-and-text.xml Outdated Show resolved Hide resolved
examples/intro-times-only.xml Show resolved Hide resolved
figures/sources/class-diagram.puml Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
<p>Some of the properties in the DAPT data model are common within more than one object type,
and carry the same semantic everywhere they occur.
These <dfn>shared properties</dfn> are listed in this section.
</p>
<p>Some of the value sets in DAPT are reused across more than one property,
and have the same constraints everywhere they occur.
Copy link
Contributor

Choose a reason for hiding this comment

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

Represents and Script Represents don't really have put the same constraints on content-descriptor ... Maybe we should remove the end of the sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no difference to the constraints on <content-descriptor>, there's a difference in how many content descriptors are permitted in Script Represents vs Represents, but that difference isn't described in this section.

* Merge registry table entries for `eventType` and `<content-descriptor>` (removing the `eventType` registry)
* Extract definition of Represents into the Shared Properties section
* Remove Script Event Type
* Make Represents an optional property of Script Event and move example that was in Script Event Type up into Script Event under the Represents bullet
* Add _permitted_ `#represents-div` feature
* Update class diagram to remove Script Event Type and add Represents to Script Event
Fix to be one or more, was zero or more.
Separate scriptRepresents from represents as applied to Script Events.

Define usage constraints.

Define the syntax of `<content-descriptor>` to allow for generic validity checking without needing to understand the contents of the values used.

Update the feature extensions and examples.

Update the introduction to mention represents.

Update the examples.

Update the profiles and dispositions.

Define an inheritance model for `daptm:represents`.

Make Represents required to have a non-empty and valid computed value, but don't require `daptm:represents` on every Script Event `<div>` element.

Allow user-defined content-descriptor values beginning `x-`.

State that ttm:desc SHOULD NOT be empty.
Move the term inside the angle brackets, which makes it harder to find in the index of terms, but fixes the Respec error.
* Fix broken reference to scriptRepresents
* Explain presence of inherited `daptm:represents` on `<body>` element in example
* Explain how scriptRepresents might include values not present in Script - Represents
* Fix syntax references to NMToken and `<lwsp>`
* Take suggestions for better term definitions for content-descriptor
* Clarify that Represents is inheritable, within the Represents property aside.
@nigelmegitt nigelmegitt force-pushed the issue-0227-change-script-event-type-to-represents branch from d37708f to 5a3704d Compare September 28, 2024 00:12
@nigelmegitt nigelmegitt merged commit f3fb76f into main Sep 28, 2024
2 checks passed
@nigelmegitt nigelmegitt deleted the issue-0227-change-script-event-type-to-represents branch September 28, 2024 00:13
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

Successfully merging this pull request may close these issues.

Interaction between "represents" and "script event type"
3 participants