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

Allow Jellyfin and Emby to coexist on the same domain #361

Merged

Conversation

oddstr13
Copy link
Member

Jellyfin and Emby both overwrite the servercredentials3 local storage object when saved auth doesn't match. Renaming this key allows both to stay logged in on the same domain.

See jellyfin-archive/jellyfin-apiclient-javascript#14 for corresponding apiclient pr

Copy link
Contributor

@DrPandemic DrPandemic left a comment

Choose a reason for hiding this comment

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

This works on FF. Though, you are not targeting the right branch. I think you should be targeting master. This file seems to have been renamed to credentialprovider.js by vitorsemeano in April.

@thornbill
Copy link
Member

I added a comment in the corresponding apiclient pull request jellyfin-archive/jellyfin-apiclient-javascript#14 (review)

@oddstr13
Copy link
Member Author

Targeting of release branch was intentional, as I consider this a bug fix and not a feature.
(I would also prefer getting it in sooner rather than later - next build including master is likely db rewrite, which is likely a while before it is ready)

There is one side effect from this patch - users will get logged out on update.
This could be mitigated by including code to copy the old key to the new - but I'm not currently familiar enough with the code base to do that, I just did a search and replace.

As for the name of the variable, this doesn't really matter to me, but I would prefer jellyfin_credentials over JellyfinCredentials (CamelCase screams "this is a class" at me, and that just feels wrong)

@thornbill
Copy link
Member

jellyfin_credentials sounds good to me. 👍

Copy link
Contributor

@DrPandemic DrPandemic left a comment

Choose a reason for hiding this comment

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

If we are good with doing a new release, this LGTM

@oddstr13
Copy link
Member Author

jellyfin_credentials rather than jf_servercredentials?

@joshuaboniface @DrPandemic

@DrPandemic
Copy link
Contributor

I think jellyfin_credentials reads better since we don't really ever use the jf acronym, but both are good.

@joshuaboniface joshuaboniface merged commit 6a0672a into jellyfin:release-10.3.z Jun 16, 2019
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