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

UI: System Wide Settings #299

Closed
davepl opened this issue Jun 1, 2023 · 3 comments
Closed

UI: System Wide Settings #299

davepl opened this issue Jun 1, 2023 · 3 comments

Comments

@davepl
Copy link
Contributor

davepl commented Jun 1, 2023

We need a single page in the app for setting the global user settings, such as:

ZIP Code and City
Time Zone
Weather API Key
Stocks API Key (if needed)
YouTube subscriber count API key

Those last 3 could be per-effect settings, but that would make them a lot less discoverable, so I argue for promoting them up to global in the UI so people can find them.

@zephyrr
Copy link

zephyrr commented Jun 2, 2023

If effects can expose new settings to the API (per #294), could they not also add entries to the global user settings page, as needed (via a similar but different mechanism)? Adding a new effect would add the global settings it needs, omitting the effect would omit the settings.

If there can be more than one instance of an effect, this could potentially mean a corresponding number of copies of the global setting on the page. And that implies some name or index number for each of the individual instances of the effect, so the user can distinguish them.
Which in turn suggests that the settings on the global user settings page might be divided into sections, each with an effect type, and if multiple separately configured instances of that effect are allowed, a name or number for which instance is asking.

Which could be back to "per effect settings", but still via (sections within) the global user settings page and thus easily discoverable.

@rbergen
Copy link
Collaborator

rbergen commented Jun 2, 2023

I think it's important to make a distinction between where the effect settings/properties/... live, and how they are presented to the user for inspection and modification. I'm aware it may be that I am preaching to the choir and this is plainly obvious to everybody, but just in case:

  • In the NightDriverStrip object structure on the device (what lives "behind" the API) effect settings live within the effect classes that declare/need them, and IMHO they have to live there. Published effect settings start with very basic, generic stuff like the effect name and its maximum show duration, but can include others that are very specific to an effect and core to their operation; externalizing those violates key principles of OOP to begin with. Furthermore, as already stated, the default effect sets already contain multiple instances of individual effects - differentiated by changes in properties/settings. With the ability added in Expose LEDStripEffect settings, copy effects and remove copies #293 to copy effects at runtime this has now become "fluid by design". To make the YouTube channel GUID a case in point: if one would want to copy the Subscriber effect and make it show the number for another channel, the copy would have to use a different channel GUID than the original.
    Trying to centralize the storage and management of these settings within the object space will lead to a massive duplication of properties (read: memory use) and dependencies/relationships between the central settings manager (now DeviceConfig) and the effects that are hideously complex and nearly impossible to manage.
    And again, there's no need. Effects already publish their supported settings in a known place: via the /settings/effect/specs endpoint. You have to make a request to get the specs (that is, it's a "pull" of specifications, not a "push"), but that's just because it's the "API way".

  • Exposing those settings to the user in an accessible way ("in front of" the API) is a UI/UX challenge, and not one I intend to downplay. I'm not sure if one "global settings page to rule them all" is in fact the right solution, or if this is better addressed with a per-effect cogwheel-with-pop-up, or something else altogether. Whatever solution is chosen, it must support multiple effect instances requiring different values for the exact same set of properties, and it must support these property sets to change (by copying and/or removing effects) at runtime.

Just for completeness: the Weather API key being a device-level setting might seem to go against my first point, as it is currently clearly intended for use by "the" Weather effect. There are two reasons why I think this is not the case:

  1. The Open Weather API could be used by other effects that require access to weather data (the API key is a key to identify the user consuming the API, not an individual application or, indeed, effect)
  2. The API key is a "protected" setting, which is why it can be set (and in fact validated) but not retrieved via the API. With effect properties being copied when effects are, effect properties being serialized by other classes (i.e. EffectManager), etc. protecting decentralized settings is much harder, not to say nearly impossible, to do.

@rbergen
Copy link
Collaborator

rbergen commented Oct 28, 2023

I'm closing this as the Web UI now includes a screen for system-wide settings and a solution for effect-specific settings. Many thanks to @KeiranHines for implementing this.

@rbergen rbergen closed this as completed Oct 28, 2023
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

3 participants