Skip to content

bandwidth based quality selection #59

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
mash-graz wants to merge 2 commits into
base: master
Choose a base branch
from
Open

bandwidth based quality selection #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b173815
fix missing bracket
Mar 17, 2020
95db6f6
bandwidth based quality selection
Mar 17, 2020
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
2 changes: 1 addition & 1 deletion src/ui/State.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ export class StateStore
this.props.player.tracks.sort(
(a, b) => Number(b.height) - Number(a.height),
),
'height',
'bandwidth',
Copy link
Copy Markdown
Owner

@matvp91 matvp91 Mar 26, 2020

Choose a reason for hiding this comment

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

I believe we can drop the uniqBy filter above, as this would select "bandwidth" to group by.

);

let activeTrack = null;
Expand Down
8 changes: 6 additions & 2 deletions src/ui/components/Settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ tabs[SettingsTabs.OPTIONS] = (props: SettingsProps) => (
item: SettingsTabs.TRACKS,
label: props.data.getTranslation('Quality'),
info: `${
props.data.activeTrack ? props.data.activeTrack.width : ''
props.data.activeTrack ?
Copy link
Copy Markdown
Owner

@matvp91 matvp91 Mar 26, 2020

Choose a reason for hiding this comment

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

Nested condition shorthands are a bit confusing, could this be better structured?

Copy link
Copy Markdown
Author

@mash-graz mash-graz Mar 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

as i already mentioned in a earlier reply, i'm not very familiar with TS or JS. if it would be python or rust code, i would definitely choose the most pleasant and idiomatic expression, but on the java script side i'm unfortunately hardly able to keep my head above water. ;)

if you don't mind, i would prefer, that you simply modify the suggested changes to the better.

i know, it's a little bit of annoying work, but at the end it's definitely better to keep the code base clean and in exemplary style, which i'm simply not capable to serve in this particular programming language without spending huge amounts of time to study and learn the most basic expressive possibilities.

(props.data.activeTrack.width ?
(""+props.data.activeTrack.width+"x"+props.data.activeTrack.height+", "):"")
+(props.data.activeTrack.bandwidth/1e6).toPrecision(2)+"M" : ''
Copy link
Copy Markdown
Owner

@matvp91 matvp91 Mar 26, 2020

Choose a reason for hiding this comment

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

Not a big fan of this line, ES6 template literals would be a cleaner approach imo.

}`,
},
props.data.visibleSettingsTabs.includes(SettingsTabs.SUBTITLES) && {
Expand Down Expand Up @@ -55,7 +58,8 @@ tabs[SettingsTabs.TRACKS] = (props: SettingsProps) => (
items={[
...props.data.tracks.map(track => ({
item: track,
label: `${track.width}`,
label: `${(track.width?(""+track.width+"x"+track.height+", "):"")
+(track.bandwidth/1e6).toPrecision(2)+"M"}`,
})),
{
item: 'auto',
Expand Down
1 change: 1 addition & 0 deletions src/ui/i18n.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ export const translations = {
'Automatic quality': 'Automática',
'Enable subtitles': 'Habilitar legenda',
'Disable subtitles': 'Desabilitar legenda',
}
};

export const getTranslation = languageCode => text => {
Expand Down