PlummersSoftwareLLC / NightDriverStrip

NightDriver client for ESP32
https://plummerssoftwarellc.github.io/NightDriverStrip/
GNU General Public License v3.0
1.33k stars 213 forks source link

UI: System Wide Settings #299

Closed davepl closed 1 year ago

davepl commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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:

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 commented 1 year ago

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.