GNOME Bugzilla – Bug 685928
add a panel for configuring notifications
Last modified: 2012-12-20 21:56:31 UTC
See https://live.gnome.org/Design/SystemSettings/Notifications The design calls for a way to configure per-application, whether notifications are shown, with details or not, and whether they are shown on the lock screen. I haven't thought through the details of how to best store this information in settings, but it seems likely that a list of application ids is going to be involved. The notification spec has a desktop-entry hint, which is well suited for this. It is defined as the desktop file basename. See http://people.gnome.org/~mccann/docs/notification-spec/notification-spec-latest.html. Also see bug 685926
I now roughly implemented the mockups at https://live.gnome.org/Design/SystemSettings/Notifications I'm attaching here the patch to get early design validation. On the control center side, besides minor polishing, it is feature complete. On the other hand, the policy bits implemented in the shell (in bug 685926) are a subset of those exposed.
Created attachment 227931 [details] [review] Add Notifications panel For the 3.7 notifications filtering feature, we introduce a new panel that will show applications using the message tray, and we allow configure in what way the shell presents them.
Created attachment 227932 [details] How it looks like now
Created attachment 227933 [details] The editing dialog
Hey Giovanni, you are fast ! I think for the list, we probably want to go for egglistbox - that would allow us to get closer to the design. See what Cosimo has been doing in bug 687490
While I see the advantage of packing a real GtkSwitch there, I'd rather try a CcCellRendererSwitch, as using GtkTreeModel is quite useful. Not to mention that I would lose Glade editing by going for egglistbox.
Created attachment 227997 [details] [review] Add Notifications panel For the 3.7 notifications filtering feature, we introduce a new panel that will show applications using the message tray, and we allow configure in what way the shell presents them. Updated patch.
There's unrelated whitespace changes in the network panel in the patch.
Review of attachment 227997 [details] [review]: How do applications declare that they can/will display notifications? ::: panels/notifications/cc-edit-dialog.c @@ +36,3 @@ + gboolean bold; +} policy_settings[] = { + { "enable", N_("Notifications"), TRUE }, Will need context here. ::: panels/notifications/cc-notifications-panel.c @@ +48,3 @@ + +static void build_app_store (CcNotificationsPanel *panel); +static void on_app_removed (CcNotificationsPanel *panel, const gchar *canonical_app_id); use char, not gchar. @@ +153,3 @@ +} + +/* ---------------------------------------------------------------------------------------------------- */ Remove this please. @@ +178,3 @@ +} + +/* ---------------------------------------------------------------------------------------------------- */ Remove. @@ +199,3 @@ + l1 = g_strv_length (panel->app_ids); + + gchar **new_app_ids = g_settings_get_strv (panel->master_settings, C-99. @@ +221,3 @@ + panel->app_ids = g_settings_get_strv (panel->master_settings, + "application-children"); + g_qsort_with_data (panel->app_ids, g_strv_length (panel->app_ids), The sorting should be based on last use (so that the last application to show a popup will be at the top of the list, as the usual use of that settings panel is "shut up application"). The shell would return a list of identifiers, in MRU order, which we'd use for sorting. @@ +230,3 @@ +} + +/* ---------------------------------------------------------------------------------------------------- */ Remove @@ +270,3 @@ + const char *canonical_app_id) +{ + gchar *path = g_strconcat ("/org/gnome/shell/notifications/application/", canonical_app_id, "/", NULL); No assignment in declaration. @@ +272,3 @@ + gchar *path = g_strconcat ("/org/gnome/shell/notifications/application/", canonical_app_id, "/", NULL); + + GSettings *settings = g_settings_new_with_path ("org.gnome.shell.notifications.application", path); Ditto. And I would really rather the schema lived in gsettings-desktop-schemas, otherwise we make gnome-control-center depend on gnome-shell (and vice-versa for some functionality like network dialogues). ::: panels/notifications/gnome-notifications-panel.desktop.in.in @@ +8,3 @@ +StartupNotify=true +Categories=GNOME;GTK;Settings;DesktopSettings;X-GNOME-Settings-Panel;X-GNOME-PersonalSettings; +OnlyShowIn=GNOME;Unity; You can remove Unity here. ::: panels/network/panel-cell-renderer-text.c @@ -28,3 @@ #include <gtk/gtk.h> -#include "panel-cell-renderer-text.h" Do the move/renaming in a separate patch, and change the users of it in a separate patch too.
(In reply to comment #9) > Review of attachment 227997 [details] [review]: > > How do applications declare that they can/will display notifications? They don't. gnome-shell manages the list of "known" applications the first time they send a notification, and apps are never removed from the list. Ideally we'd use GSettingsList, but that's not there yet. > > @@ +221,3 @@ > + panel->app_ids = g_settings_get_strv (panel->master_settings, > + "application-children"); > + g_qsort_with_data (panel->app_ids, g_strv_length (panel->app_ids), > > The sorting should be based on last use (so that the last application to show a > popup will be at the top of the list, as the usual use of that settings panel > is "shut up application"). > > The shell would return a list of identifiers, in MRU order, which we'd use for > sorting. That sorting is not used for showing (that's handled by GtkTreeView), it's just for computingly quickly (< O(N^2)) the differences between the new and the old list, to find which rows to remove and which to add. As writing dconf is slow, I'd like to avoid changing a setting for each notification if possible, and I believe alphabetical order is fine.
PS: do we really expect anyone to build gnome-control-center with msvc? If not, I don't see why C99 should be rejected.
Created attachment 228707 [details] [review] Split text cell renderer out of network panel The network panel has an activatable text cell renderer. Split it out and rename it as CcCellRendererText.
Created attachment 228708 [details] [review] Add a GtkCellRenderer that renders like a GtkSwitch Copy some code from GtkSwitch and obtain a GtkCellRendererToggle-derived class that looks like a switch, but is actually a cell renderer and can be used togheter with GtkTreeView, alleviating the need for EggListBox.
Created attachment 228709 [details] [review] Add Notifications panel For the 3.7 notifications filtering feature, we introduce a new panel that will show applications using the message tray, and we allow configure in what way the shell presents them.
Created attachment 228710 [details] [review] Add styling for cell renderer switches in the control center The control center uses switches rendered through a GtkCellRenderer, and those don't take the styling from GtkSwitch. Patch for gnome-themes-standard, to make the CcCellRendererSwitches look nice.
Created attachment 229929 [details] [review] Add Notifications panel For the 3.7 notifications filtering feature, we introduce a new panel that will show applications using the message tray, and we allow configure in what way the shell presents them. Rebased on bug 687490, dropped GtkTreeView in favor of EggListBox.
(In reply to comment #11) > PS: do we really expect anyone to build gnome-control-center with msvc? > If not, I don't see why C99 should be rejected. No, but we do expect them to build with Sun CC (used to be named SunPro, not sure what it's called now), which doesn't support C99.
(In reply to comment #17) > (In reply to comment #11) > > PS: do we really expect anyone to build gnome-control-center with msvc? > > If not, I don't see why C99 should be rejected. > > No, but we do expect them to build with Sun CC (used to be named SunPro, not > sure what it's called now), which doesn't support C99. http://docs.oracle.com/cd/E19205-01/820-7598/bjayy/index.html tells me otherwise.
Review of attachment 229929 [details] [review]: ::: panels/notifications/cc-edit-dialog.c @@ +28,3 @@ +#include <gio/gdesktopappinfo.h> + +#include <shell/cc-cell-renderer-text.h> This include is no longer needed, and breaks the build
Again, nice work ! I think On/Off in the mockups are meant to be just textual representation of the current state, not switches - you can see how I've done that for the privacy panel in the wip/privacy branch. You should also add separators between rows in the list box (can again look at wip/privacy for how to do that with egg_list_box_set_separator_funcs), and add some padding in the popup, otherwise it looks very close to the mockups. Some details like hooking up mnemonics still missing.
Comment on attachment 229929 [details] [review] Add Notifications panel Marking it as needs-work based on Matthias' review.
Created attachment 230312 [details] [review] Add Notifications panel For the 3.7 notifications filtering feature, we introduce a new panel that will show applications using the message tray, and we allow configure in what way the shell presents them.
Created attachment 230526 [details] [review] notifications: Add Notifications panel The notifications panel will be used to filter notifications from applications, and configure how they will be presented by gnome-shell.
This could be working, but I don't have any applications showing up in the list. I think that relying on applications to launch a notification for them to show up in the list is a bad idea. On other OSes, application that need notifications will make the system popup a dialogue asking whether the application should be allowed to popup notifications. This also makes sure that applications know that their notifications will not be visible. I don't think we have a way to do that right now.
So, what is the next step here ? Bastien, you want the list pre-populated ? Giovanni, can we do that from the shell side ?
(In reply to comment #25) > So, what is the next step here ? > Bastien, you want the list pre-populated ? The other behaviour changes (denying by default) would be something separate. Having the list pre-populated is an absolute requirement though. Cosimo mentioned exporting the use of the notification system through the .desktop files. > Giovanni, can we do that from the shell side ?
(In reply to comment #26) > (In reply to comment #25) > > So, what is the next step here ? > > Bastien, you want the list pre-populated ? > > The other behaviour changes (denying by default) would be something separate. > > Having the list pre-populated is an absolute requirement though. Cosimo > mentioned exporting the use of the notification system through the .desktop > files. Will that require more XDG bureaucracy? (Like the dead jumplists 3.2 feature...) And what about non GNOME apps? Denying by default could be considered "breaking working code" by ISVs. I'm not personally convinced it makes sense, given the history of allowing everything through.
(In reply to comment #27) > (In reply to comment #26) > > (In reply to comment #25) > > > So, what is the next step here ? > > > Bastien, you want the list pre-populated ? > > > > The other behaviour changes (denying by default) would be something separate. > > > > Having the list pre-populated is an absolute requirement though. Cosimo > > mentioned exporting the use of the notification system through the .desktop > > files. > > Will that require more XDG bureaucracy? > (Like the dead jumplists 3.2 feature...) We can just make it X-GNOME-
(In reply to comment #27) <snip> > Denying by default could be considered "breaking working code" by ISVs. > I'm not personally convinced it makes sense, given the history of allowing > everything through. I'm not sure I can make comment 26 any clearer: >The other behaviour changes (denying by default) would be something separate.
Created attachment 231400 [details] [review] Add Notifications panel For the 3.7 notifications filtering feature, we introduce a new panel that will show applications using the message tray, and we allow configure in what way the shell presents them. The set of applications shown include all applications that ever showed a notification in gnome-shell and all applications that have a X-GNOME-UsesNotifications in the desktop file.
Review of attachment 231400 [details] [review]: ::: panels/notifications/cc-notifications-panel.c @@ +387,3 @@ + */ + load_menu_tree (panel); +} With glib 2.35, you can probably get this information with g_app_info_list_all and g_desktop_app_info_get_string and avoid bringing in gnome-menus
(In reply to comment #32) > Review of attachment 231400 [details] [review]: > > ::: panels/notifications/cc-notifications-panel.c > @@ +387,3 @@ > + */ > + load_menu_tree (panel); > +} > > With glib 2.35, you can probably get this information with g_app_info_list_all > and g_desktop_app_info_get_string and avoid bringing in gnome-menus Oh, that's useful to hear. Now I can go back to async loading (which I first implmented, then discovered libgmenu is not thread-safe...)
Created attachment 231409 [details] [review] Add Notifications panel For the 3.7 notifications filtering feature, we introduce a new panel that will show applications using the message tray, and we allow configure in what way the shell presents them. The set of applications shown include all applications that ever showed a notification in gnome-shell and all applications that have a X-GNOME-UsesNotifications in the desktop file. Now with async loading!
Review of attachment 231409 [details] [review]: You were missing updates to the POTFILES. ::: panels/notifications/cc-notifications-panel.c @@ +69,3 @@ + +static void +cc_notifications_panel_dispose (GObject *object) I added a note as to why a separate dispose is necessary (took me a while to notice). @@ +104,3 @@ + /* Yes, this looks wrong, but it's a work around for the bad + refcounting in egg-list-box (and fixing that would require + changing every other user of it) Removed in favour of linking to: https://bugzilla.gnome.org/show_bug.cgi?id=690545 @@ +311,3 @@ + int l; + + desktop_id = (char*) g_app_info_get_id (app_info); This is dirty. @@ +323,3 @@ + + l = strlen (desktop_id); + if (G_UNLIKELY (l <= strlen(".desktop") || Any reasons why you didn't just use g_str_has_suffix()? @@ +385,3 @@ + + app = iter->data; + if (g_desktop_app_info_get_boolean (app, "X-GNOME-UsesNotifications")) Can you please start filing bugs against applications that should use this? Will we have whitelisting in the shell, or will we need to implement this here to stop "gnome-settings-daemon" from showing up for example (or having it show up as separate "applications" depending on the usage)? @@ +393,3 @@ + +static void +load_menu_tree_async (CcNotificationsPanel *panel) I've renamed all the "menu" instances by "apps" ::: panels/notifications/cc-notifications-panel.h @@ +37,3 @@ +GType cc_notifications_panel_get_type (void) G_GNUC_CONST; + +void cc_build_edit_dialog (CcNotificationsPanel *panel, GAppInfo *app, GSettings *settings); This should have had its own header file. @@ +41,3 @@ +G_END_DECLS + +#endif /* __GOA_PANEL_H__ */ Sure :) ::: shell/gnome-control-center.c @@ +72,3 @@ extern GType cc_network_panel_get_type (void); #endif /* BUILD_NETWORK */ +extern GType cc_notifications_panel_get_type (void); This needed updating too.
Please make sure to answer the questions above about the X-GNOME-UsesNotifications key, and whitelisting/blacklisting of system level apps. commit e8cd35edce01bc73d321e3cb2aa2021f4cdfe5a5 Author: Giovanni Campagna <gcampagna@src.gnome.org> Date: Wed Oct 31 22:31:06 2012 +0100 notifications: Add panel Show applications using the message tray, and allow configuring in what way the shell presents them. The set of applications shown include all applications that ever showed a notification in gnome-shell and all applications that have a boolean X-GNOME-UsesNotifications key set to true in their desktop file. https://bugzilla.gnome.org/show_bug.cgi?id=685928
For the g_str_has_suffix() question, the reason was efficiency. I wanted to compute strlen() only once, and I didn't want to call strlen(".destkop") inside g_str_has_suffix(). Not a big win, probably. As for X-GNOME-UsesNotifications, this needs to be added to https://live.gnome.org/action/diff/GnomeGoals/NotificationSource, and then implemented in every applications. System level notifications should use desktop-entry to associate with an desktop file, otherwise they get the default policy (so only the global switches apply). For gnome-shell, you can see in the other bug I tried to use core apps or gnome-control-center panels. That's not always possible, and some notifications, especially from g-s-d, maybe need to override the default policy. We can think of this on a case by case basis, so if you find something, just open a shell bug. It's trivial to add a hint like "x-gnome-override-policy".
I guess you mean: https://live.gnome.org/GnomeGoals/NotificationSource For at least gnome-settings-daemon, we'll need to get bugs opened. The power notifications aren't optional for example. In g-s-d we have those plugins with notifications: a11y-keyboard color housekeeping power print-notifications updates Color and housekeeping's notifications can already be controlled separately. Updates should be opt-in, the others should probably be always on.