Notifications

[marbetschar]

This PR is add notifications for tasks which are due:

[marbetschar]

@mcclurgm thank you very much for your help!

AFAIK the only known issue left now: For an unknown reason notifications are fired twice: Once by the background process and once when the tasks app is started (if the task in question was due within the last 12 hours). Do you have any clue why?

[mcclurgm]

I haven't made it to that yet, but I did notice a few things that are getting in the way.

Now each time I click the close button I get a warning:

(io.elementary.tasks:20557): GLib-GObject-WARNING **: 07:53:59.574: ../../../gobject/gsignal.c:2736: instance '0x555d39a311a0' has no handler with id '1200'

The id number varies.

Also, I'm getting an assertion failure. When I try to save with a time set, one of the assertions in my icaltime_get_timezone function fails and crashes the app. Specifically, the ICal.Time has an empty TZID, but its timezone is not null. I didn't account for that in the original code. I'm not sure how that can happen, do you have any suggestions? Or could you point me to where we generate timezones for tasks so I can see what's being set?

[mcclurgm]

Sorry for the delay reviewing this, been quite busy lately.

Overall, seems to work. I get notifications for all my tasks that I set reminders for. It also no longer crashes the same way that I experienced before, after opening multiple new windows. Have found a couple issues though.

I found a new regression, but it's only a glib warning. I'm seeing a runtime warning when closing the main window. This doesn't happen 100% of the time, but pretty reliably if I wait a second or two between opening and closing the window. Since it doesn't seem to cause any real issues I'm not sure it's a big deal, but it may be worth investigating.

The TZID bug I found is still an issue, just mitigated by your commit so it no longer crashes. I plan to investigate and see if I can submit a PR for that, but no time estimate on when I'll be able to, sorry. It seems like the timezone is being freed when we leave the datetimes_to_icaltime function, but looking at the C code I don't see any code that would, so I'm confused.

Also, there's something a little odd about the handling of times. First, the displayed time doesn't correspond to the actual time the notification goes off. It's 10:00 here, and if I set the reminder for 10:00, then it says that's in the past. I have to go all the way to 14:00 for it to not be in the past. This means that I'm getting notifications at times that don't seem like they should be sent according to what's in the UI. This may be unrelated to the notifications code, so if so it's probably worth fixing in a separate PR and not here. I assume this is something to do with timezones, which I see you assign by default.

I haven't looked into the multiple notifications issue. I hope to soon, but thought all this was worth bringing up in the meantime.

[marbetschar]

@mcclurgm I guess the time issue with the notification is related to https://github.com/elementary/tasks/issues/312?

[mcclurgm]

I hadn't made that connection--I checked and that's the case. So that issue is out of scope for this PR, although potentially a blocker for making it usable in a release once it's merged. Since it affects so many things I think it's best to address it separately.

(edited, I originally guessed at the connection but now verified it by checking my tasks.)

[marbetschar]

This is now ready for review :partying_face:

• Fixed Merge Conflicts
• Thanks to the fix of @mcclurgm notifications are now fired at the right time
• I also added a fix which prevents firing the same notification again when Tasks is started in the foreground

Sidenote: The aforementioned fix does not prevent the same notification to be fired in all cases, because I only store sent notifications in memory. Therefore we'll see the same notification for due tasks within the last 12 hours whenever io.elementary.tasks --background is restarted (e.g. for a new login session or after reboot). But I think that's not a blocker for now and needs some more real world testing if this is really an issue.