AsteroidOS / asteroid-settings

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

Timezone setting #63

Closed beroset closed 1 year ago

beroset commented 1 year ago

This addresses https://github.com/AsteroidOS/asteroid-launcher/issues/35 by adding the ability to set the timezone from the settings page. This is useful for travelers and required to be able to successfully import calendar appointments.

dodoradio commented 1 year ago

do you think there's a way to integrate this into the current time page, or to make it a subpage of that? maybe it could be a horizontal scroll on the current time page (since vertical swipes will trigger the numberic tumblers). I don't think it matters too much, but I think it's more elegant when there's only one time-related item on the main settings menu

beroset commented 1 year ago

The code looks good to me overall but I'd like to have a debate around the strategy and benefits of adding time zone support first.

There are multiple reasons to support time zone. Among them are:

  1. People who travel can have the correct local time
  2. Calendar import (which makes the Agenda app much more useful) requires correct timezone
  3. At least one person had a need for it for a 2FA app for AsteroidOS
  4. It would allow for multi-timezone watchface(s)
  5. Correct application of daylight savings time rules depend on timezone being set correctly

And if we agree that there is added value (I'm sure there is, I just fail to see it atm) then, should we also have a page like this in asteroid-launcher's first boot setup?

Probably so.

What happens when the user changes their time zone, does it also change the current wall clock time ? Have you tried if it breaks alarms/timers ?

With no changes, today if you change the time zone as with timedatectl, the time (and possibly date) instantly changes accordingly on the watch.

And if we need a new time sync protocol to be time zone aware, what does the migration path look like? As in, how do we keep some amount of backward compatibility in order not to break all existing sync clients (I'm thinking we should support both protocols for at least some time)

I think we could keep the existing protocol for quite some time and have the app assume it's "local time" for whatever timezone is currently set on the watch. In parallel, we could implement @dodoradio 's suggestion to use the standard Bluetooth Time Change Service which already handles timezones. I've started looking into this.

Depending on the cost/benefit ratio I'm thinking we may want to postpone this after 2.0 because if "time zone awareness" is a long series of tasks that will only push back 2.0 and can introduce bugs just before the release, we should merge this in a branch for now and only integrate it by default after 2.0 is cuts so it gets proper testing before being released.

From my own usage, today I always set the timezone via ssh on my watch because otherwise the Agenda app is useless to me (because I can't sync my real calendar without timezone support). This appears to have no adverse effects on any other watch function. I also travel often, so if I create a meeting on the other side of the planet and then fly there, I don't want my 2PM appointment alarm going off at 2AM instead!

Basically, I'd like to see the bigger picture of what exactly you're trying to achieve here to make sure we shepherd the changes correctly.

I hope the above explains a bit more of what I see as the necessity of supporting timezones. Discussion is welcome!

FlorentRevest commented 1 year ago

The code looks good to me overall but I'd like to have a debate around the strategy and benefits of adding time zone support first.

There are multiple reasons to support time zone. Among them are:

  1. People who travel can have the correct local time
  2. Calendar import (which makes the Agenda app much more useful) requires correct timezone
  3. At least one person had a need for it for a 2FA app for AsteroidOS
  4. It would allow for multi-timezone watchface(s)
  5. Correct application of daylight savings time rules depend on timezone being set correctly

And if we agree that there is added value (I'm sure there is, I just fail to see it atm) then, should we also have a page like this in asteroid-launcher's first boot setup?

Probably so.

What happens when the user changes their time zone, does it also change the current wall clock time ? Have you tried if it breaks alarms/timers ?

With no changes, today if you change the time zone as with timedatectl, the time (and possibly date) instantly changes accordingly on the watch.

And if we need a new time sync protocol to be time zone aware, what does the migration path look like? As in, how do we keep some amount of backward compatibility in order not to break all existing sync clients (I'm thinking we should support both protocols for at least some time)

I think we could keep the existing protocol for quite some time and have the app assume it's "local time" for whatever timezone is currently set on the watch. In parallel, we could implement @dodoradio 's suggestion to use the standard Bluetooth Time Change Service which already handles timezones. I've started looking into this.

Depending on the cost/benefit ratio I'm thinking we may want to postpone this after 2.0 because if "time zone awareness" is a long series of tasks that will only push back 2.0 and can introduce bugs just before the release, we should merge this in a branch for now and only integrate it by default after 2.0 is cuts so it gets proper testing before being released.

From my own usage, today I always set the timezone via ssh on my watch because otherwise the Agenda app is useless to me (because I can't sync my real calendar without timezone support). This appears to have no adverse effects on any other watch function. I also travel often, so if I create a meeting on the other side of the planet and then fly there, I don't want my 2PM appointment alarm going off at 2AM instead!

Basically, I'd like to see the bigger picture of what exactly you're trying to achieve here to make sure we shepherd the changes correctly.

I hope the above explains a bit more of what I see as the necessity of supporting timezones. Discussion is welcome!

Fwiw, it was discussed on Matrix and I was ok with all of the above and agreed with the conclusions so I think we should move forward with this ;)

I see the PR is still marked as draft, your UI idea isn't implemented and some comments are not addressed. Do you need more time to work on this ?

If we want to take this in for 2.0, these "Time Zone" titles are the last strings we should add before asking all our translators to complete their translations.

beroset commented 1 year ago

I see the PR is still marked as draft, your UI idea isn't implemented and some comments are not addressed. Do you need more time to work on this ?

Yes. It turns out that this will require the installation and configuration of polkit (see https://unix.stackexchange.com/questions/741695/how-do-i-allow-a-non-root-user-to-set-the-timezone-via-d-bus/741732#741732 ) in addition to the GUI changes I mentioned.

FlorentRevest commented 1 year ago

I see the PR is still marked as draft, your UI idea isn't implemented and some comments are not addressed. Do you need more time to work on this ?

Yes. It turns out that this will require the installation and configuration of polkit (see https://unix.stackexchange.com/questions/741695/how-do-i-allow-a-non-root-user-to-set-the-timezone-via-d-bus/741732#741732 ) in addition to the GUI changes I mentioned.

It would be interesting to check how much dependencies polkit pulls in. If I remember correctly I tried to keep it out because it had a crazy dependency on mozjs that would decouple build time and no overheat my laptop until it shuts down :p

beroset commented 1 year ago

This is only useful if the polkit patch is merged. After struggling for some days in trying to convert this to a hierarchical set of lists, I gave up and decided that having one long list is good enough for now and adds a significant benefit. It can be improved later if there's sufficient interest.

FlorentRevest commented 1 year ago

Thank you for this @beroset ! :)

As you pointed out, the UI still has room for improvement but that's already a step in the right direction and hopefully this unblocks new use-cases like what you've been doing around calendar sync so that's already quite neat. Keep it up!