search

elementary / tasks

Synced tasks and reminders on elementary OS
https://elementary.io
GNU General Public License v3.0
84 stars 22 forks source link

Use floating timezones when saving timed tasks #313

Closed mcclurgm closed 2 years ago

mcclurgm commented 3 years ago

Fixes #312

This PR currently does the bare minimum to fix this issue. It uses a floating timezone when saving every task it gets. I copied the updated datetimes_to_icaltime function from elementary/calendar#653 to allow for floating timezones. I intend to iterate it with feedback.

A few action items for review before I would consider this complete:

  1. Overwriting all timezones may not be desirable. Do we want to overwrite every event with floating, or should we preserve existing timezones? (I believe we should preserve the data that's there, but haven't implemented it currently.)
  2. If we don't overwrite timezones, then we will probably need to do something to manage existing tasks that are erroneously in UTC, since Tasks won't allow users to fix that. I would appreciate guidance from @elementary/ux on this.

Still to do for myself:

If you know of any other places in Tasks where times are handled, let me know and I will investigate for required changes. I'm not super familiar with the Tasks codebase, so I'm not sure where to look beyond what I have changed here.

marbetschar commented 3 years ago

@mcclurgm does it work if we ask the user to remove the time, save the task and then re-add the time in case they run into this?

I doubt there are that many tasks affected that it s reasonable to add an automated solution :shrug: - but I could be wrong. What do others think?

mcclurgm commented 3 years ago

I'm not sure what the solution would be. Adding an automated solution would be a bunch of work for probably little benefit. Adding a warning like that could make sense. Also, since we now convert time zones to local, users will see that their events' times have changed. This could be a prompt for them to change them, but it also looks like the app is destroying their data (when really it's correcting a past bug).

I pushed an update that now converts to local times (code copied from Calendar). I changed all the places I noticed where we display times, but please let me know if there are any I missed.

marbetschar commented 3 years ago

@danrabbit what do you think is the best approach here? Doing an automatic conversion or doing nothing automatic at all and just somehow inform the user? (Either programmatically or just in a blog post for example)

mcclurgm commented 2 years ago

For a status update: the only things remaining to do here before I consider it ready for merge review

marbetschar commented 2 years ago

@mcclurgm can you please elaborate why we need to store the timezone if it already exists? Is this needed for newly added tasks as well - or just for tasks created before this fix?

mcclurgm commented 2 years ago

Sorry that wasn't very clear at all. This storage is internal, it's just a matter of getting the relevant information from the component to where it needs to go. If one exists we need to pass it to datetime_to_ical, which needs it because it constructs a new ICal.Time from scratch every time.

I suppose there's a really simple option for this that I just glossed over yesterday. Since the saving only happens in the save_task function, we could probably just poll the task property there and be done with it. That would be simplest. To prevent remaking objects from whole cloth every time we could also rearchitect the converter to accept an ICal.Time and modify it, which would inherently preserve all the relevant info from the component. I expect that's more work than it's worth, though.

One more warning that just occurred to me: once we start accepting and converting arbitrary timezones, we run into all sorts of conversion issues from libical to glib (see elementary/calendar#709). This is signing ourselves up for a fair bit of trouble, but since we don't control where people get their tasks from, I don't see how we can get around it.

mcclurgm commented 2 years ago

I just pushed an update the does the simple version. It's not quite the prettiest, but I don't think there's much of a way to simplify it, since there are a few branching cases that we need to handle.

danirabbit commented 2 years ago

Yeah I think we can just mention in a blog post to double check the timezone of your tasks. I don't think we need to spend a bunch of time on a programmatic solution here