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

Merge video and audio options #535

Merged
merged 12 commits into from
Apr 7, 2022
Merged

Conversation

debmint
Copy link
Contributor

@debmint debmint commented Mar 17, 2022

Changes

Merge media sources and audio channels under one button - Labeled "Options" - on the Movies Details page. The original commit had separate buttons for each and called up separate menus. This commit was built upon that and combined the buttons and merged the menus. There is also an indicator - sort of a superscript - beside the Audio and Video displays indicating when there are additional choices available

Issues

There are no real problems that this fixes, just gives an alternate approach to dealing with the choices.

Display count of additional audio or video sources if anyn are present.
This is shown to right of currently-selected source
@debmint debmint marked this pull request as draft March 17, 2022 22:28
@debmint
Copy link
Contributor Author

debmint commented Mar 17, 2022

This is an attempt to follow a suggestion by @neilsb as I understand him. I'm putting this up for review to see if this would be a good alternative

@neilsb
Copy link
Member

neilsb commented Mar 19, 2022

I like how that works. Makes sense to me, but would be good to hear what other people think.

Aesthetically, could maybe do with a little space between the end of codec and the new label, and not sure the bold font is needed.

@debmint
Copy link
Contributor Author

debmint commented Mar 19, 2022

I like how that works. Makes sense to me, but would be good to hear what other people think.

Yes, it would be good to get some input due to the many choices. I sort of like the idea presented in my other PR, but this seems to be a good option.

Aesthetically, could maybe do with a little space between the end of codec and the new label, and not sure the bold font is needed.

No problem adding space, but do you really think it's needed? Often a superscript is sort of crowded near what it's annotating? Re: the bold font. on my set, at least, it seemed a bit faint without it. Did you try removing the bold spec to compare? I would have liked to have the superscript text a bit smaller, but from my experimenting, you cannot change the size of system fonts - is that correct?

I'll play around with it some more following your suggestions and see what I can come up with.

@neilsb
Copy link
Member

neilsb commented Mar 19, 2022

Probably right about the space. Fine just to leave it.

You can set the font size in the for the system fonts I'm pretty sure. From memory you can just set the size property in brs file (maybe in xml too)

I'll check out the other PR tomorrow and give it a proper try. 😄

@debmint
Copy link
Contributor Author

debmint commented Mar 19, 2022

Probably right about the space. Fine just to leave it.

I'll experiment with adding space

You can set the font size in the for the system fonts I'm pretty sure. From memory you can just set the size property in brs file (maybe in xml too)

I thought I tried setting size and it didn't seem to affect it, but will re-check. Do you think it would look better if it were a bit smaller?

I'll check out the other PR tomorrow and give it a proper try. smile

I set it to not display if only only one media source. This could be done with audio as well. That way, the buttons would not be there to be a distraction, so to speak. I may have discovered how to handle the buttons, with ButtonGroup, now. Haven't tried, but I think you could add the buttons by string, the call getChildren() and cycle through them and set id baed on the text. I was not using getChildren() correctly when I tried it before

@debmint debmint marked this pull request as ready for review April 2, 2022 14:56
Copy link
Member

@neilsb neilsb left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@neilsb neilsb merged commit 6a0b790 into jellyfin:master Apr 7, 2022
@jimdogx
Copy link
Contributor

jimdogx commented Apr 7, 2022

Late to the party, but wanted to say @debmint that I love the combined video/audio in to one "Options". Way cleaner than what I did 👍

@debmint
Copy link
Contributor Author

debmint commented Apr 7, 2022

Late to the party, but wanted to say @debmint that I love the combined video/audio in to one "Options". Way cleaner than what I did +1

Thanks for the comment. I had thought jellyfin-roku needed this ability to play all available versions, but I don't know if I'd have been able to implement it without your providing the initial starting point. I just built upon what you had done.

@debmint debmint deleted the mergeVideoAudioOpts branch July 24, 2022 03:00
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.

4 participants