search

AsteroidOS / asteroid-settings

Default settings app for AsteroidOS
GNU General Public License v3.0
9 stars 18 forks source link

Nightstand: Add support for using the regular watchface #61

Closed MagneFire closed 1 year ago

MagneFire commented 1 year ago

This follows up on the work done by @beroset in https://github.com/AsteroidOS/asteroid-settings/pull/59.

Essentially it adds support for using the same watchface for both nightstand mode and regular mode.

It implements a watchface tracking mode to ensure that the nightstand watchface matches the regular watchface whenever a different regular watchface is selected.

FlorentRevest commented 1 year ago

I very much like the goal of this PR but I think the logic should have been implemented differently. Let me try to explain the issue.

If the user sets a "day" watchface to A and a custom "night" watchface to B. And then, the user tries a few different "day" watchfaces (cycles through them in settings just to see what they are all like). If at any point the user picks up the B watchface as a day watchface, then the "custom watchface" setting will switch off by itself and the night watchfaces will start to automatically follow the day watchface. This results in a confusing UX since the user never unchecked the "custom watchface" switch, I would call it a bug.

If the "custom watchface" settings was remembered as an independent boolean settings, the launcher could just have the watchface be (isDay || !useCustomNightWatchface) ? dayWatchface : nightWatchface. In settings, the custom nighstand watchface switch could just toggle useCustomNightWatchface (feel free to name it differently) and this would result in a code that is a lot simpler (for example we don't need to leak nighstand implementation details into the day watchface selector page) and also fixes the bug described in the previous paragraph (if the user turns on "custom watchface", it stays on).

MagneFire commented 1 year ago

Excellent point! I'll adjust the logic to include said boolean. Initially I had the code setup this way as well, but for some reason I removed it. Thanks for this swift thinking!

MagneFire commented 1 year ago

Your suggestion is now implemented, thanks for the feedback! It now also relies on https://github.com/AsteroidOS/asteroid-launcher/pull/120.

beroset commented 1 year ago

Here's another scenario that doesn't seem to work quite right.

Initial settings - nightstand is ON, custom nightstand watchface is OFF.

  1. user selects a watchface; both daytime and nightstand watchface change
  2. user enables custom nightstand watchface and selects a different watchface
  3. user disables custom nightstand watchface but previously selected nightstand watchface remains

In that last step, shouldn't it revert to using the daytime watchface?

Also at this point, if the user selects a different daytime watchface, the previously selected nightstand watchface remains unchanged.

beroset commented 1 year ago

Sorry, that's my error. I didn't notice that it also needed the commit from asteroid-launcher. When I add that, it works just fine and as expected.