jellyfin / jellyfin-meta

A repository to hold our roadmap, policies, and more.
25 stars 4 forks source link

Chromecast Selector #7

Closed Artiume closed 3 months ago

Artiume commented 3 years ago

Currently the Chromecast selector is selectable via User Settings > Playback. This has benefits and disadvantages.

Advantages: It is easy to do testing on a client without impacting the entire server. Disadvantages: On every client, you have to remember which version you have selected. Any user, even inexperienced users may select unstable which may break any and all playback. You can't prevent users from using unstable if you dont wish them to.

I am making a PR to add a personal Chromecast client

https://github.com/Artiume/jellyfin-web/pull/19

This will be addable via Dashboard > Playback > Streaming

I am tempted to move the chromecast selector to this page as well. It will make it so the admin has full control of which version of chromecast is used. The main disadvantage is that if you decide to test a non-stable client, this will impact the entire userbase.

thornbill commented 3 years ago

I have always thought it was odd that this was a per-user option. I think a server level setting would be better in most cases.

heyhippari commented 3 years ago

It makes sense for it to be a per-user option in regards to stable or unstable, imo.

The good way of doing this would be:

Artiume commented 3 years ago

I'm not sure how to hide the selector. As it will be, when I add custom, it will become a third option which will break casting if it is selected and the admin has not added a custom appid

I'm with Bill in that users don't need to be mucking about with the chromecast version, the admin should have control of it.

nielsvanvelzen commented 3 years ago

I agree that users don't need to set this, there isn't even any explanation of what the unstable option is, so a lot of users don't understand the option anyway.

Implementation wise I think we should simplify the server part to a single field that contains the cast id, this field would then be exposed somewhere thats easily to read. In the dashboard we can then add a input to enter the id and add two buttons to set it to the jellyfin ones (stable/unstable).

That way it's way easier to use a self hosted cast client while keeping it simple in the back end to store.

Artiume commented 3 years ago

This is my PR currently

image

I'd like to make it

image

nielsvanvelzen commented 3 years ago

I'd say keep the "custom chromecast appid" and add two buttons/links "set to Jellyfin Stable" and "set the Jellyfin Unstable". Why would we store 2 values in the server when it can be simplified to just one.

Artiume commented 3 years ago

I'd like to not erase the currently provided Stable and Unstable appid's unless I'm confusing what you mean

heyhippari commented 3 years ago

I'm not sure how to hide the selector

Get the server config when accessing the page, if the disable is set, set the hide class on the selector. That's pretty much it.

The idea for having the unstable one defined on the client is this:
How do you test the unstable client without setting it for the entire server?

It is, as the name implies, unstable. So someone wanting to check a fix on their server wouldn't likely want their entire user base to be stuck with an unstable client for a while, while they perform their tests.
We can't ask for people to force their users to the unstable branch or even set up a second server just for testing something, so having it be toggle-able on the client is what makes the most sense, imo.

Artiume commented 3 years ago

Only the dev's really test unstable. And if you're going to test chromecast for more than a minute to see if something works, ideally, you should use a test instance for that and not one that is shared. I don't think it's worth it, having a button that can break functionality available to end users who don't know squat is asking for headaches.

nielsvanvelzen commented 3 years ago

image

Maybe this illustrates my idea better, I don't think we should add 2 values to the server (one for the custom id and one for an enum) but instead only keep the id in the server. This would make it way easier for clients to implement casting because they don't need to check which id to use.

heyhippari commented 3 years ago

Only the dev's really test unstable

You're making a terrible assumption here.

Take the following scenario: A user is trying to play something on Chromecast, it's not working on stable. We think we have fixed it on the unstable one, but we don't have their files so we can't test it. The only reliable way to confirm that it's fixed is to ask said user to test it themselves and report back.

We also have a bunch of non-developers using the unstable client, to help figure out bugs and find regressions. This is an important step of development.

heyhippari commented 3 years ago

having a button that can break functionality available to end users who don't know squat is asking for headaches

That's why you set the "Allow users to use the unstable client" or whatever toggle to false by default. That way, the "advanced" setting is hidden and is controlled by the admin.

Artiume commented 3 years ago

https://github.com/Artiume/jellyfin-web/pull/19/files#diff-d55ed754bb9143001c8693e6290e98654fb8b6dec73bccd176efce3a62d2d17dR110-R114

This is how clients check the appid

heyhippari commented 3 years ago

This is how I'd do the admin part: image

The client part would check for this last checkbox's setting, then hide or show the current selector (Stable/Unstable) depending on the value of said checkbox on the server.

Artiume commented 3 years ago

I think a simple If statement here would work

https://github.com/Artiume/jellyfin-web/blob/27de291a35fb3f49690b901b71fbe970179466ed/src/components/playbackSettings/playbackSettings.js#L206