Notifications

[marbetschar]

This PR fixes #635 with a background service which notifies about new mails received.

Stuff which still need to be addressed before this can be merged:

• The folder_changed signal is not emitted - this was because the folder returned from Camel.Store.get_inbox_folder is not a "real" folder. So we need to get the real one with Camel.Store.get_folder (inbox_folder.full_name...)
• Notifications do not show the correct icon (related to Revert "Fix #117: Show icons for daemons (#118)" notifications#122)

Execute the following to run and test the current code:

ninja -C build install
io.elementary.mail --background

[marbetschar]

Added the fix from #627 - but now getting a SegmentationFault with the following backtrace:

#0  0x00007ffff7ec75f4 in g_str_hash () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#1  0x00007ffff7ec5fbf in  () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#2  0x00007ffff73175d7 in  () at /lib/x86_64-linux-gnu/libcamel-1.2.so.62
#3  0x00007ffff7317870 in  () at /lib/x86_64-linux-gnu/libcamel-1.2.so.62
#4  0x00007ffff7318c88 in camel_sasl_authtype () at /lib/x86_64-linux-gnu/libcamel-1.2.so.62
#5  0x000055555555a008 in mail_camel_session_real_authenticate_sync
    (base=0x5555555b3a60, service=0x555555628520, mechanism=0x7fffd0019ab0 "PLAIN", cancellable=0x555555a24390, error=0x7fffe5cb5840) at ../daemon/CamelSession.vala:94
#6  0x00007ffff7322875 in camel_session_authenticate_sync () at /lib/x86_64-linux-gnu/libcamel-1.2.so.62
#7  0x00007fffec0ad48a in camel_imapx_server_connect_sync () at /usr/lib/evolution-data-server/camel-providers/libcamelimapx.so
#8  0x00007fffec08c7e8 in  () at /usr/lib/evolution-data-server/camel-providers/libcamelimapx.so
#9  0x00007fffec08cda3 in camel_imapx_conn_manager_connect_sync () at /usr/lib/evolution-data-server/camel-providers/libcamelimapx.so
#10 0x00007fffec0b6465 in  () at /usr/lib/evolution-data-server/camel-providers/libcamelimapx.so
#11 0x00007ffff731ce39 in  () at /lib/x86_64-linux-gnu/libcamel-1.2.so.62
#12 0x00007ffff7476c62 in  () at /lib/x86_64-linux-gnu/libgio-2.0.so.0
#13 0x00007ffff7f03374 in  () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#14 0x00007ffff7f02ad1 in  () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#15 0x00007ffff6ed6609 in start_thread (arg=<optimized out>) at pthread_create.c:477
#16 0x00007ffff7085293 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

UPDATE: Was able to fix the SegmentationFault by reverting parts of those changes. The daemon now does no longer crash and seems to work as expected - but the [FATAL 10:23:40.273967] [camel] MailSimpleSasl has an empty CamelServiceAuthType error is back again.

UPDATE: MailSimpleSasl has an empty CamelServiceAuthType has been fixed by #681

[marbetschar]

@davidmhewitt I tried as you suggested in the latest commit. But the notification center still refuses to show the correct app icon. What am I missing here?

[davidmhewitt]

I can't see where you set the application id of the Daemon but I am reading the code on mobile, so I might have missed it. That should be set to the desktop filename, minus .desktop

[marbetschar]

@davidmhewitt thanks for the hint! Adding the application id nearly fixed it. Strangely I also had to make sure to set Type=Application instead of Type=application. After I changed both things, the right icon appeared: a1b0d4a

That said, I guess this is ready for review 👍

[davidmhewitt]

Thinking about this a bit more however...

How well does this work when you consider Flatpak? I know we're not shipping this as a Flatpak right now, but, does the background portal stuff allow running a separate executable to the main executable, and do all the notifications work properly there?

It kind of feels like the Flatpak model would make this more complicated than just having the main executable send notifications, but I don't know enough to say for sure.

[davidmhewitt]

It sounds like at the very least that your desktop filename needs to be .daemon instead of -daemon because of flatpak/flatpak#750

[marbetschar]

@davidmhewitt I'm not sure how this is supposed to work as Flatpack tbh. But I've seen @Marukesu's pull request for Calendar which introduces support for Flatpack using the background portal: elementary/calendar#681

Therefore I assumed I can solve it the same way and simply start with the daemon approach because Mail is not Flatpacked atm. So we can ship now and extend/rework later.

[Marukesu]

I'm not sure how this is supposed to work as Flatpack tbh. But I've seen @Marukesu's pull request for Calendar which introduces support for Flatpack using the background portal: elementary/calendar#681

So there should be two .desktop files for the daemon, one in the XDG_DATA_DIRS/applications and another for the autostart.

For Flatpak, the background portal is responsible for generating the autostart one (what elementary/calendar#681 does), but we still need to export a second .desktop file for the notifications server, unless we go the one binary/application route.

[marbetschar]

@Marukesu thanks for the clarification. Now this makes a whole lot more sense. I adjusted this PR so it does generate two desktop files for the daemon as you described.

We can surely refactor once we start packaging Mail for Flatpack.

[marbetschar]

I just noticed I do have two entries for Mail in System Settings > Notifications. I guess this is due to the fact I'm currently deploying one *.desktop file for the Mail app itself and another one for the Daemon:

... so I guess we'd really want to embed the daemon within the Mail app itself, so we end up with one binary as suggested by @Marukesu. From what I understand this would remove the second entry in the notification settings, but still guarantee we are displaying the correct icon in Notification Center.

I'm not 100% sure on how to implement this, but will look into how its done in AppCenter.

[marbetschar]

UPDATE: I was able to fix the double-entry issue in System Settings > Notification with the one-binary approach described above.

This also means we can simply start the background task now with io.elementary.mail --background, same way as we do in AppCenter.

[danirabbit]

I can confirm this works, but I imagine we don't want to shoot off like 20 notifications at a time 😅

Could we count up the messages and senders and if we have more than one new message do something like "10 new messages from 3 senders"?

[marbetschar]

@danrabbit made the notification a bit more intelligent and therefore less intrusive.

[danirabbit]

Awesome that's much better! I just have one question about multiple accounts

I'm still getting 2 notifications on startup. I wonder if that's because it's per mail account? That's a good thing imo, but maybe we can call that out more clearly in the notification?

I'm not sure exactly how best to split up the information onto the two lines we have. Maybe @cassidyjames has thoughts? But something like

5 new messages
from 3 senders for [email protected]

not sure exactly how to make that work for the individual emails. Maybe it's less necessary because you have the full context of the sender and subject?

[marbetschar]

I'm still getting 2 notifications on startup. I wonder if that's because it's per mail account? That's a good thing imo, but maybe we can call that out more clearly in the notification?

Exactly: The event is emitted once for each inbox folder - so its run once per mail account. I agree, its probably a good idea to also add the display name of this account to the notification. Most probably this would be a good thing to add in the title. I played a bit with it and ended up with the following, what do you think?

[danirabbit]

Awesome, this is a pretty big feature and it seems to work as expected, so let's get it merged and make sure it's well tested for a release at the end of the month!

Nice work :D