Created attachment 327501 [details] [review]
Preliminary work

I was able to partially to do this in the attached patch.

Some notes:
* Notifications didn't work for me as long as some files (e.g. the .desktop files) didn't get properly namespaced (same bug as here: http://stackoverflow.com/questions/33723549/glib-notification-and-gnome-shell )
* I'm momentarily stuck at adding the action to open the specific email, as adding an ActionEntry doesn't seem to work (or is even the right choice, given bug 713991)

Hey Niels, thanks for taking a look at this as well.

Looking in /usr/share/applications, quite a few other GNOME apps are using the fully-qualified app id for their desktop files, so I guess this is the way to go. At some point we should probably rename the app id to org.gnome.Geary, but that's a lot of work for another bug. Anyway, this will be much easier to review if the work in renaming the desktop and app info files and updating use of their names names is separate standalone patch - would you mind splitting it out?

GNotifications are tied to app-scoped GActions, so you're right - we can't just simply add an ActionEntry since that produces a GtkAction instead. However we do also add GActions to GearyApplication - look for `exported_actions` in GearyController. It's a bit convoluted at the moment and this could get cleaned up as well, but that's also more work for a different bug. You'd need to add a `GearyOpen` action to the app that takes an (account,folder,email) tuple as its target, but first just get the action "working" by bringing the window to the front and worry about the target later - it might be a bit of work.

One other thing, I'd probably name the new class & file something like GLibNotification/glibnotification.vala to be make it obvious which library it's using and to be consistent with the other classes. You could also probably do a `git mv libnotify.vala glibnotification.vala` if you are basing the new impl off of that, rather than a copy and delete. It'll keep that file's history that way.

Created attachment 327523 [details] [review]
Patch 1 - the actual migration

Created attachment 327524 [details] [review]
Patch 2 - geary.* to org.gnome.geary.*

To be honest, I think this situation would be the ideal opportunity to change from
org.yorba.geary to org.gnome.geary. We're already changing the id, so gnome-shell
effectively already classifies it as another app. If we're doing this, we might
as well do it properly. One side-effect I noticed which I might mention: all the
translator files use "geary.desktop" to translate descriptions instead of
"org.yorba.geary.desktop". I don't really know whether it's our task to do simple
search & replace, or if it is the job of the translators.

Will look into the actions as soon as I can, thanks for writing out the
explanation!

Notifier was a name I made to replace LibNotify while I was a little uninspired,
so I changed it per your recommendation in Patch 1.

Small fix to my last comment: we're not changing the id per se, but the desktop
file, and gnome-shell indeed does see it as a different app in some cases.

(In reply to Niels De Graef from comment #5)
> To be honest, I think this situation would be the ideal opportunity to
> change from
> org.yorba.geary to org.gnome.geary. We're already changing the id, so
> gnome-shell
> effectively already classifies it as another app. If we're doing this, we
> might
> as well do it properly.

Yep, that was my thought initially as well, but there's a bunch of other things that would want to happen at the same time as well - updating the GSettings schema and migrating existing user settings, advertising the change in case someone else was using the existing app id via DBus, and probably other things. Also, call me sentimental but it's a bit of a hat-tip to Yorba to keep it for the moment.

> One side-effect I noticed which I might mention: all
> the
> translator files use "geary.desktop" to translate descriptions instead of
> "org.yorba.geary.desktop". I don't really know whether it's our task to do
> simple
> search & replace, or if it is the job of the translators.

The name of the files in `po/POTFILES.in` needs to be updated at least - you'll also want to add the new glibnotification.vala source file there and remove libnotify.vala if they have translatable strings. I believe that updating the actual PO files is something that the translators will handle. One of us should look into that though.

Although this may be a good argument for changing the app id now - so translators don't need to deal with the filename change twice. If we do change it, that (and patch #2 here) should get spun out as a separate bug that this one depends on, so we can track issues relating to the app id change separately.

> Will look into the actions as soon as I can, thanks for writing out the
> explanation!
> 
> Notifier was a name I made to replace LibNotify while I was a little
> uninspired,
> so I changed it per your recommendation in Patch 1.

No probs, I'm really happy with your work here!

Just filed that bug: #766196.

Created attachment 328412 [details] [review]
Patch - Activate application on notification.

Changes since the last patch:
* The notification now uses a simple action that simply activates the app when a notification is clicked. I looked into it, and it does seem that with the current way we work with Actions does not easily allow for targets (i.e. parameters) to be specified.
* POTFILES.in is updated as well.
* no more need for bindings to libnotify (i.e. bindings/vapi/libnotify.vapi)
* cleaned ubuntu and fedora dependencies of libnotify(-dev)

Can this also be marked as dependent on bug 766196 ?

Created attachment 328414 [details] [review]
Final patch

Whoops, seems I forgot to remove the dependencies for debian packages. Should be fixed now.

Hey, thanks for the updated patch. This looks pretty good.

Since the fix for Bug 759980 landed last week, we don't actually need Gtk.Action instances for app-level actions any more. Instead of constructing them as Gtk.Action instances, adding them to `entries`, then later going over `exported_actions` and adding them to the GearyApplication instance, it should be possible in ::create_actions to just create instances of SimpleAction instead and add them directly to GearyApplication.

You could do that just for the new GearyOpenMail action at the moment so as to be able to specify the target type, but it would be good to convert the existing app actions beforehand so they all work in the same way. Again, that would probably be worth doing in a separate patch or bug though, to keep the nature of the changes distinct.

I'm happy for this to depend on bug 766196 if you're willing to drive it. The most important things would be adding code to migrate the settings and passwords at startup, testing that out thoroughly, and checking out if things like user reviews in gnome-software and adding the app as a favourite in gnome-shell depends on the appid, so we know what needs to be mentioned as release notes. Otherwise, let's just do the file rename for the moment and worry about the appid rename later.

Created attachment 342489 [details]
screenshot notification unicode

I didn't tested this patch, but libnotify do not show emojis properly in notification, see attached screenshot. I did not report a new bug since I think this bug should solve it once merged though :).

Just wanted to know current status of this bug? There are plenty of features I can think of that directly depends on that:

- buttons in notification
- UTF16/emoji handling
- fix "Geary is ready" double notification issue (saw it reported but couldn't find it back) when clicking on a mail notification.
- properly handle notifications. Eg. when an email is opened/treated, remove the associated notification, if any.

If Niels hasn't any time to work on it, I may finish it; but if you can handle it I'd happily wait and work on something else in the meantime! :)

You did good to remind me :)

I rebased the patch locally and got it working again, so there's that. The only thing which still bothers me, is the fact that without a SimpleAction we might get broken persistent notifications.

Now, to switch to Actions, I need a serializable (i.e. string) version of the newly arrived messages, and possibly the same for their folders. After that, we still need to somehow start from that serialized form (i.e. a folder and email id) and open it in the conversation list, and select the right conversation, at the specific message.

So those two things still bother me and I'm hesitant to to push the patch without a solution for them.

I believe persistent notifications do not work currently neither, do they?

Obviously it would be better to have it, but if it could be done as a second part, I would be happy with the patch as-is :).

