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

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

Open
randomevents opened this issue Oct 27, 2017 · 11 comments
Open

Comments

@randomevents
Copy link

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
Copy link
Member

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

@randomevents
Copy link
Author

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
Copy link
Member

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

@randomevents
Copy link
Author

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

start with soundeffects

@LukePulverenti
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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

@randomevents
Copy link
Author

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
Copy link
Author

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

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

No branches or pull requests

2 participants