GNOME Bugzilla – Bug 685926
message tray: add filtering for notifications
Last modified: 2013-01-31 12:46:38 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. I've created a GNOME goal to set this hint on app notifications: https://live.gnome.org/GnomeGoals/NotificationSource Quickly looking at the code in notificationDaemon.js, the code around // Filter out chat, presence, calls and invitation notifications from // Empathy, since we handle that information from telepathyClient.js looks like a perfect place to add this kind of filtering. It does point out as well that chat notifications need to be handled somewhere else.
Created attachment 226573 [details] [review] MessageTray: introduce configurable per-source notification policy Allow message tray sources to provide a NotificationPolicy object, that will configure how and if the source is displayed. For notification daemon sources, this object is hooked to GSettings. This is a very rough patch, but I think it gets a feeling of it could be implemented on the shell side. The filtering is at a later stage than suggested, as having the policy there would be too coarse and would not allow for filtering in the lock screen. It's yet to be discussed if the empathy specific hacks should be rewritten in terms of the new NotificationPolicy, if they should be kept as they are, of if empathy should be fixed not to generate those unnecessary notifications when the shell is running. While we're there, we should also consider dropping the English only XChat hack.
*** Bug 685927 has been marked as a duplicate of this bug. ***
*** Bug 557974 has been marked as a duplicate of this bug. ***
Review of attachment 226573 [details] [review]: ::: js/ui/messageTray.js @@ +256,3 @@ +// A notification without a policy object will inherit the default one. +const NotificationPolicy = new Lang.Class({ + Name: 'NotificationPolicy', Why not just a regular object? @@ +1109,3 @@ + + get policy() { + this._ensurePolicy(); uh, considering the policy is going to be used almost immediately after creating the summary item, it doesn't make sense to have an _ensurePolicy thing. Just doing this.policy = this._createPolicy(); in the constructor should be enough, no? ::: js/ui/notificationDaemon.js @@ +106,3 @@ +const NotificationGSettingsPolicy = new Lang.Class({ + Name: 'NotificationGSettingsPolicy', + Extends: MessageTray.NotificationPolicy, Why bother inheriting at all? @@ +114,3 @@ + this.id = id; + this._settings = new Gio.Settings({ schema: 'org.gnome.shell.notifications', + path: '/org/gnome/shell/notifications/' + this._canonicalizeId(id) + '/' }); I wonder what should happen if the settings change at runtime. Show/hide all previous summary items so that the new policies apply? Should we have a 'policy-changed' signal on the SummaryItem? @@ +564,3 @@ this.trayIcon = trayIcon; if (this.trayIcon) { + // Try again finding the app, using the WM_CLASS from the tray icon This sort of thing should be in a separate patch. (We should also pay attention to the "desktop-entry" hint in the notification) @@ +584,3 @@ + return new NotificationGSettingsPolicy(id); + else + return new MessageTray.NotificationPolicy(); What's the case where we don't have an ID?
(In reply to comment #4) > Review of attachment 226573 [details] [review]: > > ::: js/ui/messageTray.js > @@ +256,3 @@ > +// A notification without a policy object will inherit the default one. > +const NotificationPolicy = new Lang.Class({ > + Name: 'NotificationPolicy', > > Why not just a regular object? Because sometimes it is a regular Params-filled object, sometimes it is a full-blown instance with getters. > @@ +1109,3 @@ > + > + get policy() { > + this._ensurePolicy(); > > uh, considering the policy is going to be used almost immediately after > creating the summary item, it doesn't make sense to have an _ensurePolicy > thing. > > Just doing this.policy = this._createPolicy(); in the constructor should be > enough, no? Ok > ::: js/ui/notificationDaemon.js > @@ +106,3 @@ > +const NotificationGSettingsPolicy = new Lang.Class({ > + Name: 'NotificationGSettingsPolicy', > + Extends: MessageTray.NotificationPolicy, > > Why bother inheriting at all? Cleanliness? > @@ +114,3 @@ > + this.id = id; > + this._settings = new Gio.Settings({ schema: > 'org.gnome.shell.notifications', > + path: > '/org/gnome/shell/notifications/' + this._canonicalizeId(id) + '/' }); > > I wonder what should happen if the settings change at runtime. Show/hide all > previous summary items so that the new policies apply? Should we have a > 'policy-changed' signal on the SummaryItem? Uhm... Yeah, it probably makes sense. Btw, we still have to define what is the policy (the mockups are not very clear for example on the meaning of "details"). > @@ +564,3 @@ > this.trayIcon = trayIcon; > if (this.trayIcon) { > + // Try again finding the app, using the WM_CLASS from the tray > icon > > This sort of thing should be in a separate patch. (We should also pay attention > to the "desktop-entry" hint in the notification) The patch does this, but I'll split it. > @@ +584,3 @@ > + return new NotificationGSettingsPolicy(id); > + else > + return new MessageTray.NotificationPolicy(); > > What's the case where we don't have an ID? notify-send (or other untracked sender), without tray icon and without desktop-entry hint.
Created attachment 227934 [details] [review] MessageTray: introduce configurable per-source notification policy Allow message tray sources to provide a NotificationPolicy object, that will configure how and if the source is displayed. For notification daemon sources, this object is hooked to GSettings. Updating to make bug 685928 testable.
Created attachment 227994 [details] [review] MessageTray: introduce configurable per-source notification policy Allow message tray sources to provide a NotificationPolicy object, that will configure how and if the source is displayed. For notification daemon sources, this object is hooked to GSettings. This patch includes all policy bits exposed in the panel and is now ready for review.
Created attachment 227995 [details] [review] NotificationDaemon: improve associating sources with applications Try harder to find a ShellApp for a notification source, by looking at the PID, tray icon wm_class and desktop-entry libnotify hint. This is needed to find the correct policy.
Created attachment 227996 [details] [review] Add policy for builtin message sources Some notifications, despite being emitted by shell code, should appear to be from application or "separable" system components. Do that by associating them with a notification-daemon policy. Note that for this to look really good, empathy should rename itself to Chat. I'm not particularly sure about this one, but for example nm-applet had the dreaded "Not notify again" buttons, and at least for chat it's quite useful.
Review of attachment 227994 [details] [review]: ::: data/org.gnome.shell.gschema.xml.in.in @@ +219,3 @@ </schema> + + <schema id="org.gnome.shell.notifications" path="/org/gnome/shell/notifications/" This should probably be in a separate schema in gsettings-desktop-schemas. ::: js/ui/messageTray.js @@ +246,3 @@ +// NotificationPolicy: +// An object that holds all bits of configurable policy related to a notification +// source, such as whether to show sound or honour the critical bit. "show sound"? Also, it doesn't look like 'enable-sound' is ever used. I think we have a very old bug about adding sound playback with libcanberra (I think you made a patch for that, actually?); do we want to bring it back? @@ +1111,3 @@ + // Initialize source's NotificationPolicy, + // This is a virtual function, override if you want + // to use a custom policy. All functions in JS are "virtual functions". Find a better message here, or just remove it. @@ +1117,3 @@ + + get policy() { + return this._policy; Why the getter? @@ +1697,3 @@ } + this._summaryItems.push(obj); Either keep _summaryItems full of SummaryItems, or rename this. I've never liked the .sources / ._summaryItems split (getIndexOfSummaryItemForSource, etc.) -- maybe you can clean this up in a preliminary patch? @@ +1761,3 @@ }, + getSummaryItems: function() { Why did this move? @@ +2312,3 @@ // to make sure their position is updated. + if (this._notification.urgency == Urgency.CRITICAL || + this._notification.source.policy.forceExpanded || lol, I don't think anybody will ever set forceExpanded. @@ +2618,3 @@ this._summaryBoxPointerItem.disconnect(this._summaryBoxPointerContentUpdatedId); this._summaryBoxPointerContentUpdatedId = 0; + if (this._summaryBoxPointerClockClickedId) { Why the guard? @@ -2532,2 @@ this._summaryBoxPointerItem.source.disconnect(this._sourceDoneDisplayingId); - this._summaryBoxPointerDoneDisplayingId = 0; Can you make this dumb error into a separate one-liner, and push immediately? ::: js/ui/notificationDaemon.js @@ +113,3 @@ + + this.id = id; + this._canonicalId = this._canonicalizeId(id) ; @@ +115,3 @@ + this._canonicalId = this._canonicalizeId(id) + + this._masterSettings = new Gio.Settings({ schema: 'org.gnome.shell.notifications' }); Why don't shell notifications respect the master settings? @@ +123,3 @@ + }, + + store: function() { Why are you storing anything at all? @@ +124,3 @@ + + store: function() { + this._settings.set_string('application-id', this.id + '.desktop'); ??? This is already part of the path name. @@ +129,3 @@ + if (apps.indexOf(this._canonicalId) < 0) { + apps.push(this._canonicalId); + this._masterSettings.set_strv('application-children', apps); ??? You never read this. What is it for? @@ +142,3 @@ + this.emit('enable-changed'); + else + this.emit('policy-changed'); Just policy-changed, please, and add the key so that people can react sanely. @@ +146,3 @@ + + _canonicalizeId: function(id) { + return id.replace(/[\/\-._ ]/g, '-'); Why? GSettings can handle all those characters just fine. @@ +154,3 @@ + + get enableSound() { + return this._settings.get_boolean('enable-sound'); This should have a master settings thing, right? ::: js/ui/screenShield.js @@ +237,3 @@ + _onPolicyChanged: function(policy) { + let obj = this._items[this._findPolicy(policy)]; Just have policy-changed emit the notification as a separate parameter, rather than this nonsense. @@ +245,3 @@ let itemShouldBeResident = this._sourceIsResident(obj.source); + if (!itemShouldBeVisible) { Flow control in here is very nasty. I'd try to flatten it out as much as possible, and split it up: if (itemShouldBeVisible && !obj.visible) { // ... } else if (!itemShouldBeVisible && obj.visible) { // ... } if (itemShouldBeResident && !obj.resident) { // ... } else if (!itemShouldBeResident && obj.resident) { // ... } Note that we may remove resident notifications in the lock screen (or entirely, maybe) in 3.8. The only use we've found for them so far is a poor media player control, which we're hopefully going to be replacing with MPRIS (I know you have your own opinions on that) @@ +267,3 @@ + let count = obj.source.unseenCount; + obj.countLabel.text = this._makeNotificationCountText(count, obj.source.isChat); + obj.sourceBox.visible = count != 0; Shouldn't this be done everywhere, not when we update an item and it's already fine and cool? @@ +321,3 @@ obj.source.disconnect(obj.sourceDestroyId); obj.source.disconnect(obj.sourceCountChangedId); + obj.source.policy.disconnect(obj.policyChangedId) ;
Review of attachment 227995 [details] [review]: Could you strip out the policy stuff here, and move it before the policy one? ::: js/ui/notificationDaemon.js @@ +593,3 @@ + this.trayIcon = trayIcon; + this.pid = pid; + this.app = this._getApp(appId); _setApp? @@ +619,3 @@ }, + _createPolicy: function() { ... the previous commit should hook up the policy, here. @@ +624,3 @@ + if (this.app) { + // remove '.desktop' + id = this.app.get_id().slice(0, -8); this.app.get_id().replace(/\.desktop$/, ''); @@ +626,3 @@ + id = this.app.get_id().slice(0, -8); + } else if (this.trayIcon) { + id = this.trayIcon.wm_class; If we didn't have an app from the WM_CLASS, I don't think we should be doing this.
Review of attachment 227996 [details] [review]: I wonder if it makes sense to add an "appid" parameter to the Source constructor, and use that always. I'm quite sure we could find an appropriate .desktop file for the auto-run guy, and then we could remove the weird non-GSettings policies. ::: js/ui/components/telepathyClient.js @@ +395,3 @@ this._subscriptionSource = new MessageTray.Source(_("Subscription request"), 'gtk-dialog-question'); + this._accountSource.policy = new NotificationDaemon.NotificationGSettingsPolicy('empathy'); what's an _accountSource? ::: js/ui/messageTray.js @@ -1117,3 @@ - get policy() { - return this._policy; Sigh. I guess I should read all patches before reviewing. Please squash this with the original.
(In reply to comment #10) > Review of attachment 227994 [details] [review]: > [...] > > @@ +115,3 @@ > + this._canonicalId = this._canonicalizeId(id) > + > + this._masterSettings = new Gio.Settings({ schema: > 'org.gnome.shell.notifications' }); > > Why don't shell notifications respect the master settings? Some shell notifications are special (see e.g. bug 662900). I agree though that a NotificationGenericPolicy that only respects masterSettings makes sense. > @@ +123,3 @@ > + }, > + > + store: function() { > > Why are you storing anything at all? It's part of the mechanism to "register" applications with the control center panel. As soon as they push a notification, a new dconf node is created, pointing to the new application. From the second notification on, this should be a no-op, because the value is not changing. > @@ +124,3 @@ > + > + store: function() { > + this._settings.set_string('application-id', this.id + '.desktop'); > > ??? > > This is already part of the path name. But the path name is canonicalized to comply with GSettings/dconf rules, while the application-id is something that the control center can pass to g_desktop_app_info_new(). > @@ +129,3 @@ > + if (apps.indexOf(this._canonicalId) < 0) { > + apps.push(this._canonicalId); > + this._masterSettings.set_strv('application-children', apps); > > ??? > > You never read this. What is it for? It's a hack for not having GSettingsList yet. It should be removed once that lands. > [...] > > @@ +146,3 @@ > + > + _canonicalizeId: function(id) { > + return id.replace(/[\/\-._ ]/g, '-'); > > Why? GSettings can handle all those characters just fine. "Key names are restricted to lowercase characters, numbers and '-'. Furthermore, the names must begin with a lowercase character, must not end with a '-', and must not contain consecutive dashes" (from the GSettings documentation) Even if we assume that dconf and dbus work fine with paths containing weird characters (which is not a given), / is still special and needs escaping. > @@ +154,3 @@ > + > + get enableSound() { > + return this._settings.get_boolean('enable-sound'); > > This should have a master settings thing, right? The set of master settings is derived from the mockup at https://live.gnome.org/Design/SystemSettings/Notifications?action=AttachFile&do=get&target=NotificationsSettings.png > @@ +245,3 @@ > let itemShouldBeResident = this._sourceIsResident(obj.source); > > + if (!itemShouldBeVisible) { > > Flow control in here is very nasty. I'd try to flatten it out as much as > possible, and split it up: > > if (itemShouldBeVisible && !obj.visible) { > // ... > } else if (!itemShouldBeVisible && obj.visible) { > // ... > } > > if (itemShouldBeResident && !obj.resident) { > // ... > } else if (!itemShouldBeResident && obj.resident) { > // ... > } > > Note that we may remove resident notifications in the lock screen (or entirely, > maybe) in 3.8. The only use we've found for them so far is a poor media player > control, which we're hopefully going to be replacing with MPRIS (I know you > have your own opinions on that) Except that the mockup has "Show details in the lock screen", which is interpreted as residentInLockScreen. > @@ +267,3 @@ > + let count = obj.source.unseenCount; > + obj.countLabel.text = this._makeNotificationCountText(count, > obj.source.isChat); > + obj.sourceBox.visible = count != 0; > > Shouldn't this be done everywhere, not when we update an item and it's already > fine and cool? If we update the item and it becomes invisible, we destroy anything it is showing, so it doesn't matter. If we update the item and it becomes visible and non resident, we're creating the actor ex novo and the countLabel is already ok. If we update the item and it is visible and resident, updates are handled by SummaryItem, so countLabel is null.
Review of attachment 227994 [details] [review]: ::: js/ui/notificationDaemon.js @@ +118,3 @@ + this._settings = new Gio.Settings({ schema: 'org.gnome.shell.notifications.application', + path: '/org/gnome/shell/notifications/application/' + this._canonicalId + '/' }); + This needs to be updated for the gsettings-desktop-schemas location of these settings.
I've tried these patches now - nice work !
Created attachment 230315 [details] [review] Add a new HashTable module A semplicistic HashTable data structure, uses JS objects and System.addressOf() to support keys that are arbitrary objects. Should help replacing linear searches in various places around the shell.
Created attachment 230317 [details] [review] MessageTray: fix signal tracking for summary notifications 'done-displaying' was moved from SummaryItem to Source some time ago, and 'close-clicked' not connected when the source is right-clicked.
Created attachment 230318 [details] [review] MessageTray: clean up source tracking Use the new Hash.HashTable class, and store signal connections along with the source and summaryItem. This allows to remove sources without destroying them.
Created attachment 230319 [details] [review] MessageTray: introduce configurable per-source notification policy Allow message tray sources to provide a NotificationPolicy object, that will configure how and if the source is displayed. For notification daemon sources, this object is hooked to GSettings.
Created attachment 230320 [details] [review] NotificationDaemon: improve associating sources with applications Try harder to find a ShellApp for a notification source, by looking at the PID, tray icon wm_class and desktop-entry libnotify hint. This is needed to find the correct policy.
Created attachment 230321 [details] [review] Add policy for builtin message sources Some notifications, despite being emitted by shell code, should appear to be from application or "separable" system components. Do that by associating them with a notification-daemon policy. Note that for this to look really good, empathy should rename itself to Chat.
Review of attachment 230315 [details] [review]: I'd prefer this to be a shim of the ES6 WeakMap/Map/Set collections for easier porting. See: http://wiki.ecmascript.org/doku.php?id=harmony:simple_maps_and_sets http://wiki.ecmascript.org/doku.php?id=harmony:weak_maps There are also existing shims we can possibly use: https://github.com/Benvie/harmony-collections https://github.com/WebReflection/es6-collections
Review of attachment 230317 [details] [review]: Commit message is confusing. This doesn't touch anything with 'close clicked' in the name, so why are you talking about it?
Review of attachment 230318 [details] [review]: Will obviously require updates for ES6 shim. ::: js/ui/messageTray.js @@ +1712,3 @@ + _removeSource: function(source) { + let [, obj] = this._sources.remove(source); Huh; never realized you could do that.
Review of attachment 230319 [details] [review]: I'm skeptical of the needs of a "NotificationPolicy" object, especially now that Bastien didn't like the "prefs on first notification" aspect. No code review, just pointing it out.
Review of attachment 230320 [details] [review]: Mixes _createPolicy and other changes. These changes should be pushed before any of the policy stuff.
Review of attachment 230321 [details] [review]: ::: js/ui/status/bluetooth.js @@ +288,3 @@ if (!this._source) { this._source = new MessageTray.Source(_("Bluetooth"), 'bluetooth-active'); + this._source.policy = new NotificationDaemon.NotificationApplicationPolicy('bluetooth-properties'); Not sure 'bluetooth-properties' is an app that makes sense. I think this is a system notification. ::: js/ui/status/network.js @@ +1675,3 @@ this._source = new MessageTray.Source(_("Network Manager"), 'network-transmit-receive'); + this._source.policy = new NotificationDaemon.NotificationApplicationPolicy('gnome-network-panel'); Same here. Talk to the designers and Bastien and Mattias about it.
(In reply to comment #27) > Review of attachment 230321 [details] [review]: > > ::: js/ui/status/bluetooth.js > @@ +288,3 @@ > if (!this._source) { > this._source = new MessageTray.Source(_("Bluetooth"), > 'bluetooth-active'); > + this._source.policy = new > NotificationDaemon.NotificationApplicationPolicy('bluetooth-properties'); > > Not sure 'bluetooth-properties' is an app that makes sense. I think this is a > system notification. Why is bluetooth-properties in the gnome-shell sources? It was never in GNOME 3.0 or any of the later releases (replaced by the Bluetooth control-center panel[1]) and it didn't popup any notifications. [1]: http://www.hadess.net/2011/02/bluetooth-panel.html
The panel has been committed now, so we should get this merged as well
Created attachment 232121 [details] [review] Add a new Hash module A simple implementation of the ES6 Map proposal, internally done as a hash table, using System.addressOf() to support keys that are arbitrary objects. Should help replacing linear searches in various places around the shell.
Created attachment 232122 [details] [review] MessageTray: fix signal tracking for summary notifications 'done-displaying' was moved from SummaryItem to Source some time ago The previous message was just a bad rebase from a patch that covered both.
Created attachment 232123 [details] [review] MessageTray: clean up source tracking Use the new Hash.Map class, and store signal connections along with the source and summaryItem. This allows to remove sources without destroying them.
Created attachment 232124 [details] [review] NotificationDaemon: improve associating sources with applications Try harder to find a ShellApp for a notification source, by looking at the PID, tray icon wm_class and desktop-entry libnotify hint.
Created attachment 232125 [details] [review] MessageTray: introduce configurable per-source notification policy Allow message tray sources to provide a NotificationPolicy object, that will configure how and if the source is displayed. For notification daemon sources, this object is hooked to GSettings. We need an object because we need something that can emit signals. Also, X-GNOME-UsesNotifications is not relevant to the shell, because we need to support third party applications that don't use it.
Created attachment 232126 [details] [review] Add policy for builtin message sources Some notifications, despite being emitted by shell code, should appear to be from application or "separable" system components. Do that by associating them with a notification-daemon policy. Note that for this to look really good, empathy should rename itself to Chat. I changed bluetooth-properties to gnome-bluetooth-panel. Other than that, I believe it is a good idea to associate notifications with control center panels. In the notification panel, they would just show up as Network or Bluetooth, and there exists a use case for hiding those (especially network, which can be annoying if the connection fails often)
Created attachment 232127 [details] [review] MessageTray: clean up source tracking Use the new Hash.Map class, and store signal connections along with the source and summaryItem. This allows to remove sources without destroying them. Ops, forgot one piece.
Apps are starting to adapt to the notification filtering, maybe it is time to review the shell bits again?
Would be nice to get this landed, indeed
Review of attachment 232121 [details] [review]: Note that JS188 (in talks with Mozilla at this very moment about fast-tracking a tarball) has a native "Map" class. We should try hard to make this compatible with that. ::: js/misc/hash.js @@ +19,3 @@ + Known differences from the ES6 proposal: + - Map is a Lang.Class, not a ES6 class, so inheritance, prototype, + sealing, etc. work differently This is fine. @@ +23,3 @@ + an hash function and equality comparator + - the default equalKey is is not compatible with ES6 because + +0 === -0, but !(+0 SameValue -0), and NaN !== NaN, but NaN SameValue NaN SameValue isn't that hard to implement, so I'd prefer we just did it right. http://wiki.ecmascript.org/doku.php?id=harmony:egal has a polyfill implementation. @@ +24,3 @@ + - the default equalKey is is not compatible with ES6 because + +0 === -0, but !(+0 SameValue -0), and NaN !== NaN, but NaN SameValue NaN + - .size and .forEach are non standard extensions .size() is in JS188, .forEach is not. They instead have .keys(), .values() and .items(). @@ +31,3 @@ + _init: function(iterable, params) { + params = Params.parse(params, { hashKey: System.addressOf, + equalKey: function(a, b) { return a === b; } }); I'd prefer not to have these be configurable by just a few params. If you really need this flexibility, let people subclass it and override _hashKey and _equalKey, but this feature will go away when we have JS188. @@ +38,3 @@ + if (iterable) { + for (let i = 0; i < iterable.length; i++) { + this.set(iterable[0], iterable[1]); let [key, value] = iterable; this.set(key, value); @@ +93,3 @@ + }, + + get size() { .size() is a method in JS188. Let's keep it compatible.
Review of attachment 232122 [details] [review]: OK.
Review of attachment 232124 [details] [review]: OK.
OK. Before I go further, I'd like clarified designs. As far as I'm aware, "Show Details in Lock Screen" refers to adding the body text in the lock screen, but not any content image or action area. So I think that the interpretation as "resident-in-lock-screen" is incorrect. If we can leave this off for now and revisit it later, that's fine with me.
Review of attachment 232125 [details] [review]: Not doing an in-depth review due to concerns mentioned in the bug. ::: js/ui/components/autorunManager.js @@ +302,3 @@ + _createPolicy: function() { + return new MessageTray.NotificationPolicy({ showInLockScreen: false }); Shouldn't this be in the next patch? ::: js/ui/messageTray.js @@ +1707,3 @@ + source.policy.connect('enable-changed', Lang.bind(this, this._onSourceEnableChanged, source)); + source.policy.connect('policy-changed', Lang.bind(this, this._updateState)); + if (source.policy.enable) this._onSourceEnableChanged(source.policy, source); ::: js/ui/screenShield.js @@ +249,3 @@ + _onPolicyChanged: function(policy) { + let obj = this._items[this._findPolicy(policy)]; *ahem* "Just have policy-changed emit the notification as a separate parameter, rather than this nonsense." @@ -850,3 @@ this._lockScreenContents.add_actor(this._lockScreenContentsBox); - if (this._settings.get_boolean('show-notifications')) { This key is deprecated.
Review of attachment 232125 [details] [review]: ::: js/ui/notificationDaemon.js @@ +672,3 @@ + if (this.app) { + // remove '.desktop' + id = this.app.get_id().slice(0, -8); *ahem* "this.app.get_id().replace(/\.desktop$/, '');" @@ +674,3 @@ + id = this.app.get_id().slice(0, -8); + } else if (this.trayIcon) { + id = this.trayIcon.wm_class; *ahem* "If we didn't have an app from the WM_CLASS, I don't think we should be doing this."
Review of attachment 232126 [details] [review]: OK.
Review of attachment 232127 [details] [review]: This will require a port to the new Map API.
(In reply to comment #42) > OK. Before I go further, I'd like clarified designs. As far as I'm aware, "Show > Details in Lock Screen" refers to adding the body text in the lock screen, but > not any content image or action area. > > So I think that the interpretation as "resident-in-lock-screen" is incorrect. > If we can leave this off for now and revisit it later, that's fine with me. Having a third "expanded but not really" state for notifications is going to be an implementation nightmare. You're aware of it, right?
(In reply to comment #47) > Having a third "expanded but not really" state for notifications is going to be > an implementation nightmare. You're aware of it, right? It's not a state for the notification object. What we need is: SourceActor Title Description and that's it. You can mock that up from the Notification object. I'd double-check with cosimoc and mccann on the designs, though I believe they're on a plane to Brussels right now.
(In reply to comment #48) > (In reply to comment #47) > > Having a third "expanded but not really" state for notifications is going to be > > an implementation nightmare. You're aware of it, right? > > It's not a state for the notification object. What we need is: > > SourceActor Title > Description > > and that's it. You can mock that up from the Notification object. I'd > double-check with cosimoc and mccann on the designs, though I believe they're > on a plane to Brussels right now. A source doesn't have a description, a notification has. So what I expect by "show details" is that each notification, individually, is shown with its own summary and body. Anyway, it turns out not to be that bad, so patches coming...
Created attachment 234550 [details] [review] Add a new Hash module A simple implementation of the ES6 Map proposal, internally done as a hash table, using System.addressOf() to support keys that are arbitrary objects. Should help replacing linear searches in various places around the shell.
Created attachment 234551 [details] [review] MessageTray: clean up source tracking Use the new Hash.Map class, and store signal connections along with the source and summaryItem. This allows to remove sources without destroying them.
Created attachment 234552 [details] [review] MessageTray: introduce configurable per-source notification policy Allow message tray sources to provide a NotificationPolicy object, that will configure how and if the source is displayed. For notification daemon sources, this object is hooked to GSettings.
Created attachment 234553 [details] [review] MessageTray: reparent notification actors when adding them a notification stack The notification actor might already have a parent, for example when the notification is currently being shown as a banner, and the user activates the lock screen. This became more visible now that all notifications can be visible in full in the lock screen (if so the user desires)
Review of attachment 234550 [details] [review]: One minor nit. ::: js/misc/hash.js @@ +6,3 @@ +const Params = imports.misc.params; + +function _sameValue(x, y) { Really should paste a link to the ES6 wiki in here, saying where it came from. @@ +24,3 @@ + string: function(s) { return s; }, + number: function(n) { return String(n); }, + undefined: function() { return 'undefined'; }, ES6 supports functions as keys, but I'm fine with not supporting that. @@ +46,3 @@ + the table while iterating + (admittedly, the ES6 spec is a bit unclear on this, and + the reference code would just blow up) Firefox JS already has iterators and yield, but they're woefully incomplete, so I'm not sure we can use them. The fancy "*items() {" class syntax desugars to "function* items()", which denotes that a function returns an iterator/generator and can use "yield" in its body instead of an immediate return value. Firefox doesn't have the syntax, it just allows "yield" anywhere, like Python. We probably shouldn't use Firefox JS generators.
Review of attachment 234551 [details] [review]: OK.
Review of attachment 234552 [details] [review]: I'd want a design OK on this before it lands. ::: js/ui/messageTray.js @@ +263,3 @@ + forceExpanded: false, + showInLockScreen: true, + residentInLockScreen: false Rename to "detailsInLockScreen", and the GSettings key to "details-in-lock-screen". ::: js/ui/screenShield.js @@ +206,3 @@ + // "Natively" resident sources are sources with notifications that + // have the resident hint, such has music players. We show these + // with the image and action buttons, if provided. No. Action buttons should never appear in the lock screen, besides the music player case. I'd really double-check with the designers on this. @@ +209,3 @@ + // "Forced" resident sources have their resident-in-lock-screen setting + // on. This is mapped to "Show Details in Lock Screen" in the UI, so + // we show the summary and banner of each notification individually. This should include the source icon actor, the summary and the body, as defined by the notification-spec, stripping URL highlighting. I don't know what the "banner" is. @@ +246,1 @@ + let boundHandler = this._updateItem.bind(this, obj); let handler = Lang.bind(this, function() { this._updateBind(obj); }); @@ +249,3 @@ + obj.sourceTitleChangedId = item.source.connect('title-changed', boundHandler); + obj.policyChangedId = item.source.policy.connect('policy-changed', boundHandler); + obj.sourceDestroyId = item.source.connect('destroy', this._onSourceDestroy.bind(this, obj)); obj.sourceDestroyId = item.source.connect('destroy', Lang.bind(this, function() { this._onSourceDestroy(obj); });
Review of attachment 234553 [details] [review]: We shouldn't lift the notification wholesale, as it has UI (action buttons, URL highlighting) that we can't show in the Lock Screen.
(In reply to comment #56) > Review of attachment 234552 [details] [review]: > > I'd want a design OK on this before it lands. > > ::: js/ui/messageTray.js > @@ +263,3 @@ > + forceExpanded: false, > + showInLockScreen: true, > + residentInLockScreen: false > > Rename to "detailsInLockScreen", and the GSettings key to > "details-in-lock-screen". It's too late the rename the GSettings key, and residentInLockScreen makes it clear what it means, when you look from the POV of the implementation. > ::: js/ui/screenShield.js > @@ +206,3 @@ > + // "Natively" resident sources are sources with notifications that > + // have the resident hint, such has music players. We show these > + // with the image and action buttons, if provided. > > No. Action buttons should never appear in the lock screen, besides the music > player case. I'd really double-check with the designers on this. Now the question becomes... do we have other resident notifications besides music? In existing applications, I mean, not just hypothetical use cases. > @@ +209,3 @@ > + // "Forced" resident sources have their resident-in-lock-screen > setting > + // on. This is mapped to "Show Details in Lock Screen" in the UI, so > + // we show the summary and banner of each notification individually. > > This should include the source icon actor, the summary and the body, as defined > by the notification-spec, stripping URL highlighting. I don't know what the > "banner" is. Heh, body, not banner. And no, it should not include the source icon actor (which is the application icon), it should include the notification icon. At which point you immediately realize the usefulness of the reusing the actual notification actor (you get updates, images, the right layout, etc.) > @@ +246,1 @@ > + let boundHandler = this._updateItem.bind(this, obj); > > let handler = Lang.bind(this, function() { > this._updateBind(obj); > }); > > @@ +249,3 @@ > + obj.sourceTitleChangedId = item.source.connect('title-changed', > boundHandler); > + obj.policyChangedId = item.source.policy.connect('policy-changed', > boundHandler); > + obj.sourceDestroyId = item.source.connect('destroy', > this._onSourceDestroy.bind(this, obj)); > > obj.sourceDestroyId = item.source.connect('destroy', Lang.bind(this, function() > { > this._onSourceDestroy(obj); > }); I see no gain in writing this your way. It is longer and produces longer stack traces. (In reply to comment #57) > Review of attachment 234553 [details] [review]: > > We shouldn't lift the notification wholesale, as it has UI (action buttons, URL > highlighting) that we can't show in the Lock Screen. Why not? The user explicitly opted in for this behavior, we should give him what he asked for.
Besides, we've been doing the resident notification thing for a while now, and nobody complained really.
(In reply to comment #58) > (In reply to comment #56) > > Review of attachment 234552 [details] [review] [details]: > > > > I'd want a design OK on this before it lands. > > > > ::: js/ui/messageTray.js > > @@ +263,3 @@ > > + forceExpanded: false, > > + showInLockScreen: true, > > + residentInLockScreen: false > > > > Rename to "detailsInLockScreen", and the GSettings key to > > "details-in-lock-screen". > > It's too late the rename the GSettings key Why? We haven't released 3.8 yet. > and residentInLockScreen makes it > clear what it means, when you look from the POV of the implementation. Except it's not what it means at all. The design of the "details in lock screen" switch mean that the body string or markup will be displayed in the lock screen, and that's it. It doesn't include any other sort of UI. We should not show custom content or the action area. > > No. Action buttons should never appear in the lock screen, besides the music > > player case. I'd really double-check with the designers on this. > > Now the question becomes... do we have other resident notifications besides > music? In existing applications, I mean, not just hypothetical use cases. Nautilus transfer notifications. Which I believe has a "Show Dialog" action button that either does nothing in the lock screen, or makes the Shell break in some spectacular way. Possibly both. > I see no gain in writing this your way. It is longer and produces longer stack > traces. Consistency with the rest of the codebase, not having to write a giant comment explaining why you're not using Lang.bind which we use everywhere... > (In reply to comment #57) > > Review of attachment 234553 [details] [review] [details]: > > > > We shouldn't lift the notification wholesale, as it has UI (action buttons, URL > > highlighting) that we can't show in the Lock Screen. > > Why not? The user explicitly opted in for this behavior, we should give him > what he asked for. Clicking on Action Buttons doesn't work in the lock screen, except in the Music Player case (we really should add a "lock-screen-safe" hint or something) Clicking on a URL doesn't work in the lock screen. We should refrain from adding UI and behavior that flat out breaks.
Ok, so this needs an implementation redesign on the "resident-in-lock-screen" thing. Essentially, what I understood as the original requirements are: - to provide the user with (actionable) feedback on the ongoing activity of the system (music, but also whatever resident) - to provide the user with a short, cumulative, status of everything else Now I see that the requirements are: - to provide the user with a short, cumulative, summary of unacknowledged notifications - to provide the user with longer, but inactive, feedback for notifications of certain apps - to provide the user music controls (and nothing else, really) As I mentioned other times, I am opposed to having a special case for music in gnome-shell, other than perhaps a libnotify category. In particular, I don't want to use MPRIS, because that has a higher overhead and requires more code and more resources on the shell side. This said, the screen shield still needs to handle music specially, since for that, and that only, we want to take the full actor out of its notification and reparent it to the big black box. (As a side question, I wonder what happens if you have an other notification in the same source as the music app) In addition to that, it needs to "logically" clone the contents of the notifications that are marked for details. Cloning/reparenting the actual actors is a lot easier here, so I'm wondering if the requirements can be stretched in a way that makes implementation easier. Otherwise, I might not get at this soon, because practically speaking the NotificationBox in ScreenShield needs a complete rewrite. And I was hoping to get this in for 3.7.5, given that the panel is merged since 3.7.3... PS: I'll check with the designers on the requirements, but you can correct me right now if you find something wrong.
Filed bug 692894 and bug 692895 for the GSettings key rename.
Created attachment 234848 [details] [review] MessageTray: introduce configurable per-source notification policy Allow message tray sources to provide a NotificationPolicy object, that will configure how and if the source is displayed. For notification daemon sources, this object is hooked to GSettings.
Created attachment 234849 [details] [review] ScreenShield: decouple detailed notifications from resident notifications The designs says that only music notifications should be shown in full in the screenshield, the others should be either shown as a summary or with very light details.
Review of attachment 234848 [details] [review]: ::: js/ui/screenShield.js @@ -852,3 @@ this._lockScreenContents.add_actor(this._lockScreenContentsBox); - if (this._settings.get_boolean('show-notifications')) { Are we going to remove this key?
Review of attachment 234849 [details] [review]: One minor style nit. ::: js/ui/screenShield.js @@ +221,3 @@ + if (n.bannerBodyText) + body = n.bannerBodyMarkup ? n.bannerBodyText : + GLib.markup_escape_text(n.bannerBodyMarkup, -1); Put braces around this, or have a proper if statement.
(In reply to comment #65) > Review of attachment 234848 [details] [review]: > > ::: js/ui/screenShield.js > @@ -852,3 @@ > this._lockScreenContents.add_actor(this._lockScreenContentsBox); > > - if (this._settings.get_boolean('show-notifications')) { > > Are we going to remove this key? I guess we should. It is already marked deprecated, so I'll file a bug to remove its last use in gnome-control-center.
Attachment 232124 [details] pushed as 4dc5bac - NotificationDaemon: improve associating sources with applications Attachment 232126 [details] pushed as 9794e71 - Add policy for builtin message sources Attachment 234550 [details] pushed as daf8be9 - Add a new Hash module Attachment 234551 [details] pushed as b150869 - MessageTray: clean up source tracking Attachment 234848 [details] pushed as 098bd45 - MessageTray: introduce configurable per-source notification policy Attachment 234849 [details] pushed as 8cb3884 - ScreenShield: decouple detailed notifications from resident notifications
Ops, already found a small problem...
Created attachment 234907 [details] [review] ScreenShield: fix fallout from 8cb3884 We must remove music notifications before we're destroyed, otherwise they get destroyed with us. Also, integrate a review comment I previously forgot.
Review of attachment 234907 [details] [review]: OK.
Attachment 234907 [details] pushed as 9ab22fe - ScreenShield: fix fallout from 8cb3884