Created attachment 154434 [details]
Notification Popup Mockup

Original attachment missing.

Created attachment 156212 [details] [review]
[MessageTray] fix so you don't see the bottom rounded corners on pop-out

the problem is only barely noticeable with ordinary notifications, but
we'll be using the same hack to add extra height to the summary area
notifications so that they can extend through the tray area to the
bottom of the screen

Created attachment 156213 [details] [review]
[StClickable] return FALSE, not TRUE from enter/leave event handlers

This lets the clickable's parents also process the event. In
particular, this fixes the tracking of "hover" when you leave one
StClickable by passing through one of its StClickable children.

--

pretty sure this is correct. I couldn't find anything about why it was
returning TRUE rather than FALSE before

Created attachment 156214 [details] [review]
[MessageTray] Use St.Clickable rather than tracking enter/leave events

St.Clickable's "hover" property takes reactive children into account
when deciding whether or not the pointer has actually left the actor.
Rather than duplicating that logic here, just rewrite it to use
St.Clickable.

Changing MessageTray.actor from an St.BoxLayout to an St.Bin subclass
requires tweaking the allocation of notificationBin and summaryBin
slightly; The new system uses anchor_gravity to keep them aligned
correctly.

Created attachment 156215 [details] [review]
[MessageTray] pop out the last notification when mousing over the summary

Review of attachment 156213 [details] [review]:

In general, I would expect enter/leave handlers to return FALSE rather than true in most cases - we are observing the event rather than acting on the event. But its a little uncomfortable if StClickable breaks if any of the children get the return value wrong. Would it work to track the hover state in captured-event?

Review of attachment 156212 [details] [review]:

I'm not too comfortable with that change, especially as it restricts the bottom padding to the left padding (the comment in the CSS is not very clear about that). Not sure if non-uniform border-radii are still part of bug 607500, but that seems like the way to go (also see .panel-button) - meanwhile you could use something like Math.max(radius[St.Corner.BOTTOM_LEFT], radius[St.Corner.BOTTOM_RIGHT]) in the height calculation.

Review of attachment 156214 [details] [review]:

The patch breaks notification actions, probably due to general St.Button brokenness - if you don't feel like fixing St.Button (heh), you can use St.Clickable for actions as well.

I wonder if we'd want to move the hover property from St.Clickable into St.Widget eventually - I already added hover support to St.Entry, now here's a case where we'd like to have it in St.BoxLayout: the MessageTray.actor change ("Box + hover") resembles a little the St.BoxLayout abuse elsewhere ("Clutter.Group + CSS").

The rest of the patch feels like a nice cleanup (especially where you change _summaryBin from Box to Bin - ugggh).

