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

Introduce a generic skip action as a replacement to skipad. #350

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

youennf
Copy link
Contributor

@youennf youennf commented Dec 16, 2024

Introduce a media kind to allow marking content as either advertisement, generic...
Fixes #340.


Preview | Diff

@youennf
Copy link
Contributor Author

youennf commented Dec 16, 2024

Bikeshed now requires Python 3.9; you are on 3.8.10.
If you're seeing this message in your CI run, you are
likely specifying an old OS; try ubuntu-latest.
If you're seeing this on the command line, see the docs
for instructions:
https://speced.github.io/bikeshed/#installing
Command bikeshed update failed with exit code: 1.

@tidoust, do you know what happened there?

tidoust added a commit that referenced this pull request Dec 16, 2024
Needed to get Python >=3.9 for Bikeshed. Also used this opportunity to bump
the version of the checkout action used.

Via #350 (comment)
tidoust added a commit that referenced this pull request Dec 16, 2024
Needed to get Python >=3.9 for Bikeshed. Also used this opportunity to bump
the version of the checkout action used.

Via #350 (comment)
@tidoust
Copy link
Member

tidoust commented Dec 16, 2024

@tidoust, do you know what happened there?

Job was using an old version of Ubuntu that ships with Python 3.8. Now using ubuntu-latest, see #351.

sequence<MediaImage> artwork = [];
sequence<ChapterInformationInit> chapterInfo = [];
};

enum MediaKind {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a section describing what these mean? "advertisement" is obvious to me, but the rest are not so obvious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the enum and added some description.
Let's discuss this enum.

Copy link
Member

Choose a reason for hiding this comment

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

Does the implementation make use of these values, e.g., in a default action handler? I'm wondering if an enum is needed and we could let the web app set any value it wants, using DOMString.

Also, a web app might want to skip between chapters, so do we need similar on ChapterInformation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kind could influence UA UI (whether displaying a skip button, for how long, skip button label customisation to Skip intro for instance...). An arbitrary DOMString might add complexity and would disallow feature detection.

do we need similar on ChapterInformation?

Right, this PR adds kind to ChapterInformation as well.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, thanks.

Copy link
Member

Choose a reason for hiding this comment

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

This list may be a bit too specific, for example BBC might want to offer a skip to next news item, skip to next song, or whatever.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated
typically happening at the beginning of a TV series episode and summarizing past episodes.
</li>
<li>
the empty string: the default value of media content, it represents any other value of a part of media content.
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like empty string will be used to mean both "actual content" and "unknown content". Should there be an explicit value for actual content?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to other for now.

@steimelchrome
Copy link
Contributor

This generally lgtm just added a bikeshed comment on the MediaKind enum

"opening-credits",
"post-credits-scene",
"summary",
""
Copy link
Member

@marcoscaceres marcoscaceres Jan 14, 2025

Choose a reason for hiding this comment

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

Maybe let's just call this "content" ? or "other".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to other, note that the default value of the dictionary kind value is also updated.

@youennf
Copy link
Contributor Author

youennf commented Jan 14, 2025

Discussed today in media WG meeting.
PR seems good with the following changes:

  • Rename "" to "other" inMediaKind enumeration
  • Add a note stating that MediaKind enumeration is experimental and can be extended.

I may also add wording about potential default handlers, (seek to next chapter if skipping a chapter or to the end if there is no chapter).

@youennf
Copy link
Contributor Author

youennf commented Jan 14, 2025

@chrisn, would the note and the renaming of "" to "other" cover your concerns? Or do you have a better idea in how to handle the case of "next song", "next news"...?

index.bs Outdated
@@ -1116,6 +1165,7 @@ dictionary MediaMetadataInit {
<li>Its <a for=MediaMetadata>title</a> is the empty string.</li>
<li>Its <a for=MediaMetadata>artist</a> is the empty string.</li>
<li>Its <a for=MediaMetadata>album</a> is the empty string.</li>
<li>Its <a for=MediaMetadata>kind</a> is the empty string.</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this say the kind is "other"?

[SameObject] readonly attribute FrozenArray&lt;MediaImage> artwork;
};

dictionary ChapterInformationInit {
DOMString title = "";
double startTime = 0;
MediaKind kind = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here about whether it should be "other"

@chrisn
Copy link
Member

chrisn commented Jan 21, 2025

@chrisn, would the note and the renaming of "" to "other" cover your concerns?

@steimelchrome asked about "actual content" vs "unknown content" so I'm not sure I see how renaming "" to "other" addresses that. For us, there may not be a need to, and "" could be enough as a general purpose skip. But (and separately to the WG) we'd be interested to see what kind of UI treatment browsers might be planning for these skip actions.

youennf and others added 5 commits January 23, 2025 17:10
Introduce a media kind to allow marking content as either advertisement, generic and so on.
Co-authored-by: Chris Needham <[email protected]>
Co-authored-by: Chris Needham <[email protected]>
Co-authored-by: Marcos Cáceres <[email protected]>
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.

Make "skipad" action more generic
5 participants