After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 610594 - Add demands attention support to the messagetray
Add demands attention support to the messagetray
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 596431 607930 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2010-02-21 10:52 UTC by drago01
Modified: 2010-02-22 19:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add demands attention support to the messagetray (7.56 KB, patch)
2010-02-21 10:53 UTC, drago01
none Details | Review
Broken window title update (45.83 KB, image/png)
2010-02-21 15:56 UTC, Florian Müllner
  Details
Add demands attention support to the messagetray (7.98 KB, patch)
2010-02-21 16:40 UTC, drago01
none Details | Review
Add demands attention support to the messagetray (8.01 KB, patch)
2010-02-21 16:43 UTC, drago01
reviewed Details | Review
Add demands attention support to the messagetray (1.60 KB, patch)
2010-02-22 10:44 UTC, drago01
none Details | Review
Add demands attention support to the messagetray (7.79 KB, patch)
2010-02-22 10:58 UTC, drago01
none Details | Review
Add demands attention support to the messagetray (7.76 KB, patch)
2010-02-22 11:39 UTC, drago01
none Details | Review
Add demands attention support to the messagetray (7.70 KB, patch)
2010-02-22 11:43 UTC, drago01
none Details | Review
Add demands attention support to the messagetray (7.41 KB, patch)
2010-02-22 13:30 UTC, drago01
reviewed Details | Review
Add demands attention support to the messagetray (7.63 KB, patch)
2010-02-22 15:36 UTC, drago01
none Details | Review
Add demands attention support to the messagetray (6.38 KB, patch)
2010-02-22 16:44 UTC, drago01
committed Details | Review
Don't set the bannerText twice on Notification.update (981 bytes, patch)
2010-02-22 18:40 UTC, drago01
needs-work Details | Review

Description drago01 2010-02-21 10:52:50 UTC
Add demands attention support to the messagetray

It is based on Jon Nettleton's "window attention" extension.
(http://mail.gnome.org/archives/gnome-shell-list/2010-February/msg00019.html)

The main differences are:

*) Don't show the workspace name (don't mention the workspace at all) in the notifications (we might want to reconsider this but that is what Jon (mccann) agreed on IRC)
*) Differentiate between startup and windows of already existing apps:
        -) Show "$App has finished starting" for the former and "$windowtitle is ready" for the later
        -) The former only happens when the newly started up is denieded focus and the window gets the demands attention hint set by the wm (i.e it is not a general startup notification indicator)
*) The notification source is removed when the user opens the window (or close it)

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.

The design document states:

More elaborate solutions may be used if necessary. For example: when an application
window pops open behind an active window due to focus-stealing prevention the active
window should momentarily become partially translucent and the new window could appear
underneath. A subtle sound effect could be played. When an application window pops
open on an inactive workspace it could appear as a ghosted image of the actual window
and point in the direction of the workspace it opened on.

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).
Comment 1 drago01 2010-02-21 10:53:27 UTC
Created attachment 154312 [details] [review]
Add demands attention support to the messagetray
Comment 2 Dan Winship 2010-02-21 13:38:31 UTC
*** Bug 596431 has been marked as a duplicate of this bug. ***
Comment 3 drago01 2010-02-21 14:51:29 UTC
*** Bug 607930 has been marked as a duplicate of this bug. ***
Comment 4 Florian Müllner 2010-02-21 15:56:01 UTC
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.
Comment 5 drago01 2010-02-21 16:04:39 UTC
(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.
Comment 6 drago01 2010-02-21 16:40:10 UTC
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.
Comment 7 drago01 2010-02-21 16:43:21 UTC
Created attachment 154329 [details] [review]
Add demands attention support to the messagetray

Upload the correct version.
Comment 8 Florian Müllner 2010-02-21 21:08:16 UTC
(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 ;)
Comment 9 Florian Müllner 2010-02-21 21:17:11 UTC
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
Comment 10 drago01 2010-02-21 21:31:38 UTC
(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" ?
Comment 11 Florian Müllner 2010-02-21 21:55:12 UTC
(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...
Comment 12 drago01 2010-02-22 10:44:00 UTC
Created attachment 154370 [details] [review]
Add demands attention support to the messagetray

Fixed up patch (depends on the patch in bug 610670 now)
Comment 13 drago01 2010-02-22 10:58:49 UTC
Created attachment 154372 [details] [review]
Add demands attention support to the messagetray

Upload the correct patch
Comment 14 drago01 2010-02-22 11:39:45 UTC
Created attachment 154378 [details] [review]
Add demands attention support to the messagetray

Cleanups.
Comment 15 drago01 2010-02-22 11:43:28 UTC
Created attachment 154380 [details] [review]
Add demands attention support to the messagetray

Used 'clicked' rather than 'button-pressed'
Comment 16 drago01 2010-02-22 13:30:40 UTC
Created attachment 154392 [details] [review]
Add demands attention support to the messagetray

One more cleanup (avoid code duplication)
Comment 17 Florian Müllner 2010-02-22 14:53:57 UTC
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;
Comment 18 Florian Müllner 2010-02-22 14:56:12 UTC
(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 ...
Comment 19 drago01 2010-02-22 15:35:43 UTC
(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
Comment 20 drago01 2010-02-22 15:36:07 UTC
Created attachment 154405 [details] [review]
Add demands attention support to the messagetray

Fixed up patch.
Comment 21 Dan Winship 2010-02-22 16:03:34 UTC
(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.
Comment 22 drago01 2010-02-22 16:34:48 UTC
(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
Comment 23 drago01 2010-02-22 16:44:08 UTC
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 24 Dan Winship 2010-02-22 17:46:00 UTC
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 25 Dan Winship 2010-02-22 17:46:21 UTC
Comment on attachment 154410 [details] [review]
Add demands attention support to the messagetray

oops, forgot to set status. ok to commit with those fixes.
Comment 26 drago01 2010-02-22 18:40:08 UTC
Created attachment 154424 [details] [review]
Don't set the bannerText twice on Notification.update

This should fix the duplicate banner issue.
Comment 27 drago01 2010-02-22 18:43:22 UTC
Comment on attachment 154424 [details] [review]
Don't set the bannerText twice on Notification.update

This is broken...
Comment 28 drago01 2010-02-22 19:10:44 UTC
Comment on attachment 154410 [details] [review]
Add demands attention support to the messagetray

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