(In reply to Niels De Graef from comment #15)
> 
> Now, to switch to Actions, I need a serializable (i.e. string) version of
> the newly arrived messages, and possibly the same for their folders. After
> that, we still need to somehow start from that serialized form (i.e. a
> folder and email id) and open it in the conversation list, and select the
> right conversation, at the specific message.

And maybe also the account?

For the message, we obviously don't want to serialise the whole message, but could you use the message's DB id for this? The same thing may be doable for the folder, or you might even just be able to get its FolderPath, serialise that using get_fullpath.

On the controller side, it then would be a matter of getting the folder from the account and poking it to manually switch folders. Loading the message might be a bit more tricky, since you might need lookup its conversation to fit in with the existing ConversationListView/ConversationMonitor machinery.

FMI: Bug 775956 needed for persistent notifications since they work by DBus activation.

This is going to be the last time I plug https://wiki.gnome.org/Apps/Geary/Design/Notifications, I promise. Well maybe.

Per the design doc, the default notification action should be to simply display the most recent unread message for an account, but that also requires that the app keeps track of how may unread messages each account currently has.

So I'm wondering if we can get away without needing to do too much to serialise folders/accounts/messages: If the app keeps track of a list of most recent unread messages, and the default action is to simply "show most recent unread message for $ACCOUNT", then we only need to include the account id as the action param, and not the full account/folder/message id. When performing the action, the application instance can then simply look at it's most-recently-unread list for the account, and get the conversation/message ids from that.

For permanent notifications, if the app is launched in response to the user activating this default action, then we can at least just ensure the appropriate account is selected in the folder list, or at best wait until the most-recently-unread list is populated then flip to the one at the top of the list.

Thoughts?

(In reply to Michael Gratton from comment #19)
> This is going to be the last time I plug
> https://wiki.gnome.org/Apps/Geary/Design/Notifications, I promise. Well
> maybe.
> 
> Per the design doc, the default notification action should be to simply
> display the most recent unread message for an account, but that also
> requires that the app keeps track of how may unread messages each account
> currently has.
> 
> So I'm wondering if we can get away without needing to do too much to
> serialise folders/accounts/messages: If the app keeps track of a list of
> most recent unread messages, and the default action is to simply "show most
> recent unread message for $ACCOUNT", then we only need to include the
> account id as the action param, and not the full account/folder/message id.
> When performing the action, the application instance can then simply look at
> it's most-recently-unread list for the account, and get the
> conversation/message ids from that.
> 
> For permanent notifications, if the app is launched in response to the user
> activating this default action, then we can at least just ensure the
> appropriate account is selected in the folder list, or at best wait until
> the most-recently-unread list is populated then flip to the one at the top
> of the list.
> 
> Thoughts?

I would love to see some actions related to the message itself (if it's only one) to "Mark as Read", "Archive/Delete" and "Reply/Reply to All" in addition to activating the notification itself brings the message into focus. :)

The way Android doing things with Notification just got me spoiled (and I sometimes miss the old notification drawer that made actions available on older notifications too).

I'm no Vala developer though, so I can't decide if these things are hard to do for the next version or not, and I'm not in position to tell you what to do, and I can't contribute myself because of Vala, so there's that too.

Thank you all for your hard work, and keeping Geary alive.

Hi Anass,

(In reply to Anass Ahmed from comment #20)
> 
> I would love to see some actions related to the message itself (if it's only
> one) to "Mark as Read", "Archive/Delete" and "Reply/Reply to All" in
> addition to activating the notification itself brings the message into
> focus. :)

Thanks for the feedback. Can you say which Android app is letting you do this? Gmail, Inbox, K9, or something else?

Since we we will be using one single notification per account, it is likely that a notification will be for multiple new messages (e.g. "You have 5 new messages in Home"), but perhaps some the action could apply to the most recent message.

If you can let me know the app you are referring to above, I'd like to look into how it handles this situation.

(In reply to Michael Gratton from comment #21)
> Thanks for the feedback. Can you say which Android app is letting you do
> this? Gmail, Inbox, K9, or something else?

I use Gmail and Android default Email apps. Gmail is the best one (notifications appear separated per account, but can be expanded so you can handle each message separately). Each message has "Reply" and "Archive" action buttons, in addition to clicking the notification opens the message).

> Since we we will be using one single notification per account, it is likely
> that a notification will be for multiple new messages (e.g. "You have 5 new
> messages in Home"), but perhaps some the action could apply to the most
> recent message.

Sometimes, you only have one new message notification (I usually clear my inbox every morning) so I hope I can re-act to it in a fast meaningful way ("Open" -by clicking the notification-, "Archive", "Mark as Read", or "Reply").

(In reply to Anass Ahmed from comment #22)
> I use Gmail and Android default Email apps. Gmail is the best one
> (notifications appear separated per account, but can be expanded so you can
> handle each message separately). Each message has "Reply" and "Archive"
> action buttons, in addition to clicking the notification opens the message).

Ah, I see. Being able to expand a single notification for multiple messages would require some additional support from the XDG Notifications spec and the GNOME, KDE, etc notification daemons - as far as I know it's not possible to do that right now.

> Sometimes, you only have one new message notification (I usually clear my
> inbox every morning) so I hope I can re-act to it in a fast meaningful way
> ("Open" -by clicking the notification-, "Archive", "Mark as Read", or
> "Reply").

Okay, I'll have a chat to the design team and see what we can come up with.

Thanks for the feedback!

(In reply to Michael Gratton from comment #23)
> Ah, I see. Being able to expand a single notification for multiple messages
> would require some additional support from the XDG Notifications spec and
> the GNOME, KDE, etc notification daemons - as far as I know it's not
> possible to do that right now.

I'm curious if we can have a notification per message if they're less than 3 unread messages from _all_ accounts (GNOME Shell allows each app to have 3 consequent notifications AFAIK).

Bump tickets to 0.14 that aren't going to make 0.13.

Fix for this is landing on https://gitlab.gnome.org/GNOME/geary/merge_requests/205

Thanks for this one too, Niels! :)