Skip to content

Improve flexibility of quality selector #1081

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.

Sign up for GitHub

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

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Open

Improve flexibility of quality selector #1081

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
14d0ce3
Tweaked quality selector to be more accomodating
gurupras Jun 30, 2018
6bd9462
Added some minor changes to code relating to i18n
gurupras Jul 1, 2018
fda8424
Updated to use inbuilt type checkers
gurupras Jul 1, 2018
6e41d5e
Incorported early-return feedback
gurupras Jul 1, 2018
faaeb5d
Linting fixes
gurupras Jul 1, 2018
28add9b
Fix plyr.quality setter
gurupras Jul 1, 2018
c3103f4
Restored old ${value}p logic for displaying qualities
gurupras Jul 1, 2018
99613ca
Removed index as a required property
gurupras Jul 1, 2018
41fe611
Fix default quality
gurupras Jul 1, 2018
2e20972
Updated plyr.quality setter to continue to be backwards compatible
gurupras Jul 1, 2018
3643a03
Fixed dedupe
gurupras Jul 1, 2018
b081ca0
Quality attribute label is no longer mandatory
gurupras Jul 1, 2018
d50c65a
Fixed getBadge logic
gurupras Jul 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 54 additions & 25 deletions src/js/controls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ const controls = {
},

// Create a settings menu item
createMenuItem({ value, list, type, title, badge = null, checked = false }) {
createMenuItem({ value, list, type, title, badge = null, checked = false, index = null }) {
Copy link
Contributor

@friday friday Jul 1, 2018

Choose a reason for hiding this comment

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

Optimally you shouldn't need to add index here. You can use value for index (like captions do). This of course means you need to create the arguments for createMenuItem before any sorting/dedupe/filtering.

const item = createElement('li');

const label = createElement('label', {
Expand All @@ -388,6 +388,7 @@ const controls = {
value,
checked,
class: 'plyr__sr-only',
index,
}),
);

Expand Down Expand Up @@ -671,6 +672,17 @@ const controls = {
},

// Set the quality menu
// options is expected to be an array of objects
// Each entry in this array should be of the type:
// {
// label: String, // mandatory
// height: Number, // mandatory
// index: Number, // optional
Copy link
Contributor

@friday friday Jul 1, 2018

Choose a reason for hiding this comment

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

Does index need to be a property. Can't we use the array index?

// badge: String, // optional
// }
// The order of qualities will be based on height. If there are multiple
// entries with the same height, then we use index instead.
// If badge is not specified, it will be looked up by height.
setQualityMenu(options) {
// Menu required
if (!is.element(this.elements.settings.panes.quality)) {
Expand All @@ -680,9 +692,9 @@ const controls = {
const type = 'quality';
const list = this.elements.settings.panes.quality.querySelector('ul');

// Set options if passed and filter based on uniqueness and config
// Set options if passed
if (is.array(options)) {
this.options.quality = dedupe(options).filter(quality => this.config.quality.options.includes(quality));
this.options.quality = dedupe(options);
Copy link
Contributor

@friday friday Jul 1, 2018

Choose a reason for hiding this comment

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

dedupe doesn't work on objects. It doesn't do "deep equals".

Copy link
Contributor Author

@gurupras gurupras Jul 1, 2018

Choose a reason for hiding this comment

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

Shall I replace this with just

if (is.array(options) {
  this.options.quality = options
}

Copy link
Contributor

@friday friday Jul 1, 2018

Choose a reason for hiding this comment

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

Yes! :)

Copy link
Contributor

@friday friday Jul 1, 2018

Choose a reason for hiding this comment

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

(Sam might disagree but at least it won't look like it's doing something and then it doesn't work)

}

// Toggle the pane and tab
Expand All @@ -702,10 +714,18 @@ const controls = {

// Get the badge HTML for HD, 4K etc
const getBadge = quality => {
const label = i18n.get(`qualityBadge.${quality}`, this.config);
let label;
if (quality.badge) {
label = quality.badge;
} else {
// The badge is based on the height of the video
// TODO: We need some rounding logic here
label = i18n.get(`qualityBadge.${quality.label}`, this.config);

if (!label.length) {
return null;
if (!label.length) {
// TODO: Try to find a badge based on height
return null;
}
}
Copy link
Contributor

@friday friday Jul 1, 2018

Choose a reason for hiding this comment

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

  1. This can be done a lot simpler with destructuring and default parameters.
  2. Shouldn't it be quality.height. Not quality.label?

Suggestion (not tested):

const getBadge = quality => {
	const {
		height,
		badge = i18n.get(`qualityBadge.${height}`, this.config);
	} = quality;

	return badge ? controls.createBadge.call(this, label) : null;
};

Copy link
Contributor Author

@gurupras gurupras Jul 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Although badge should be based on height in theory, piggy-backing on the existing i18n requires the height to be 480. A value of 482 (if it were the case) would generate no badge despite being the real height of the video. I think I will update this to implement the closest solution Sam was mentioning.

Copy link
Contributor

@friday friday Jul 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure what comment by Sam you're referring to, but you don't have to fix that. In fact, since it's a separate problem it's better not to imo. You can do that in a separate pr in that case.

Copy link
Contributor Author

@gurupras gurupras Jul 1, 2018

Choose a reason for hiding this comment

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

It's referenced here

Copy link
Contributor

@friday friday Jul 1, 2018

Choose a reason for hiding this comment

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

Alright. I did read that, but too quickly I guess. I still think this should be a separate PR optimally. Solving it with closest also means 719p would count as HD, which it by definition isn't. I would expect it work like breakpoints, if anything. Sorry I'm making this hard for you btw!


return controls.createBadge.call(this, label);
Expand All @@ -714,41 +734,40 @@ const controls = {
// Sort options by the config and then render options
this.options.quality
.sort((a, b) => {
const sorting = this.config.quality.options;
return sorting.indexOf(a) > sorting.indexOf(b) ? 1 : -1;
if (a.height === b.height) {
return a.index > b.index ? -1 : 1;
}
return a.height > b.height ? -1 : 1;
})
Copy link
Contributor

@friday friday Jul 1, 2018

Choose a reason for hiding this comment

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

I think label should be used for sorting. After all that's what's shown. In case there are multiple levels with the same height (like with that Sintel hls manifest) and the dev provides a label "818p (4149k)" for one and "818 (6214k)" for the other. It shouldn't break backward compatibility since with the previous options the sort result would be the same.

Copy link
Contributor Author

@gurupras gurupras Jul 1, 2018

Choose a reason for hiding this comment

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

I don't think that's the order you want.

["576", "1080", "720"].sort()
["1080", "576", "720"]

The previous implementation was actually doing it by height. At least on plyr.io.

Copy link
Contributor

@friday friday Jul 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The previous implementation had only one argument (quality/height), so it was the only thing that could be used for sorting. Using the label for sorting wouldn't have added anything since it's either the same with an extra "p", or translated. Either way it always meant "quality" and it it couldn't really mean anything else. With these changes however it's no longer clear this is the case.

However you're probably right that fixing the sort method could be too much work, at least for now.

The best solution might be not to sort at all, but that would be breaking, so yeah. Leave it as is 👍

.forEach(quality => {
controls.createMenuItem.call(this, {
value: quality,
value: quality.label,
list,
Copy link
Contributor

@friday friday Jul 1, 2018

Choose a reason for hiding this comment

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

The index is a better identifier for value (instead of adding the additional index property).

type,
title: controls.getLabel.call(this, 'quality', quality),
title: controls.getLabel.call(this, 'quality', quality.height, toTitleCase(quality.label)),
badge: getBadge(quality),
Copy link
Contributor

@friday friday Jul 1, 2018

Choose a reason for hiding this comment

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

I think we should respect the label and its formatting as it was specified by the developers, and only call getLabel if they didn't pass a label.

This includes i18n and changing the casing. In most languages I know of title case is incorrect formatting, and it make no sense to give developers the option to translate their own input. They should specify it in the language they want it.

index: quality.index,
});
});

controls.updateSetting.call(this, type, list);
},

// Translate a value into a nice label
getLabel(setting, value) {
getLabel(setting, value, defaultLabel) {
let label;
Copy link
Contributor

@friday friday Jul 1, 2018

Choose a reason for hiding this comment

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

I think you should restore getLabel (see above). Not sure why you removed ${value}p?

Copy link
Contributor Author

@gurupras gurupras Jul 1, 2018

Choose a reason for hiding this comment

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

Is 272p or 408p legitimate? My understanding from your post on Slack was that the 'p' represents widescreen resolutions. Formally it's progressive scanning rather than interlaced.

Copy link
Contributor

@friday friday Jul 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

No that was K As in 2K or 4K video etc (used for badges). 408p basically means its height is 408px (although the "p" does mean something else entirely).

Copy link
Contributor Author

@gurupras gurupras Jul 1, 2018

Choose a reason for hiding this comment

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

Okay, replacing with the old logic for now.

switch (setting) {
case 'speed':
return value === 1 ? i18n.get('normal', this.config) : `${value}×`;

case 'quality':
if (is.number(value)) {
const label = i18n.get(`qualityLabel.${value}`, this.config);

if (!label.length) {
return `${value}p`;
}
label = i18n.get(`qualityLabel.${value}`, this.config);

return label;
// If we don't find a valid label, we return passed in defaultLabel
if (!label.length) {
return defaultLabel || toTitleCase(value);
}

return toTitleCase(value);

return label;
case 'captions':
return captions.getLabel.call(this);

Expand All @@ -773,16 +792,26 @@ const controls = {
value = this.config[setting].default;
}

let settingOptions = this.options[setting];
if (setting === 'quality') {
// Quality is not an array of strings. It's an array of Objects
// Extract out the strings
settingOptions = this.options[setting].map(level => controls.getLabel.call(this, 'quality', level.label));
}

// Unsupported value
if (!is.empty(this.options[setting]) && !this.options[setting].includes(value)) {
if (!is.empty(settingOptions) && !settingOptions.includes(value)) {
this.debug.warn(`Unsupported value of '${value}' for ${setting}`);
return;
}

// Disabled value
if (!this.config[setting].options.includes(value)) {
this.debug.warn(`Disabled value of '${value}' for ${setting}`);
return;
// Don't check for quality which is permissive and accepts anything
if (setting !== 'quality') {
if (!this.config[setting].options.includes(value)) {
this.debug.warn(`Disabled value of '${value}' for ${setting}`);
return;
}
}
}

Expand Down
18 changes: 14 additions & 4 deletions src/js/html5.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,15 @@ const html5 = {
// Get quality levels
getQualityOptions() {
// Get sizes from <source> elements
return html5.getSources
const sizes = html5.getSources
.call(this)
.map(source => Number(source.getAttribute('size')))
.filter(Boolean);
return sizes.map((size, index) => ({
label: `${size}`,
height: size,
index,
}));
},
Copy link
Contributor

@friday friday Jul 1, 2018

Choose a reason for hiding this comment

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

There are multiple problems here.

  1. Not really a problem per se, but as Sam pointed out there's no need for the intermediate variable sizes. You can just add to the existing function chain.
  2. The label will be 720 not 720p as I'm sure you intended it to be (since otherwise the template literals doesn't really make sense).
  3. However I don't think you should set the label at all since later on in the code you need to be able to distinguish between data that was specified by developers and fallback data in order to respect the data specified by developers. I would use a class for the entries in this.options.qualities in order to solve this via getters and setters (moving the logic from getLabel and getBadge)
  4. It wouldn't be possible to set the badge and label via the source element.
  5. filter(Boolean) removes all falsy values. I think with the other changes in this PR this isn't something we want. Especially not before grabbing the array index, like now. Rather, we should do that when creating the menu.

I suggest you change it to something like this:

// I haven't tested this code, and the comments are meant for you, not to stay in the code
return html5.getSources.call(this)
    .map((source, index) => {
        // Get data from the dataset (https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/dataset)
        const {
            // For backward-compatibility, fallback on the old size-attribute (which I think we should deprecate)
            height = source.getAttribute('size'),
            label,
            badge,
        } = source.dataset;

        return {
            index, // Really don't think we need index here at all, as mentioned
            badge,
            label,
            height,
        };
    });

Copy link
Contributor Author

@gurupras gurupras Jul 1, 2018

Choose a reason for hiding this comment

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

The template literal is to ensure that label conforms to the laid down rule (by me) that label is a String.

Updated with your suggestion. This is neat. I like being able to specify label, badge via data- attributes.


extend() {
Expand All @@ -35,21 +40,26 @@ const html5 = {
const player = this;

// Quality
const qualityLevels = html5.getQualityOptions.call(player);
Object.defineProperty(player.media, 'quality', {
get() {
// Get sources
const sources = html5.getSources.call(player);
const source = sources.find(source => source.getAttribute('src') === player.source);
if (!source) {
return undefined;
}
const sourceIndex = sources.indexOf(source);

// Return size, if match is found
return source && Number(source.getAttribute('size'));
// Return label, if match is found
return qualityLevels[sourceIndex].label;
},
Copy link
Contributor

@friday friday Jul 1, 2018

Choose a reason for hiding this comment

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

This should be height, not label

set(input) {
// Get sources
const sources = html5.getSources.call(player);

// Get first match for requested size
const source = sources.find(source => Number(source.getAttribute('size')) === input);
const source = sources.find(source => source.getAttribute('size') === input.label);

Copy link
Contributor

@friday friday Jul 1, 2018

Choose a reason for hiding this comment

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

This should be height, not label

// No matching source found
if (!source) {
Expand Down
7 changes: 5 additions & 2 deletions src/js/listeners.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ class Listeners {
// Quality change
on.call(this.player, this.player.media, 'qualitychange', event => {
// Update UI
controls.updateSetting.call(this.player, 'quality', null, event.detail.quality);
controls.updateSetting.call(this.player, 'quality', null, event.detail.quality.label);
});
Copy link
Contributor

@friday friday Jul 1, 2018

Choose a reason for hiding this comment

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

This should be height, not label


// Proxy events to container
Expand Down Expand Up @@ -511,7 +511,10 @@ class Listeners {
proxy(
event,
() => {
this.player.quality = event.target.value;
this.player.quality = {
label: event.target.value,
index: Number(event.target.attributes.index.value),
};
showHomeTab();
},
'quality',
Expand Down
16 changes: 14 additions & 2 deletions src/js/plugins/youtube.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,13 @@ function mapQualityUnits(levels) {
return levels;
}

return dedupe(levels.map(level => mapQualityUnit(level)));
const mappedLevels = dedupe(levels.map(level => mapQualityUnit(level)));
return mappedLevels.map((level, index) => ({
label: levels[index],
index,
height: level,
badge: level >= 720 ? 'HD' : 'SD',
Copy link
Contributor

@friday friday Jul 1, 2018

Choose a reason for hiding this comment

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

If you set the badge here the i18n won't work, right? With the fallback you don't need to anyway right?

Copy link
Contributor Author

@gurupras gurupras Jul 1, 2018

Choose a reason for hiding this comment

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

Removing badge in favor of default logic

}));
}

// Set playback state and trigger change (only on actual change)
Expand Down Expand Up @@ -300,7 +306,13 @@ const youtube = {
return mapQualityUnit(instance.getPlaybackQuality());
},
set(input) {
instance.setPlaybackQuality(mapQualityUnit(input));
let label;
if (is.string(input)) {
label = input;
} else if (is.object(input)) {
({label} = input);
}
instance.setPlaybackQuality(mapQualityUnit(label));
},
});

Expand Down
42 changes: 30 additions & 12 deletions src/js/plyr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import source from './source';
import Storage from './storage';
import support from './support';
import ui from './ui';
import { closest } from './utils/arrays';
import { createElement, hasClass, removeElement, replaceElement, toggleClass, wrap } from './utils/elements';
import { off, on, once, triggerEvent, unbindListeners } from './utils/events';
import is from './utils/is';
Expand Down Expand Up @@ -677,21 +676,40 @@ class Plyr {
const config = this.config.quality;
Copy link
Contributor

@friday friday Jul 1, 2018

Choose a reason for hiding this comment

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

The old quality setter code was already too complex and hard to understand, so I think this is a step in the wrong direction. I think you should restore the old setter and the getter in the html5/youtube plugins and add a new getter/setter that sets only by index. videoTrack or level perhaps?

Copy link
Contributor Author

@gurupras gurupras Jul 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Part of your comment, I think, is referring to the fact that this setter is trying to account for number/string/object all-in-one.

The number version was added just now to be backwards compatible. At least with plyr.io. It wasn't meant to handle index.

The string version is to cater to users wanting to set it by label.

The object version is mainly to handle the internal implementation wherein:
user clicks quality option --> translate event.target.value into quality somehow. This was where the object came into play.

Now, with the suggestion to remove index attribute and use it as the value while creating radio button, we can update the quality setter to receive, for example, either ["0", 0, { index: 0 }]. I opted for the object implementation to allow sufficient differentiation.

const options = this.options.quality;

let quality;
// Create a local copy of input so we can handle modifications
let qualityInput = input;
if (!options.length) {
return;
}

let quality = [
!is.empty(input) && Number(input),
this.storage.get('quality'),
config.selected,
config.default,
].find(is.number);

if (!options.includes(quality)) {
const value = closest(options, quality);
this.debug.warn(`Unsupported quality option: ${quality}, using ${value} instead`);
quality = value;
// Support setting quality as a Number
if (is.number(qualityInput)) {
// Convert this to a string since quality labels are expected to be strings
qualityInput = `${qualityInput}`;
}

// Now, convert qualityInput into a quality object.
// This object is emitted as part of the qualityrequested event
if (is.string(qualityInput)) {
// We have only a label
// Convert this into an Object of the expected type
quality = {
label: qualityInput,
};
} else if (is.object(qualityInput)) {
quality = qualityInput;
} else {
this.debug.warn(`Quality option of unknown type: ${input} (${typeof input}). Ignoring`);
return;
}

// Get the corresponding quality listing from options
const entry = options.find((level) => level.label === quality.label && (quality.index ? level.index === quality.index : true));

if (!entry) {
this.debug.warn(`Unsupported quality option: ${input}. Ignoring`);
return;
}

// Trigger request event
Expand Down