GNOME Bugzilla – Bug 774442
Use in-app notifications
Last modified: 2018-06-06 22:45:13 UTC
Geary should use in-app notifications to provide feedback when actions have completed, to give an opportunity to undo them if possible, and to notify if an error has occurred: https://developer.gnome.org/hig/stable/in-app-notifications.html.en For the moment, we probably want to use in-app notifications for the following actions: - Conversation copied/moved (including Archive, Trash, etc.) with Undo button - Conversation deleted - Email sent - Trash/Junk emptied - Draft closed and saved or discarded (unless a dialog was presented) The text of the notifications should be depend on context, e.g.: - “[Bug 741224] new funct…” labelled as Todo - 3 conversations moved to Trash - Message to Jane’s Sprokets sent If an action resulted in an error, a context-dependent, human-readable message should be presented: - Could not label “[Bug 741224] new funct…”, not currently online - Could not send message to Jane’s Sprokets, a login error occurred - Could not connect to Work, remote server is not available And so on.
Created attachment 352686 [details] [review] [Patch] Implemented in-app notifications for successfully sent mail. Wrote a patch that implements an in-app notification for successfully sent mail. I introduced a new class `InAppNotification` to ease it for all the other use cases. One question I had though, was how much errors we should show. If I connect Geary.Account's "report-problem" signal to an in-app notification each, won't that be an overload? And if so, what signals should we connect to?
Review of attachment 352686 [details] [review]: Just pushed a version of this patch to wip/774442-in-app-notifications, rebased off master and with a few build fixes and po/POTFILES.in updated. I'm pretty happy with the general approach, there's maybe a few things to tweak though: - Slighty longer default keepalive? I'm finding it a bit difficult to notice it and then read it at the moment. A longer interval would give a chance of that happening, and the close button can be used to dismiss it if it gets in the way. I'd go with whatever timeout Nautilus uses, but I don't have have that number handy. - Maybe if there's only two recipients it's worth spelling put both their names, since "and 1 other" is probably longer than a number of names. - There needs to be a Gtk.Frame between the Gtk.Revealer and the rest of the content, so that users can use F6 to get to it quickly via the keyboard. - Finally, it might be worth reverting the changes unrelated to the the Gtk.Overlay addition to main-window.ui. Defs good to go in with this stuff addressed.
Thinking about what to do about errors, connecting to report-problem is probably the right approach, since that's what the signal exists for. Some care is probably needed to do handle those errors in a useful way, however. In the case where Geary will automatically attempt to resolve the problem, then using InAppNotification is definitely appropriate, e.g.: > Failed to connect to mail.example.com, retrying in 10s [Cancel][X] > Failed to send message to Some Person, retrying in 5s [Cancel][X] The notification might then auto-dismiss after the time elapses if it is short enough, otherwise be dismissed after the default keepalive passes. For more permanent/fatal errors, then we might actually want to use a MainWindow-wide Gtk.InfoBar, since we probably want to make sure users definitely know an error occurred, and provide a means for them to recover with a contextual action, e.g.: > Failed to connect to mail.example.com [Check Configuration][X] > Failed to send message to Some Person [Retry Now][X] And perhaps with some means of obtaining related logging about it: > Failed to send message to Some Person [Details][Retry Now][X] Where clicking the Details button will bring up some window with additional technical details (e.g. "Failed to resolve DNS name mail.example.com") and/or debug logging, and perhaps even a means to report the bug. This is all a lot of work though, and probably best done as the next step in Bug 713006. For this bug, I'd be happy with InAppNotification and having it hooked up for sending/moving/copying/etc and basic support for notifying errors using report-problem.
Review of attachment 352686 [details] [review]: Sorry for a drive-by review, but I have two i18n-related comments/issues. ::: src/client/application/geary-controller.vala @@ +2721,2 @@ private void on_sent(Geary.RFC822.Message rfc822) { + string message = _("Successfully sent mail to %s.".printf(rfc822.to.to_short_string())); It looks similar to bug #788387 to my layman eyes… ::: src/engine/rfc822/rfc822-mailbox-addresses.vala @@ +144,3 @@ + + return _("%s and %d others").printf(first_recipient, this.size - 1); + } These two strings need to be one, using ngettext: https://wiki.gnome.org/TranslationProject/DevGuidelines/Plurals
I've been doing some thinking about the design of notifications in general on the wiki: https://wiki.gnome.org/Apps/Geary/Design/Notifications - feedback welcome. Currently under that doc, error are indeed handled by Gtk.InfoBars, and I've been doing some work to that end over in Bug 713006. So the outstanding thing to think about here is how notify about command completion. I'm open to suggestions here, but my best suggestion is to introduce a high-level notion of an end-user Command, and use that to keep track of when the engine finally finishes executing it and hence being able to notify the user. To that end I've filed Adding a dependency for Bug 790308, since that will provide the infrastructure needed to implement command completion notifications. In the mean time, I'm inclined to polish the existing patch to address the issues above and commit it, but then keep this open and use it to cover command completion notifications once Bug 790308 (or whatever else is needed for this) lands.
I was wanting in-app notifications for some other work so I've just rebased this patch off master, addressed Piotr's and my immediate comments and pushed this as commit 7a54a1ba. It would be still good to get some more pervasive use of this per other comments above, but that should probably be tackled as a more holistic overhaul of notifications in general, so I'll leave that to another bug. Thanks Niels!