Created attachment 151258 [details] [review]
Kill notification-daemon at startup when running in --replace mode

Created attachment 151259 [details] [review]
Line-wrap the notification content

Doesn't quite work for very long notifications yet, because the
message tray itself has a fixed height.

Review of attachment 151258 [details] [review]:

::: js/ui/notificationDaemon.js
@@ +84,3 @@
+            // than killall, but on Linux at least it won't match if you
+            // pass more than 15 characters of the process name.
+            let p = new Shell.Process({'args' : ['pkill', 'notification-da']});

Could you also kill notify-osd if running? Ubutunu users need that I guess.

Review of attachment 151258 [details] [review]:

Looks mostly fine - as always, comments below:

::: js/ui/notificationDaemon.js
@@ +84,3 @@
+            // than killall, but on Linux at least it won't match if you
+            // pass more than 15 characters of the process name.
+            let p = new Shell.Process({'args' : ['pkill', 'notification-da']});

I don't like how we rely on the 15 character limitation you describe above to not match e.g. gdu-notification-daemon - especially as I have no idea which limits other systems do impose.

Assuming the -f option is portable, I'd propose something along the lines of "pkill -f '/notification-daemon$'" or "pkill -f '/notification-daemon($| )'".

Taking Milan's comment into consideration, we could make this "pkill -f '/(notify-osd|notification-daemon)($| )'

::: src/gnome-shell.in
@@ +359,3 @@
         xephyr = start_xephyr()
         # This makes us not grab the org.gnome.Panel name
+        os.environ['GNOME_SHELL_NO_REPLACE'] = '1'

As you extend the meaning of the variable, could you update the comment above?

Created attachment 151273 [details] [review]
Kill notification-daemon at startup when running in --replace mode

pkill -f appears to be supported on both Solaris and FreeBSD, and makes
Linux magically able to match more of the command name for whatever reason,
so OK. Updated patch uses

    pkill -f '^([^ ]*/)?(notification-daemon|notify-osd)$'

which is supposed to match "notification-daemon" and
"/usr/libexec/notification-daemon", but not "notification-daemon --something"
or "gdb /usr/libexec/notification-daemon"

(also fixed the comment Florian commented on)

Review of attachment 151273 [details] [review]:

Thnks for the update! Looks fine to me ...

::: js/ui/notificationDaemon.js
@@ +87,3 @@
+            // command line, it will work, but we have to be careful
+            // in that case that we don't match "gedit
+            // notification-daemon.c" or whatever...

Good catch about gedit notification-daemon.c!

Created attachment 151375 [details] [review]
[MessageTray] Deal better with long notifications

Line-wrap notifications that are too long, and implement pop-out
on mouseover if they end up too large for the message tray

