MediaBrowser / emby-webcomponents

24 stars 29 forks source link

Settings aren't being saved to displaypreferences.db correctly #5

Open randomevents opened 7 years ago

randomevents commented 7 years ago

This one is fun, so I just happened to create my admin users on a new server a month ago, when I added my family to the main server or created my test server for debugging, the sound effects menu item wasn't being respected for anyone but my initial users.

It seems that while some settings (see below) are being writing to the app settings, they're not being written to the db. Two examples are sound effects and screensaver. What I get from a quick glance at the code is that a null from the database overrides the local app settings?

I was able to work around it by adding the keys manually to the database -: table userdisplayprefences, col data, object CustomPrefs ,"soundeffects":"none","screensaver":"backdropscreensaver"

How to reproduce -: Make a new user and try to change sound effects to none.

LukePulverenti commented 7 years ago

This is by design. Settings are only saved to the server if we want to share them across apps.

randomevents commented 7 years ago

You're missing the second part, it's not respecting the local appsettings as well. I think see the problem, in UserSettings.prototype.get (line 50 usersettingsbuilder), it's not being passed a variable for enableOnServer from line 42, so returns an undefined.

That being said, I think you should rethink the mass false to enableOnServer I'm seeing. If users go through the trouble to set up a profile on TV A. They want it to follow them around the house. Even more so when you set a different theme to signify a different family member. Or the compromise would be to add a new item to the bottom that asks if you want to save this settings to your default and make that a app settings that's called at the beginning of saveuser? Actually that would be the compromise? If I PRed that would you Pull it?

LukePulverenti commented 7 years ago

Well, no, definitely don't want those kinds of prompts being added. What setting in particular is not working?

randomevents commented 7 years ago

What's wrong with adding a check box to the bottom of display preferences?

start with soundeffects

LukePulverenti commented 7 years ago

Everything that can be saved onto the server is being done that way. Due to the modular nature, some settings depend on what is installed into the app, so that is why sound effects are not. Granted, we don't have a plugin catalog set up yet, but that is still why it's done that way. I think after we get plugins going then we can revisit this.

randomevents commented 7 years ago

Dammit, missed it.

The problem is probably in soundeffectsmanager line 13 soundeffectOption = userSettings.get("soundeffects") should be soundeffectOption = userSettings.get("soundeffects",!1)

the logic in get (enableOnServer !== !1 && this.displayPrefs) would make the undefined enableOnServer pass as true.

LukePulverenti commented 7 years ago

Is the setting not working, or is it working but not saved on the server? it is by design that it is not currently saved on the server.

randomevents commented 7 years ago

It's not working, the above logic has it always checking the server instead of appsettings

userId ? enableOnServer !== !1 && this.displayPrefs ? this.displayPrefs.CustomPrefs[name] : appsettings.get(name, userId) : null

LukePulverenti commented 7 years ago

In my testing it seems to be working fine. I'm able to toggle the setting back and forth.

randomevents commented 7 years ago

Are you going out to the UI and navigating, it toggles fine, the problem was always that it doesn't respect the setting? Try it with brand new user.

randomevents commented 7 years ago

I see it in Theater PC and Theater web, new users and new users on a fresh server.