Created attachment 154312 [details] [review]
Add demands attention support to the messagetray

*** Bug 596431 has been marked as a duplicate of this bug. ***

*** Bug 607930 has been marked as a duplicate of this bug. ***

Created attachment 154325 [details]
Broken window title update

First things first:
Using the message tray to notify about windows setting the demands-attention hint feels right (to me that is) - more than the flashing window list buttons in Gnome 2.

I used gimp to crop the attached screenshot - and found another problem right away. Gimp sets the demands-attention hint after startup, so a notification popped up to inform me that gimp had finished starting. Pretty pointless given the fact that gimp was focussed anyway - not sure if we can consider that a gimp bug, but it might make sense to ignore the hint when the window demanding attention is focussed.

The screenshot shows some mess caused by window title updates - you have to clear the notification when calling update.

I'd like to get some serious ui review about the window previews though - I fail to see how they add anything useful; right now they annoy me as they make it much harder to click the notification icon to get to the window - if we allow clicking the whole notification this point will become void of course.

I'll do a real code review later, but I think that overall it's looking pretty good.

(In reply to comment #4)
> Created an attachment (id=154325) [details]
> Broken window title update
> 
> First things first:
> Using the message tray to notify about windows setting the demands-attention
> hint feels right (to me that is) - more than the flashing window list buttons
> in Gnome 2.
> 
> I used gimp to crop the attached screenshot - and found another problem right
> away. Gimp sets the demands-attention hint after startup, so a notification
> popped up to inform me that gimp had finished starting. Pretty pointless given
> the fact that gimp was focussed anyway - not sure if we can consider that a
> gimp bug, but it might make sense to ignore the hint when the window demanding
> attention is focussed.

Well this is supposed to happen (i.e not show notifications for focused windows; might be a bug somewhere in my code need to debug that).

> The screenshot shows some mess caused by window title updates - you have to
> clear the notification when calling update.

Yeah looks broken will fix that.

> I'd like to get some serious ui review about the window previews though - I
> fail to see how they add anything useful; right now they annoy me as they make
> it much harder to click the notification icon to get to the window - if we
> allow clicking the whole notification this point will become void of course.

You can simply click on the preview to get to the window.

We have window previews in the overview, and in alt-tab it does make sense to me to have them here too.

> I'll do a real code review later, but I think that overall it's looking pretty
> good.

OK, thanks.

Created attachment 154328 [details] [review]
Add demands attention support to the messagetray

OK fixed the issues mentioned above.

The notification is cleared on update now.

The code did not catch the GIMP case because the window is not focused (i.e the main window is focused but one of the docks isn't), I added a heuristic for this this should fixed it.

The patch also contains minor cleanups.

Created attachment 154329 [details] [review]
Add demands attention support to the messagetray

Upload the correct version.

(In reply to comment #5)
> You can simply click on the preview to get to the window.

Yeah, I figured that out when I went to read the code - unfortunately you are doing some nasty things there, so the cleanest solution would probably to make the whole notification clickable.


> We have window previews in the overview, and in alt-tab it does make sense to
> me to have them here too.

(In reply to comment #0)
> Jon's extension implements a "bring window to me" feature which basically moves
> the window to the current workspace and active it there on right click (left
> click actives the window where it currently is).
> 
> This is currently not implemented for consistently reasons (i.e we should have
> it for all notifications not just for demands attention), so once we agree that
> we want this (it is a good idea imho), it can be added.

I don't particularly like the idea of using right-click for anything but bringing up a context menu. Sources may eventually support menus, then it can be added as an option. Right now you can add an action to the notification.


> The design document states:
> 
> ...
> 
> This is currently not implemented, the "become partially translucent" part
> might be a bit tricky,
> anyway this part should be implemented as a separate patch (need some more
> input from Jon on this).

Any of the above sounds much more intrusive than your patch, so in this case it might be the design document which could use some patches ;)

Review of attachment 154329 [details] [review]:

Overall it looks quite good. Generally I like class names to be nouns, so DemandsAttention sounds a little weird to me. Maybe something like DemandsAttentionTracker would be better, though I don't really like that particular name either. If nobody can think of something better I'll just have to live with it ;)

Most of the comments stylistic and easy to fix with the exception of those regarding _createPreview().

::: js/ui/Makefile.am
@@ +34,3 @@
 	workspacesView.js	\
+	demandsAttention.js
+	workspace.js            \

The list is supposed to be sorted alphabetically.

::: js/ui/demandsAttention.js
@@ +30,3 @@
+
+    _handleStartup : function() {
+        let tracker = Shell.WindowTracker.get_default();

This is a signal handler, so the tracker object is passed as a parameter - if you didn't ignore it you would be able to save yourself the call to Shell.WindowTracker.get_default(). It's not an expensive operation, so doing it this way is perfectly fine - I'm just pointing out an alternative you might not be aware of.

@@ +39,3 @@
+
+    _sourceId : function(appId) {
+        return 'attention-' + appId.substr(0, appId.length - 8);

It's not obvious here that you strip the .desktop suffix from appId - you should add a comment if you do that. As long as appId identifies an application unambiguously it can be anything. I don't see how stripping a common suffix improves that, so I'd just leave it there - nobody's going to look at it anyways

@@ +53,3 @@
+            return _("%s has finished starting").format(app.get_name());
+        else
+            return _("%s is ready").format(window.title);

I don't agree with the assumption you're making about the application's reason to set the demands-attention hint. It might be to inform the user about a long lasting operation having finished, but then it might not. "XXX is ready" is a slightly inappropriate message if XXX wants to inform me that my computer is about to explode ...

(update: it appears that you actually discussed those messages with Jon - comment still applies, just don't act on it)

@@ +84,3 @@
+    },
+
+    _createPreview : function(source, window) {

This would make much more sense in Source IMHO - it's rather similar in functionality to createIcon, right?

@@ +106,3 @@
+
+        previewActor.set_child(box);
+        previewActor.connect('button-press-event', Lang.bind(this, function() {

You should rather connect to the clicked event

@@ +113,3 @@
+                                                                 }));
+        previewActor.connect('enter-event', Lang.bind(this, function() { Main.messageTray._pointerInTray = true; }));
+        previewActor.connect('leave-event', Lang.bind(this, function() { Main.messageTray._pointerInTray = false; }));

Writing private members of another class is pure evil, I'm sure you know that ;)

Seriously, I think I understand what you want to achieve, but don't do it that way. The easiest thing to do for now would be to make the preview non-reactive - it would suck right now, but if we make the whole notification clickable it will be magically fixed. Of course that would not address the underlying issue that adding reactive actors to notifications conflicts with the pop out/in logic in MessageTray - that's another bug and should probably be addressed there.

::: js/ui/main.js
@@ +42,3 @@
 let messageTray = null;
 let notificationDaemon = null;
+let demandsAttention = null;

demandsAttention is functionally equivalent to notificationDaemon, so I'd keep the two together

@@ +121,3 @@
     messageTray = new MessageTray.MessageTray();
     notificationDaemon = new NotificationDaemon.NotificationDaemon();
+    demandsAttention = new DemandsAttention.DemandsAttention();

Same as above

(In reply to comment #9)
> Review of attachment 154329 [details] [review]:
> 
> Overall it looks quite good. Generally I like class names to be nouns, so
> DemandsAttention sounds a little weird to me. Maybe something like
> DemandsAttentionTracker would be better, though I don't really like that
> particular name either. If nobody can think of something better I'll just have
> to live with it ;)

Thinking about it ... sounds odd to me too now.

I'd rather use "Handler" than "Tracker" (it does not only track them but perform actions), but I am open to other suggestions ;)

> Most of the comments stylistic and easy to fix with the exception of those
> regarding _createPreview().
> 
> ::: js/ui/Makefile.am
> @@ +34,3 @@
>      workspacesView.js    \
> +    demandsAttention.js
> +    workspace.js            \
> 
> The list is supposed to be sorted alphabetically.

OK

> ::: js/ui/demandsAttention.js
> @@ +30,3 @@
> +
> +    _handleStartup : function() {
> +        let tracker = Shell.WindowTracker.get_default();
> 
> This is a signal handler, so the tracker object is passed as a parameter - if
> you didn't ignore it you would be able to save yourself the call to
> Shell.WindowTracker.get_default(). It's not an expensive operation, so doing it
> this way is perfectly fine - I'm just pointing out an alternative you might not
> be aware of.

I indeed was not aware that I get a tracker object as a parameter here, will use that instead.

> @@ +39,3 @@
> +
> +    _sourceId : function(appId) {
> +        return 'attention-' + appId.substr(0, appId.length - 8);
> 
> It's not obvious here that you strip the .desktop suffix from appId - you
> should add a comment if you do that. As long as appId identifies an application
> unambiguously it can be anything. I don't see how stripping a common suffix
> improves that, so I'd just leave it there - nobody's going to look at it
> anyways

True, will remove this part.

> @@ +53,3 @@
> +            return _("%s has finished starting").format(app.get_name());
> +        else
> +            return _("%s is ready").format(window.title);
> 
> I don't agree with the assumption you're making about the application's reason
> to set the demands-attention hint. It might be to inform the user about a long
> lasting operation having finished, but then it might not. "XXX is ready" is a
> slightly inappropriate message if XXX wants to inform me that my computer is
> about to explode ...

Well it is ready to explode ;)

> (update: it appears that you actually discussed those messages with Jon -
> comment still applies, just don't act on it)

Yeah he didn't like "requests attention" or "demands attention" (even worse) so he suggested the "is ready" but sure this part can (and probably should) be improved.

> @@ +84,3 @@
> +    },
> +
> +    _createPreview : function(source, window) {
> 
> This would make much more sense in Source IMHO - it's rather similar in
> functionality to createIcon, right?

Yeah this makes sense.

> @@ +106,3 @@
> +
> +        previewActor.set_child(box);
> +        previewActor.connect('button-press-event', Lang.bind(this, function()
> {
> 
> You should rather connect to the clicked event

OK

> @@ +113,3 @@
> +                                                                 }));
> +        previewActor.connect('enter-event', Lang.bind(this, function() {
> Main.messageTray._pointerInTray = true; }));
> +        previewActor.connect('leave-event', Lang.bind(this, function() {
> Main.messageTray._pointerInTray = false; }));
> 
> Writing private members of another class is pure evil, I'm sure you know that
> ;)

Yeah I couldn't think of a better (less ugly) way to do it (only had ideas that are even worse), and still allow the user to click on the preview.

> Seriously, I think I understand what you want to achieve, but don't do it that
> way. The easiest thing to do for now would be to make the preview non-reactive
> - it would suck right now, but if we make the whole notification clickable it
> will be magically fixed. Of course that would not address the underlying issue
> that adding reactive actors to notifications conflicts with the pop out/in
> logic in MessageTray - that's another bug and should probably be addressed
> there.

Yeah making the whole notification clickable sounds like a good solution to me (and avoids the icon hunting in other cases too).

Should do this as a separate patch ... 

> ::: js/ui/main.js
> @@ +42,3 @@
>  let messageTray = null;
>  let notificationDaemon = null;
> +let demandsAttention = null;
> 
> demandsAttention is functionally equivalent to notificationDaemon, so I'd keep
> the two together

Sorry I don't get this part ... what do you mean by "keep them together" ?

(In reply to comment #10)
> Sorry I don't get this part ... what do you mean by "keep them together" ?

Oh, I just meant to put the messageTray = ... line before or after the other two:

notificationDaemon = new NotificationDaemon.NotificationDaemon();
demandsAttention = new DemandsAttention.DemandsAttention();
messageTray = new MessageTray.MessageTray();

Not that important obviously...

Created attachment 154370 [details] [review]
Add demands attention support to the messagetray

Fixed up patch (depends on the patch in bug 610670 now)

Created attachment 154372 [details] [review]
Add demands attention support to the messagetray

Upload the correct patch

Created attachment 154378 [details] [review]
Add demands attention support to the messagetray

Cleanups.

Created attachment 154380 [details] [review]
Add demands attention support to the messagetray

Used 'clicked' rather than 'button-pressed'

Created attachment 154392 [details] [review]
Add demands attention support to the messagetray

One more cleanup (avoid code duplication)

Review of attachment 154392 [details] [review]:

OK, still a bunch of comments, but no worry - all of them are harmless :)

There is one issue with the preview image moving when the window title updates - the changes I propose in _createPreview are unfortunately only stylistic, so no idea how to fix that properly :(

::: js/ui/demandsAttentionHandler.js
@@ +1,3 @@
+
+const GLib = imports.gi.GLib;
+/* -*- mode: js2; js2-basic-offset: 4; indent-tabs-mode: nil -*- */

unused

@@ +5,3 @@
+const Lang = imports.lang;
+const GLib = imports.gi.GLib;
+/* -*- mode: js2; js2-basic-offset: 4; indent-tabs-mode: nil -*- */

unused

@@ +29,3 @@
+    },
+
+    _handleStartup : function(tracker) {

Instead of _handleStartup I'd propose either _onStartupSequenceChanged (which matches our style for signal handlers) or _updateStartupIds (which matches what the function does) (obviously in the latter case there are other names just as acceptable)

@@ +38,3 @@
+
+    _sourceId : function(appId) {
+        return 'attention-' + appId;

See - this looks so clean _nobody_ is ever going to look into it to find out that we return attention-gnome-terminal.desktop here ;)

@@ +55,3 @@
+    },
+
+    _handleAttention : function(display, window) {

Pretty much the same as the _handleStartup comment - _onWindowDemandsAttention or ... mmh,  _showNotificationForWindowDemandingAttention would match the functionality but makes a crappy function name

@@ +56,3 @@
+
+    _handleAttention : function(display, window) {
+        if (!window || window.has_focus() || window.get_window_type() == Meta.WindowType.UTILITY)

I'm not 100% sure about the gimp-hack here - it certainly fixes gimp, but it might break other obscure applications (obscure because IMHO both _not_ setting the skip-taskbar hint on utility windows and setting the urgency hint on a skip-taskbar window are corner cases - especially as few applications actually set the type hint).
If such a case shows up, we'll have to do more complicated stuff, like checking whether the utility window belongs to the currently focussed app - this is ugly IMO, so we should not do that now, but a comment along the lines of
// we check for the UTILITY type hint to handle gimp setting the demands-attention hint on
// its utility window while focusing the main window
would probably help tracking it down as it points out that the check is actually handling something rather obscure. Note that I wrote the comment without investigation, so if you think that it does not describe correctly what's happening you are probably right and should correct it ;)

@@ +112,3 @@
+        let scale = Math.min(1.0, THUMBNAIL_SIZE / width, THUMBNAIL_SIZE / height);
+        let clone = new Clutter.Clone({ source: windowTexture,
+                                        reactive: true,

You don't need the reactive here (events are handled by previewActor)

@@ +116,3 @@
+                                        height: height * scale });
+
+        let previewActor = new St.Clickable({ name: "thumbnail",

There is a convention of using double quotes for translatable strings (there are a lot of places where we break this rule, but we should try not to ;) )

@@ +125,3 @@
+        let bin = new St.Bin({ name: "thumbnail" });
+        bin.add_actor(clone);
+        box.add_actor(bin);

I think you can make this a whole lot more compact using:

let clone = ...
let previewActor = new St.Clickable({ name: 'thumbnail',
                                      child: bin,
                                      reactive: true });
previewActor.connect('clicked', ...);

return previewActor;

(In reply to comment #17)
> @@ +5,3 @@
> +const Lang = imports.lang;
> +const GLib = imports.gi.GLib;
> +/* -*- mode: js2; js2-basic-offset: 4; indent-tabs-mode: nil -*- */
> 
> unused

Mmmh, I think I ran into a splinter bug here - the second "unused" refers to the const Mainloop line ...

(In reply to comment #17)
> Review of attachment 154392 [details] [review]:
> 
> OK, still a bunch of comments, but no worry - all of them are harmless :)

OK

> There is one issue with the preview image moving when the window title updates
> - the changes I propose in _createPreview are unfortunately only stylistic, so
> no idea how to fix that properly :(

Well the issue is that we clear the notification on update but we shouldn't have to do that. (i.e like I did in the first patch)

Notification.update ought to just update the text on the label not add new ones (see your first screenshot), in this case we wouldn't have to re add the previewActor which is causing this problem.

> ::: js/ui/demandsAttentionHandler.js
> @@ +1,3 @@
> +
> +const GLib = imports.gi.GLib;
> +/* -*- mode: js2; js2-basic-offset: 4; indent-tabs-mode: nil -*- */
> 
> unused
> 
> @@ +5,3 @@
> +const Lang = imports.lang;
> +const GLib = imports.gi.GLib;
> +/* -*- mode: js2; js2-basic-offset: 4; indent-tabs-mode: nil -*- */
> 
> unused

OK, removed.

> @@ +29,3 @@
> +    },
> +
> +    _handleStartup : function(tracker) {
> 
> Instead of _handleStartup I'd propose either _onStartupSequenceChanged (which
> matches our style for signal handlers) or _updateStartupIds (which matches what
> the function does) (obviously in the latter case there are other names just as
> acceptable)

OK

> @@ +38,3 @@
> +
> +    _sourceId : function(appId) {
> +        return 'attention-' + appId;
> 
> See - this looks so clean _nobody_ is ever going to look into it to find out
> that we return attention-gnome-terminal.desktop here ;)

;)

> @@ +55,3 @@
> +    },
> +
> +    _handleAttention : function(display, window) {
> 
> Pretty much the same as the _handleStartup comment - _onWindowDemandsAttention
> or ... mmh,  _showNotificationForWindowDemandingAttention would match the
> functionality but makes a crappy function name

OK

> @@ +56,3 @@
> +
> +    _handleAttention : function(display, window) {
> +        if (!window || window.has_focus() || window.get_window_type() ==
> Meta.WindowType.UTILITY)
> 
> I'm not 100% sure about the gimp-hack here - it certainly fixes gimp, but it
> might break other obscure applications (obscure because IMHO both _not_ setting
> the skip-taskbar hint on utility windows and setting the urgency hint on a
> skip-taskbar window are corner cases - especially as few applications actually
> set the type hint).
> If such a case shows up, we'll have to do more complicated stuff, like checking
> whether the utility window belongs to the currently focussed app - this is ugly
> IMO, so we should not do that now, but a comment along the lines of
> // we check for the UTILITY type hint to handle gimp setting the
> demands-attention hint on
> // its utility window while focusing the main window
> would probably help tracking it down as it points out that the check is
> actually handling something rather obscure. Note that I wrote the comment
> without investigation, so if you think that it does not describe correctly
> what's happening you are probably right and should correct it ;)

OK added a comment describing why this is needed.

> @@ +112,3 @@
> +        let scale = Math.min(1.0, THUMBNAIL_SIZE / width, THUMBNAIL_SIZE /
> height);
> +        let clone = new Clutter.Clone({ source: windowTexture,
> +                                        reactive: true,
> 
> You don't need the reactive here (events are handled by previewActor)

OK

> @@ +116,3 @@
> +                                        height: height * scale });
> +
> +        let previewActor = new St.Clickable({ name: "thumbnail",
> 
> There is a convention of using double quotes for translatable strings (there
> are a lot of places where we break this rule, but we should try not to ;) )

OK

> @@ +125,3 @@
> +        let bin = new St.Bin({ name: "thumbnail" });
> +        bin.add_actor(clone);
> +        box.add_actor(bin);
> 
> I think you can make this a whole lot more compact using:
> 
> let clone = ...
> let previewActor = new St.Clickable({ name: 'thumbnail',
>                                       child: bin,
>                                       reactive: true });
> previewActor.connect('clicked', ...);
> 
> return previewActor;

OK

(In reply to comment #18)
> (In reply to comment #17)
> > @@ +5,3 @@
> > +const Lang = imports.lang;
> > +const GLib = imports.gi.GLib;
> > +/* -*- mode: js2; js2-basic-offset: 4; indent-tabs-mode: nil -*- */
> > 
> > unused
> 
> Mmmh, I think I ran into a splinter bug here - the second "unused" refers to
> the const Mainloop line ...

OK

Created attachment 154405 [details] [review]
Add demands attention support to the messagetray

Fixed up patch.

(In reply to comment #19)
> Well the issue is that we clear the notification on update but we shouldn't
> have to do that. (i.e like I did in the first patch)
> 
> Notification.update ought to just update the text on the label not add new ones

indeed, it even has documentation that states that that's exactly what it does. So if it doesn't, then there's a bug in Notification.update.

I agree with most of Florian's comments. One additional thing is that at least for the "%s is ready" case, you really need quotes around the %s, because the window title could be totally random. (eg, if a thunderbird composer window for a message with the subject "this is a test" were to demand attention, your current code would notify the user "Write: this is a test is ready".)

(I also agree that "is ready" probably won't work semantically for a lot of cases, but we can think about that again after seeing it in the wild.)

For the gimp thing: figure out what rule metacity uses, and do the same thing.

Finally, I think you should remove the window previews (and the associated problems, including the dependency on 610670). After we get the basic code in, we can consider whether or not to add the previews separately.

(In reply to comment #21)
> (In reply to comment #19)
> > Well the issue is that we clear the notification on update but we shouldn't
> > have to do that. (i.e like I did in the first patch)
> > 
> > Notification.update ought to just update the text on the label not add new ones
> 
> indeed, it even has documentation that states that that's exactly what it does.
> So if it doesn't, then there's a bug in Notification.update.

OK

> I agree with most of Florian's comments. One additional thing is that at least
> for the "%s is ready" case, you really need quotes around the %s, because the
> window title could be totally random. (eg, if a thunderbird composer window for
> a message with the subject "this is a test" were to demand attention, your
> current code would notify the user "Write: this is a test is ready".)

Yeah that makes sense.

> (I also agree that "is ready" probably won't work semantically for a lot of
> cases, but we can think about that again after seeing it in the wild.)

I don't disagree here ...

> For the gimp thing: figure out what rule metacity uses, and do the same thing.

It does pretty much the same thing, utility windows aren't shown in the taskbar at all and therefore they don't "blink" when the attention hint is set.

> Finally, I think you should remove the window previews (and the associated
> problems, including the dependency on 610670). After we get the basic code in,
> we can consider whether or not to add the previews separately.

OK

Created attachment 154410 [details] [review]
Add demands attention support to the messagetray

Updated patch:

*) Don't clear the notification just update it
*) Removed previews
*) Add quotes around %s for the "is ready" case
*) Leave GIMP special case as is (as it is the same that metacity + gnome-panel does)

Comment on attachment 154410 [details] [review]
Add demands attention support to the messagetray

>+        /*
>+         * We don't want to show the notification when the window is already focused,
>+         * because this is rather pointless.
>+         * Some apps (like GIMP) do stupid things like setting the urgency hint on the
>+         * toolbar windows which would result into a notification even though GIMP itself is
>+         * focused.
>+         * We are just ignoring the hint on utility windows for now.
>+         */
>+        if (!window || window.has_focus() || window.get_window_type() == Meta.WindowType.UTILITY)

1) use //, not /*

2) please refrain from calling other apps stupid, unless they *really* deserve it. :)

3) since as you said:

    <drago01> (they aren't shown in the taskbar at all and therefore cannot "blink")

the right way to catch apps that are unintentionally expecting metacity-like behavior is to check window.is_skip_taskbar(), not window.get_window_type()

>+        window.connect('notify::title', Lang.bind(this, function(win) {
>+                                                            notification.update(this._getTitle(app, win), this._getBanner(app, win), false);

this needs a patch to Notification.update too doesn't it?

(or at least a bug report...)

Comment on attachment 154410 [details] [review]
Add demands attention support to the messagetray

oops, forgot to set status. ok to commit with those fixes.

Created attachment 154424 [details] [review]
Don't set the bannerText twice on Notification.update

This should fix the duplicate banner issue.

Comment on attachment 154424 [details] [review]
Don't set the bannerText twice on Notification.update

This is broken...

Comment on attachment 154410 [details] [review]
Add demands attention support to the messagetray

Pushed as 059504c, with the changes from the last comment.