oh, still needs some way to indicate explicitly that the notification extends off the bottom of the screen. You can't do both ellipsizing and line-wrap, so if we wanted ellipsizing we'd have to limit the notification to a single line rather than the two we get now. (Or we'd have to do some clever hacking to make it work..)

Another possibility would be to have the notification fade out toward the bottom?

Review of attachment 151375 [details] [review]:

Looks good overall.

I think we should call popOut() on notification in _onMessageTrayEntered() instead of monitoring for an enter event on just the notification area. It seems difficult to have to manoeuvre to a specific area at the bottom of the screen rather than to just anywhere at the bottom of the screen. This especially makes sense since you only need to move your mouse to anywhere over the message tray to keep the notification showing.

I also think we should show a single ellipsized line first and then show the full text on pop out. On my machine, the second line in the notification is only partially visible, so there isn't much value in it. This will require doing the full this._textBox height calculations separately and changing the wrapping when we are in the popped out mode.

::: js/ui/messageTray.js
@@ +37,3 @@
     _init: function() {
+        this.actor = new Clutter.Group();
+        this.actor.connect('allocation-changed', Lang.bind(this, this._allocated));

The documentation suggests just listening to the notifications about the ::allocation property if your are not interested in the flags.

http://www.clutter-project.org/docs/clutter/stable/ClutterActor.html#ClutterActor-allocation-changed

Not sure is that makes any difference, but connecting to 'notify::allocation' is more common in our code too.

@@ +45,3 @@
+        this.actor.add_actor(this._box);
+        this._boxEventHandler = this._box.connect('captured-event',
+                                                  Lang.bind(this, this._boxCapturedEvent));

Should be a more function-like name. Perhaps, this._boxCapturedEventHandler

Though this callback is not necessary if we change the behavior to popping out when the mouse moves anywhere over the message tray.

@@ +62,3 @@
         this._text = new St.Label();
+        this._text.clutter_text.line_wrap = true;
+        this._text.clutter_text.ellipsize = Pango.EllipsizeMode.NONE;

This will change if we make it a single ellipsized line when not popped out.

@@ +79,3 @@
+        let [min, nat] = this._text.get_preferred_width(forHeight);
+
+        alloc.min_size = alloc.nat_size = Math.min(nat, global.screen_width / 2);

Should we use Math.floor here to make sure we end up with an integer number?

@@ +83,3 @@
+
+    _textBoxGetPreferredHeight: function (actor, forWidth, alloc) {
+        // St.BoxLayout passes -1 for @forWidth, which isn't what we want.

Do you know why this is happening? Is it a bug?

(In reply to comment #9)
> I think we should call popOut() on notification in _onMessageTrayEntered()
> instead of monitoring for an enter event on just the notification area.

ah... yeah. I just copied code from the sidebar without thinking about the fact that we were already tracking the pointer in the tray...

> I also think we should show a single ellipsized line first and then show the
> full text on pop out. On my machine, the second line in the notification is
> only partially visible, so there isn't much value in it.

Well... it makes it obvious that the message is being truncated. Ellipsization is ambiguous, since it's possible that the other person just sent you an IM that ended in ellipses too...

Jon filed another bug about long notifications, which I'm going to dup to this one, and I won't be updating the patch until he responds to that.

> +        alloc.min_size = alloc.nat_size = Math.min(nat, global.screen_width /
> 2);
> 
> Should we use Math.floor here to make sure we end up with an integer number?

I thought about that, but any real screen width would be an even number of pixels, and if you're doing virtual/xinerama/whatever things involving odd screen widths, you're probably *trying* to lose.

> +    _textBoxGetPreferredHeight: function (actor, forWidth, alloc) {
> +        // St.BoxLayout passes -1 for @forWidth, which isn't what we want.
> 
> Do you know why this is happening? Is it a bug?

It's intentional. Even though you're calling

    someBoxLayout.get_preferred_height(100)

it doesn't make sense for it to pass that 100 on to its children, since it's going to split that width among them. But it hasn't figured out its layout yet, so it doesn't know what width to pass each child. So it just doesn't bother and passes -1.

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

Another comment for the review - we need to do this._notificationBin.hide() instead of this._notificationBox.actor.hide() in _init() and _hideComplete() and this._notificationBin.show() instead of this._notificationBox.actor.show() in showNotification() so that this._notificationBin does not take up the space in the message tray when it is in the summary mode.

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

Created attachment 152739 [details] [review]
[MessageTray] fix up allocation, ellipsize long notifications

This handles ellipsization of long notifications, and forces a maximum
notification width of 1/3 screen_width. That can be adjust later, but
I picked 1/3 because the latest mockups involve sometimes showing
notifications in the center of the screen, and sometimes over the summary
area, so 1/3 seemed to make some sense, since it would keep them out of
each other's way...

Created attachment 152742 [details] [review]
[MessageTray] do pop-out on hover for long notifications

This changes the structure of notifications a bit, and implements pop-out.
Notifications are now:

  ICON  Title  Summary...

        Body body body
        body body body

But the "Summary" part is hidden when the body is visible.

For notifications from notificationDaemon.js, "Title" is the summary/title,
and "Summary" is the body, with newlines converted to spaces. If the
summary gets ellipsized, then the summary will also be duplicated to the
body, so you can pop it out to see the whole text. This seems to mostly
duplicate the functinoality of the latest mockups

Created attachment 152744 [details]
sample code from ebassi to do a fade instead of ellipsization

if we'd rather do fading than ellipsizing, here's some code Emmanuele wrote showing how we could do that

Review of attachment 152739 [details] [review]:

Looks fine, with the exception of two changes in behavior noted below.

::: js/ui/messageTray.js
@@ +112,3 @@
                                         reactive: true });
 
+        this._notificationBin = new St.Bin();

Adding this._notificationBin has somehow changed the way positioning works in _showNotification() and _hideNotification(), so now the notification doesn't slide down when _hideNotification() is called. Only its opacity is changing instead. You can see that if you readjust Tweener.slowDownFactor Also, when there are multiple notifications queued, the animation of one sliding out and another one sliding in no longer works, so it's only the opacity that is changing, with one notification fading out and another one fading in.

@@ +121,1 @@
+        this._summaryBin = new St.BoxLayout();

Should this be renamed to this._summaryBox since it's no longer a Bin?

@@ +153,3 @@
+        this._notificationBin.width = third;
+        this._summaryBin.x = this.actor.width - third;
+        this._summaryBin.width = third;

This seems to explicitly say that the summary can take up only up to the third of the screen width. What happens right now is that if there are too many icons, the older ones are being pushed out from the screen. They reappear if you close off some of the visible icons. We need a ui design input on what should happen if there are more icons than can fit in 1/3 of the screen at normal size.

I wonder if we need to do the hiding and showing of this._notificationBox.actor any longer given that it should be sliding off-screen when it is not supposed to be visible. Also, perhaps we need to replace it with hiding and showing of this._notificationBin.

Just a quick comment that with the second patch, the problem of notifications not sliding in/out goes away.

OK, this is totally a weird case but still.  The following test overflows the expanded view width:
notify-send "This is a summary" "this is a message body and hopefully it will be pretty long aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa "

Other than that is seem pretty good except the padding at the top between the icon/text and the top of the banner looks a little tight.

Review of attachment 152742 [details] [review]:

Looks good and should be ready to commit with a few minor changes. Perhaps you can move the changes in hiding of this._notificationBin and the changes in _showNotification() and _hideNotification() to the first patch and then you can commit both patches.

The disappearance of the banner text in the top line when popping out is a little weird. Makes me feel like something got scrolled up and I might be now missing it. It then takes a moment to realize it's now on the line below. Maybe we can change the animation to make it feel like the banner text is moving down instead of up, make the banner text disappearance quicker, or remove the animation for it completely.

::: js/ui/messageTray.js
@@ +36,3 @@
+// Creates a notification. In banner mode, it will show
+// @icon, @title (in bold) and @banner, all on a single line
+// (with @banner ellipsized if necessary). If @body is not %null, then

than

@@ +459,3 @@
         this._notificationBox.setContent(this._notificationQueue.shift());
 
+        let notification = this._notificationBin;

I'm not sure this reassignment serves any purpose anymore. Might be cleaner to just use this._notificationBin throughout _showNotification() and _hideNotification()

@@ +463,3 @@
         notification.y = this.actor.height;
         notification.show();
         let futureY = this.actor.height - notification.height;

This line was a left-over from debugging in my previous commit. Could you please remove it since you've modified the surrounding lines.

(In reply to comment #17)
> Adding this._notificationBin has somehow changed the way positioning works in
> _showNotification() and _hideNotification(), so now the notification doesn't
> slide down when _hideNotification() is called.

fixed

> +        this._summaryBin = new St.BoxLayout();
> 
> Should this be renamed to this._summaryBox since it's no longer a Bin?

eh. i like the naming consistency between notificationBin and summaryBin.

> This seems to explicitly say that the summary can take up only up to the third
> of the screen width. What happens right now is that if there are too many
> icons, the older ones are being pushed out from the screen.

Of course, this would happen when they reached the full width of the screen no matter what we did...

(In reply to comment #20)
> The disappearance of the banner text in the top line when popping out is a
> little weird.

Not changed now, but of course that can be changed later.

> +// (with @banner ellipsized if necessary). If @body is not %null, then
> 
> than

Uh... no :-)

> +        let notification = this._notificationBin;
> 
> I'm not sure this reassignment serves any purpose anymore.

agreed. removed.

>          let futureY = this.actor.height - notification.height;
> 
> This line was a left-over from debugging in my previous commit. 

removed

(In reply to comment #19)
> OK, this is totally a weird case but still.  The following test overflows the
> expanded view width:
> notify-send "This is a summary" "this is a message body and hopefully it will
> be pretty long
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
> "

Not yet fixed.

> Other than that is seem pretty good except the padding at the top between the
> icon/text and the top of the banner looks a little tight.

Feel free to commit a fix to the CSS.

Attachment 152739 [details] pushed as 3c9d0fb - [MessageTray] fix up allocation, ellipsize long notifications
Attachment 152742 [details] pushed as fa60165 - [MessageTray] do pop-out on hover for long notifications

leaving open for the line-breaks-on-very-long-words problem

(In reply to comment #19)
> OK, this is totally a weird case but still.  The following test overflows the
> expanded view width:
> notify-send "This is a summary" "this is a message body and hopefully it will
> be pretty long
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
> "

It seems like this should be fixable by just changing the clutter_text's wrap_mode to Pango.WrapMode.WORD_CHAR, but that doesn't work. Still shouldn't be too hard... Assuming WORD_CHAR isn't just failing for some stupid reason, we can fix it by forcing a reallocation with WrapMode.CHAR if WrapMode.WORD would overflow.

Created attachment 153664 [details] [review]
[MessageTray] fix notifications with short banners and action buttons

Previously the banner was only promoted to the body if it got
truncated, but the banner was *always* hidden when expanding the
notification. This meant a message with a short banner, plus
notification buttons, would have no banner text visible when it was
expanded. Fix that.

Created attachment 153665 [details] [review]
[St] Implement max-width/max-height in the CSS parser

st_theme_node_adjust_preferred_width/height now limit the content area
of an actor to the max, if given. (The requested width/height may be
larger to make room for borders, etc.)

Created attachment 153666 [details] [review]
[MessageTray] notification size/wrapping fixes

Make the font match the panel.
Make the max width be em-based rather than screen-size-based.
Fix it to break lines mid-word if needed.

First patch is a sort-of-unrelated bug Marina noticed the other day that I noticed again while working on this.

The St patch adds max-width/max-height support. I mentioned on IRC having problems that were possibly St.Table's fault, but these went away when I made st_theme_node_adjust_for_height/st_theme_node_adjust_for_width also adjust their arguments. (Originally I was just doing st_theme_node_adjust_preferred_height/st_theme_node_adjust_preferred_width.)

Third patch incorporates multiple size and wrapping fixes. "Make the font match the panel" is something Jon said in bug 609765. "Make the max width be em-based" is from bug 607025, which was duped to this. "Fix it to break lines mid-word" is the problem Jon noted above, which I was on the right track on before, I just had the property name wrong :)

Review of attachment 153665 [details] [review]:

Mostly looks perfect, except for the adjustment of for_width, for_height

::: src/st/st-theme-node.c
@@ +2281,3 @@
+
+      if (node->max_height != -1)
+        *for_height = MIN (*for_height, node->max_height);

Hmm, this doesn't make sense to me - unless we actually limit the height when allocated, then we're going to get a requested height for a narrower text wrap than we actually do.

And if we tried to do adjust allocation in get_content_box() we'd hit the problem that we don't know about alignment there; maybe get_context_box() actually needs to take alignment as parameters. Ugh.

Created attachment 153723 [details] [review]
[St] Implement max-width/max-height in the CSS parser

removed incorrect adjustment of for_width/for_height. These patches now
depend on bug 609848. (without the patches there, this will allocate too
little height to the notification when it is expanded)

Review of attachment 153664 [details] [review]:

Looks good and works as expected. There is just one minor comment.

::: js/ui/messageTray.js
@@ +113,3 @@
 
+        if (this._bannerBody)
+            this._bannerBodyText = banner;

Should set this._bannerBodyText to null in _init.

Review of attachment 153666 [details] [review]:

Looks good in combination with the other patches that enforce max-width. (This patch should not be committed before them because it alone results in the notifications with width that goes up to the screen width.)

Comment on attachment 153664 [details] [review]
[MessageTray] fix notifications with short banners and action buttons

Attachment 153664 [details] pushed as 090335a - [MessageTray] fix notifications with short banners and action buttons

Attachment 153666 [details] pushed as 94f3203 - [MessageTray] notification size/wrapping fixes
Attachment 153723 [details] pushed as 721e1ea - [St] Implement max-width/max-height in the CSS parser