(In reply to comment #8)
The patch breaks clicks on the notification icon as well.

Review of attachment 156215 [details] [review]:

Looks good overall!

Currently it is possible for both "normal" and summary notifications to show up simultaneously, which looks overly busy on small screens - not sure we want this.

That said, there's not too much to say:

::: js/ui/messageTray.js
@@ +395,2 @@
         this._summaryBin.opacity = 0;
+        this._summaryNotificationBin = new St.Clickable({ name: 'notification-box',

The style id is currently unused? Would it make sense to make this a style class instead and use the same on this._notificationBin?

@@ +867,3 @@
+    _showSummaryNotification: function() {
+
+    },

// See previous review

::: js/ui/notificationDaemon.js
@@ -361,3 @@
-            this.handleReplacing = false;
-        if (app.get_name() == EMPATHY)
-

Looks unrelated to this patch.

(In reply to comment #10)
> Currently it is possible for both "normal" and summary notifications to show up
> simultaneously, which looks overly busy on small screens - not sure we want
> this.

that's currently called for in the design. we probably need some sort of separation between them though rather than having the two black backgrounds just bleed into each other. i figured Jon and Jeremy would have further refinements after we got the initial version in.

(In reply to comment #6)
> its a little uncomfortable if StClickable breaks if any of the
> children get the return value wrong. Would it work to track the hover state in
> captured-event?

Not if one of its ancestors got the return value of captured-event wrong. :) I don't think we need to be second-guessing the semantics of event handling here. If the child wants to keep the parent from knowing that the pointer is hovering over it, then it can do it. If it didn't want to do that but got the return value wrong, then it's just a bug and needs to be fixed.


I mentioned has-pointer yesterday, but it turns out that that is only set on the innermost reactive actor, and it still doesn't work the way we want wrt grabs. If we want to be able to update hover automatically after a pointer grab (qv bug 609802), we'd need to create our own grab framework with grab-end notification, and use that consistently everywhere in the shell rather than using clutter_grab_pointer() directly.

Created attachment 156904 [details] [review]
[StWidget] add (optional) hover tracking

If track-hover is set, update the hover property automatically, and
the "hover" pseudo class to match, as StClickable used to do. (Remove
the corresponding code in StClickable). Tweak the tooltip handling to
use track-hover, which also makes it slightly more reliable in the
presence of reactive children, etc.

Created attachment 156906 [details] [review]
[MessageTray] Use #StWidget:track-hover

St.Widget's new "hover" property takes reactive children into account
when deciding whether or not the pointer has actually left the actor,
so it works better than the code that used to be here.

Created attachment 156907 [details] [review]
[MessageTray] remove bottom rounded corners

Created attachment 156908 [details] [review]
[MessageTray] pop out the last notification when mousing over the summary

Review of attachment 156904 [details] [review]:

Looks good to me other than style niggles. Not completely sold on the event-bubbling vs. capture thing - it seems to me that there's a much stronger responsibility to think about the whole picture when you are capturing events, but we can try it this way and see how much time we spend whack-a-moling enter/leave event returns :-). In many cases people probably should be using notify::hover instead in any case.

::: src/st/st-clickable.c
@@ +85,3 @@
 {
   StClickable *self = ST_CLICKABLE (actor);
+  gboolean ret;

'gboolean result' is currently beating 'gboolean ret' in ST by 5:1.

@@ +93,2 @@
   if (self->priv->held)
+    set_pressed (self, st_widget_get_hover (ST_WIDGET (actor)));

TRUE would seem clearer than get_hover()

@@ +112,2 @@
+  if (self->priv->held)
+    set_pressed (self, st_widget_get_hover (ST_WIDGET (actor)));

Similarly here FALSE would seem clearer than get_hover()

::: src/st/st-widget.c
@@ +1146,3 @@
 static gboolean
+st_widget_contains (ClutterActor *widget,
+                    ClutterActor *other)

If this was a StWidget method (even static), I'd expect it to take a StWidget argument... I think I'd just call it 'actor_contains()' or something like that - most of the static functions in StWidget aren't st_widget_*.

@@ +1162,3 @@
+    {
+      if (st_widget_contains (actor, event->source))
+        st_widget_set_hover ((StWidget*) actor, TRUE);

My feelings on ad-hoc "checked casts aren't fast enough" optimizations are well known, I think. There's a #define if we think that's the case.

@@ +2029,3 @@
+ * st_widget_set_track_hover:
+ * @widget: A #StWidget
+ * @track_hover: #TRUE if the widget should track the pointer hover state

%TRUE not #TRUE

@@ +2054,3 @@
+  priv = widget->priv;
+
+  if (priv->track_hover != track_hover)

gboolean setter should have a 'track_hover = track_hover != FALSE'

@@ +2092,3 @@
+ * If you have set #StWidget:track-hover, you should not need to
+ * call this directly. Use st_widget_sync_hover() if you need to
+ * manually track hover due to a pointer grab.

I don't think that's manually tracking hover. "if you need to get hover tracking back into sync after a pointer grab disturbs tracking" or something.

@@ +2104,3 @@
+  priv = widget->priv;
+
+  if (priv->hover != hover)

Similarly should have the != FALSE

(In reply to comment #13)
> If track-hover is set, update the hover property automatically, and
> the "hover" pseudo class to match, as StClickable used to do. (Remove
> the corresponding code in StClickable).

Should the corresponding code in StEntry and StButton be adapted as well?

(In reply to comment #17)
> TRUE would seem clearer than get_hover()

> Similarly here FALSE would seem clearer than get_hover()

it's possible for hover to be FALSE after an enter event or TRUE after a leave event (if we have a grab and this was actually someone else's event, or if we left into a child).

(In reply to comment #19)
> (In reply to comment #17)
> > TRUE would seem clearer than get_hover()
> 
> > Similarly here FALSE would seem clearer than get_hover()
> 
> it's possible for hover to be FALSE after an enter event or TRUE after a leave
> event (if we have a grab and this was actually someone else's event, or if we
> left into a child).

Needs a comment then. (and/or maybe factor the if (pressed) ... into an _update() function)

Review of attachment 156907 [details] [review]:

::: data/theme/gnome-shell.css
@@ +667,3 @@
+     * if the background is a gradient. So, we draw a gradient
+    /* FIXME: currently StWidget can only draw non-uniform corners
+    border-radius: 5px 5px 0px 0px;

I'm pretty sure that this won't work if you leave out background-gradient-direction

Review of attachment 156906 [details] [review]:

Very nice cleanup.

Review of attachment 156908 [details] [review]:

Overall looks good - there is a problem when hovering over a source while it has a notification popped out (or the opposite of a notification popping out while hovering over its source): the same notification is displayed twice (either _both_ bottom center or _both_ bottom right, seems more or less random)

(In reply to comment #17)
> My feelings on ad-hoc "checked casts aren't fast enough" optimizations are well
> known, I think. There's a #define if we think that's the case.

I've changed it, but that code was just copied here from StClickable.

> +  if (priv->track_hover != track_hover)
> 
> gboolean setter should have a 'track_hover = track_hover != FALSE'

That was also just copied code. (from st_widget_set_has_tooltip). The only place in all of st where we currently do "!= FALSE" is st_scroll_view_set_vshadows(). I'm going to leave this matching everything else for now and we can fix all of St later. (There are also a lot of missing g_object_notify()s.)

> I don't think that's manually tracking hover. "if you need to get hover
> tracking back into sync after a pointer grab disturbs tracking" or something.

yeah... half the sentence got rewritten and half didn't :)

Attachment 156906 [details] pushed as 994b4c0 - [MessageTray] Use #StWidget:track-hover
Attachment 156907 [details] pushed as 68b943d - [MessageTray] remove bottom rounded corners

Created attachment 157008 [details] [review]
don't show same notification as both main and summary notification

addendum to previous patch to fix that problem

(In reply to comment #26)
> Created an attachment (id=157008) [details] [review]
> don't show same notification as both main and summary notification
> 
> addendum to previous patch to fix that problem

I wonder if it makes sense (sometime, not now) to set the hover notification after the "regular" notification popped in - IMO this would stress the impression of notifications "moving" into the summary after timing out ...

Patches look good to me, may make sense to squash the second one before committing.

squashed, merged, pushed

Attachment 156908 [details] pushed as ac54fed - [MessageTray] pop out the last notification when mousing over the summary