The tooltip was removed from the power status. Is this still relevant?

the queue_relayout part presumably is if we're going to use StTooltip at all in the future

From investigation we were doing on mysterious drawing problems on Jon's machine, StTooltip causes some massive breakage with the tricks it is doing with calling clutter_actor_set_parent() / clutter_actor_unparent() behind the stage's back. We were getting things Clutter trying to paint alt-Tab icons actors as children of the stage.

If we are going to use the tooltip in the future, probably the first thing to look at at MxTooltip and see what changes/fixes have been made there.

Actually, the implementation used by Mx is completely different. Their tooltip is not parented to the stage, instead it is owned by the relevant widget, and then they do magic tricks to ensure it is painted/picked last.

On the other hand, probably the biggest mistake in our tooltip is just using clutter_actor_set_parent, when instead we should use clutter_container_add. At that point it becomes just another toplevel actor (we may need to weakref it though).

Hmm any chance this is causing bug 635695?

IIRC, the tooltip was showing up at 0, 0.

Created attachment 175543 [details] [review]
StTooltip: use clutter_container_add for adding to the stage

Use ClutterContainer functions for adding the tooltip instead of
calling clutter_actor_set_parent behind the stage's back, and do
it as soon as the owner widget is added to a stage instead of
waiting until shown (causing queue_relayout warnings).
Also, remove map / unmap vfuncs that are not needed as of clutter
1.5.8

Created attachment 175546 [details] [review]
StTooltip: use clutter_container_add for adding to the stage

Use ClutterContainer functions for adding the tooltip instead of
calling clutter_actor_set_parent behind the stage's back, and do
it as soon as the owner widget is added to a stage instead of
waiting until shown (causing queue_relayout warnings).
Also, remove map / unmap vfuncs that are not needed as of clutter
1.5.8

Created attachment 175548 [details] [review]
Status Area: hide tooltips when the menus are open

Destroy tooltips when opening the menu, and reinstate them when
the menus are closed.
This requires tooltips to be set using .setTooltip, instead of
.actor.tooltip_text (or they won't be reset when closing the menu).

Comment on attachment 175546 [details] [review]
StTooltip: use clutter_container_add for adding to the stage

>Also, remove map / unmap vfuncs that are not needed as of clutter
>1.5.8

feel free to commit that part now

>+static void
>+st_widget_tooltip_parent_set (StWidget *widget, ClutterActor *old_parent, gpointer user_data)
>+{
>+  StWidgetPrivate *priv;
>+
>+  priv = widget->priv;
>+
>+  g_signal_handlers_disconnect_by_func (priv->tooltip, st_widget_tooltip_parent_set, NULL);
>+
>+  st_widget_add_tooltip (widget, CLUTTER_CONTAINER (clutter_actor_get_stage (CLUTTER_ACTOR (widget))));
>+}

This won't always work, because it's possible that the widget's new parent isn't yet parented.

Given that none of the current designs call for tooltips, it might be simplest to just not bother fixing this for now...

>+            g_signal_connect (widget, "parent-set", G_CALLBACK (st_widget_tooltip_parent_set), NULL);

it's generally preferred to override widget_class->parent_set rather than connecting to a signal on yourself.

Comment on attachment 175548 [details] [review]
Status Area: hide tooltips when the menus are open

it shouldn't destroy them (for the reason mentioned in your commit message). it should just keep them from being displayed

again, not clear that it makes sense to fix this now though

Created attachment 177596 [details] [review]
StTooltip: use clutter_container_add for adding to the stage

Use ClutterContainer functions for adding the tooltip instead of
calling clutter_actor_set_parent behind the stage's back, and do
it inside st_widget_show_tooltip (which is a normal method) instead
of overriding st_tooltip_show, which is a vfunc and it is called
internally by Clutter, therefore it is limited in what it can safely
do.

Seems to work without corruptions. Attaching because there seems to be
a renewed interest in tooltips, for example in app icons.

Review of attachment 177596 [details] [review]:

I think this is generally the right approach

One thing we need to deal with this is that this isn't going to interact with the magnifier correctly - within the shell context, all UI content needs to be a child of Main.uiGroup and not part of the stage. (Zoom regions are done as clones of Main.uiGroup). My suggestion for this is to add:

 st_get/set_ui_root (ClutterStage *stage);

Which uses g_object_set_data() to store an actor that should be the root for tooltips and similar.

::: src/st/st-widget.c
@@ +1470,3 @@
+            st_widget_add_tooltip (widget, CLUTTER_CONTAINER (stage));
+          else
+            clutter_actor_set_parent (CLUTTER_ACTOR (priv->tooltip), CLUTTER_ACTOR (widget));

I don't see why you need to do this and then fix up later.

One general thing I don't like about this patch is that if you move a widget with a tooltip between stages the tooltip is left on the old stage until next shown.

Does it work to:

 - Add the tooltip to the stage in st_widget_map (and also when creating it if already mapped)
 - Remove the tooltip from the stage in st_widget_unmap

?

@@ +1478,3 @@
         {
+          if (priv->tooltip)
+            clutter_actor_destroy (CLUTTER_ACTOR (priv->tooltip));

You seem to have lost setting priv->tooltip to NULL

@@ +1588,3 @@
+        {
+          g_assert (tooltip_parent == (ClutterActor*)widget);
+          g_object_ref (tooltip);

You leak the reference. The best thing to do here is to make memory management of the tooltip independent of the widget hierarchy - to keep it around until you dispose it. To do that, 

          priv->tooltip = g_object_new (ST_TYPE_TOOLTIP, NULL);
+         g_object_ref_sink (priv->tooltip);

Then unref the tooltip in st_widget_dispose()

@@ +1589,3 @@
+          g_assert (tooltip_parent == (ClutterActor*)widget);
+          g_object_ref (tooltip);
+          clutter_actor_unparent (tooltip);

clutter_actor_unparent() is only for container implementations. Use clutter_container_remove()

Created attachment 178049 [details] [review]
Make St aware of the UI group.

Inside the Shell, all the UI (including chrome, the overview, and
the actual windows) is not a child of the stage but of a special
ClutterGroup, which is cloned inside the magnifier.
Add function for setting this special actor so that actors added by
St are visible in the magnifier. Nothing yet uses this, but the
tooltip will soon.

Created attachment 178053 [details] [review]
Make St aware of the UI group.

Inside the Shell, all the UI (including chrome, the overview, and
the actual windows) is not a child of the stage but of a special
ClutterGroup, which is cloned inside the magnifier.
Add function for setting this special actor so that actors added by
St are visible in the magnifier. Nothing yet uses this, but the
tooltip will soon.

Second try, now using ClutterContainer instead of ClutterActor, and
with g_return_*_if_fail.

(In reply to comment #15)
> Created an attachment (id=178053) [details] [review]
> Make St aware of the UI group.
> 
> Inside the Shell, all the UI (including chrome, the overview, and
> the actual windows) is not a child of the stage but of a special
> ClutterGroup, which is cloned inside the magnifier.
> Add function for setting this special actor so that actors added by
> St are visible in the magnifier. Nothing yet uses this, but the
> tooltip will soon.
> 
> Second try, now using ClutterContainer instead of ClutterActor, and
> with g_return_*_if_fail.

Can't we just give the actor a name and use clutter_container_find_child_by_name ?

(In reply to comment #16)
> (In reply to comment #15)
> > Created an attachment (id=178053) [details] [review] [details] [review]
> > Make St aware of the UI group.
> > 
> > Inside the Shell, all the UI (including chrome, the overview, and
> > the actual windows) is not a child of the stage but of a special
> > ClutterGroup, which is cloned inside the magnifier.
> > Add function for setting this special actor so that actors added by
> > St are visible in the magnifier. Nothing yet uses this, but the
> > tooltip will soon.
> > 
> > Second try, now using ClutterContainer instead of ClutterActor, and
> > with g_return_*_if_fail.
> 
> Can't we just give the actor a name and use
> clutter_container_find_child_by_name ?

Check out the implementation of that function.

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > Created an attachment (id=178053) [details] [review] [details] [review] [details] [review]
> > > Make St aware of the UI group.
> > > 
> > > Inside the Shell, all the UI (including chrome, the overview, and
> > > the actual windows) is not a child of the stage but of a special
> > > ClutterGroup, which is cloned inside the magnifier.
> > > Add function for setting this special actor so that actors added by
> > > St are visible in the magnifier. Nothing yet uses this, but the
> > > tooltip will soon.
> > > 
> > > Second try, now using ClutterContainer instead of ClutterActor, and
> > > with g_return_*_if_fail.
> > 
> > Can't we just give the actor a name and use
> > clutter_container_find_child_by_name ?
> 
> Check out the implementation of that function.

Well the uiGroup is the first (and besides dnd actors iirc the only) direct child of the stage, so we shouldn't hit the recursive code path at all (i.e it should stop at the first list iteration).

Or where you referring to something else?

Review of attachment 178053 [details] [review]:

Looks good to me except for details

::: src/st/st-widget.c
@@ +108,3 @@
 gfloat st_slow_down_factor = 1.0;
 
+static GQuark st_ui_root_quark (void);

You don't need this forward declaration if you make the function static as it should be, since it's only used after declaration

@@ +1954,3 @@
+
+GQuark
+st_ui_root_quark (void)

Should be static

This is not a suggestion for a change - it's fine this way and this technique does make sense in many cases, but I'll point out that the gain from using a static variable and a quark is pretty small - for something like mapping a tooltip once per frame it would be fine to just do:

 g_object_get_data (G_OBJECT (stage), "shell-ui-root");

@@ +1958,3 @@
+  static GQuark value = 0;
+  if (G_UNLIKELY (value == 0))
+    value = g_quark_from_static_string ("shell-ui-root");

should be 'st-ui-root' - the namespace inside shell/st is 'st' - in theory, code in st can be moved to MX by global search and replace of ST with MX.

@@ +1967,3 @@
+ * @container: the new UI root
+ *
+ * Sets a #ClutterContainer to be the parent of all UI in the program.

Add "This container is used when St needs to add new content outside the widget hierarchy, for example, when it shows a tooltip over a widget."

@@ +1970,3 @@
+ */
+void
+st_set_ui_root (ClutterStage *stage, ClutterContainer *container)

Parameters should be on multiple lines

@@ +1975,3 @@
+  g_return_if_fail (CLUTTER_IS_CONTAINER (container));
+
+  g_object_set_qdata_full (G_OBJECT (stage), st_ui_root_quark (), g_object_ref (container), g_object_unref);

While we don't need it for our usage, I think I'd like to see a "destroy" connection to the container that clears the container for added robustness. Use g_signal_handlers_disconnect_by_func() to remove the old handler here if st_get_ui_root() is non-null. 

[It's safe to do this even in the case where the old root is being destroyed, since during destruction "destroy" is emitted, *before* signal handlers are removed, so you can just always remove.]

@@ +1983,3 @@
+ *
+ * Returns: (transfer none): the container which should be the parent of all user interface,
+ *   which can be set with st_set_ui_root. If not set, returns @stage

style nit: functions in gtk-doc comments should have trailing () so that gtk-doc will linkify them.

Created attachment 178072 [details] [review]
StTooltip: fix various warnings and use Clutter API correctly

Use ClutterContainer functions for adding the tooltip instead of
calling clutter_actor_set_parent behind the stage's back, and do
it inside st_widget_show_tooltip (which is a normal method) instead
of overriding st_tooltip_show, which is a vfunc and it is called
internally by Clutter, therefore it is limited in what it can safely
do.
Also, remove the complex positioning from st_widget_allocate (and
st_widget_show_tooltip), as it was causing queue_relayout warnings,
and replace it with a ClutterAlignConstraint.

WARNING: Does not work! Posting it anyway in case someone is able
to help me.

(In reply to comment #20)
> Created an attachment (id=178072) [details] [review]
> StTooltip: fix various warnings and use Clutter API correctly
> 
> Use ClutterContainer functions for adding the tooltip instead of
> calling clutter_actor_set_parent behind the stage's back, and do
> it inside st_widget_show_tooltip (which is a normal method) instead
> of overriding st_tooltip_show, which is a vfunc and it is called
> internally by Clutter, therefore it is limited in what it can safely
> do.
> Also, remove the complex positioning from st_widget_allocate (and
> st_widget_show_tooltip), as it was causing queue_relayout warnings,
> and replace it with a ClutterAlignConstraint.
> 
> WARNING: Does not work! Posting it anyway in case someone is able
> to help me.

In what way does this not work? Can you add a test case to tests/interactive that exercises this functionality?

Try it:
1) you still get queue_relayout warnings, despite that no code is doing layout at allocation time.
2) the tooltip is fixed at 0,0 instead of being constrained as code would suggest.
Maybe I'm doing it wrong?

(Oh, right: if you need a tooltip, summon the looking glass, click the background of a status icon, r(0)._delegate.setTooltip('test'), mouse hover)

Created attachment 181759 [details] [review]
Make St aware of the UI group.

Inside the Shell, all the UI (including chrome, the overview, and
the actual windows) is not a child of the stage but of a special
ClutterGroup, which is cloned inside the magnifier.
Add function for setting this special actor so that actors added by
St are visible in the magnifier. Nothing yet uses this, but the
tooltip will soon.

Updated patch

Created attachment 181761 [details] [review]
Make St aware of the UI group.

Inside the Shell, all the UI (including chrome, the overview, and
the actual windows) is not a child of the stage but of a special
ClutterGroup, which is cloned inside the magnifier.
Add function for setting this special actor so that actors added by
St are visible in the magnifier. Nothing yet uses this, but the
tooltip will soon.

I forgot there was a review on the previous patch - now it's in.

Created attachment 181762 [details] [review]
StTooltip: fix various warnings and use Clutter API correctly

Use ClutterContainer functions for adding the tooltip instead of
calling clutter_actor_set_parent behind the stage's back, and do
it inside st_widget_show_tooltip (which is a normal method) instead
of overriding st_tooltip_show, which is a vfunc and it is called
internally by Clutter, therefore it is limited in what it can safely
do.
Also, remove the complex positioning from st_widget_allocate (and
st_widget_show_tooltip), as it was causing queue_relayout warnings,
and replace it with a StTooltipConstraint, a custom ClutterConstraint.

Now it works, hopefully without bugs and corruptions. But we're back
to queue_relayout warnings from Clutter, because the constraint is
listening on allocation-changed on a child of the UI group (the widget),
and queuing the relayout of another (the tooltip), which means
queuing the relayout of the UI group while in allocation.
Maybe I should use an idle?

Created attachment 181840 [details] [review]
StTooltip: fix various warnings and use Clutter API correctly

Use ClutterContainer functions for adding the tooltip instead of
calling clutter_actor_set_parent behind the stage's back, and do
it inside st_widget_show_tooltip (which is a normal method) instead
of overriding st_tooltip_show, which is a vfunc and it is called
internally by Clutter, therefore it is limited in what it can safely
do.
Also, remove the complex positioning from st_widget_allocate (and
st_widget_show_tooltip), as it was causing queue_relayout warnings,
and replace it with StTooltipConstraint, a custom ClutterConstraint.

With the g_idle_add hack, everything works correctly, including tooltips
for message tray sources (shown above) and near the boundaries of the
screen, and we avoid warnings.

(In reply to comment #27)
>
> With the g_idle_add hack, everything works correctly, including tooltips
> for message tray sources (shown above) and near the boundaries of the
> screen, and we avoid warnings.

Well we use this "hack" in a lot of places to avoid allocation cycle warnings so unless clutter provides a better way this is fine.

(In reply to comment #28)
> Well we use this "hack" in a lot of places to avoid allocation cycle warnings
> so unless clutter provides a better way this is fine.

Shouldn't that rather be meta_later_add()?

(In reply to comment #29)
> (In reply to comment #28)
> > Well we use this "hack" in a lot of places to avoid allocation cycle warnings
> > so unless clutter provides a better way this is fine.
> 
> Shouldn't that rather be meta_later_add()?

Yeah, the comment was more about using an idle handler to fix those warnings.

(In reply to comment #28)
> (In reply to comment #27)
> >
> > With the g_idle_add hack, everything works correctly, including tooltips
> > for message tray sources (shown above) and near the boundaries of the
> > screen, and we avoid warnings.
> 
> Well we use this "hack" in a lot of places to avoid allocation cycle warnings
> so unless clutter provides a better way this is fine.

It's bad in every place we do it. If there's no better way to do it, then we have to do it anyways, but that doesn't mean that it's fine. In particular, one thing we have to be careful about is single frame misdraws, which are normally visible, and more visible if performance is bad. It's not OK, say, to show a tooltip at 0,0 and then position it at the right position in an idle (not saying that this patch does that, but it's the kind of thing we have to avoid.)

We can't use meta_later_add() here because this is in ST, and it doesn't really help anyways - if we discover that we need to reposition something when an actor is allocated, then we don't have any opportunity to position it until the frame that follows the allocation is drawn.

What I think we can do to make this work without any hacks is to leave the tooltip actor at 0,0 and change where it appears using clutter_actor_set_anchor_point()

Created attachment 182820 [details] [review]
StTooltip: fix various warnings and use Clutter API correctly

Use ClutterContainer functions for adding the tooltip instead of
calling clutter_actor_set_parent behind the stage's back, and do
it inside st_widget_show_tooltip (which is a normal method) instead
of overriding st_tooltip_show, which is a vfunc and it is called
internally by Clutter, therefore it is limited in what it can safely
do.
Also, instead of positioning the tooltip with clutter_actor_set_position,
modify the anchor point when the associated widget moves, so that
only a redraw is queued.

----

Going back to master for most of the positioning logic, but using
clutter_actor_set_anchor_point instead of clutter_actor_set_position
in st_tooltip_update_position.
I removed all calls clutter_actor_set_size, allocation should take care
of ensuring that the tooltip is not wider than the stage (and it is very
unlikely anyway).

Review of attachment 182820 [details] [review]:

In generall, this looks like it's using techniques that will be reliable and make sense. 

I'm not really sure about the tracking of visibility, however - it looks like we can have shown tooltips when the actor is unmapped  - that doesn't happen in the hover case because st_widget_unmap() will unset hover, which will call st_widget_hide_tooltip(), but can happen for explicit show/hide calls.

I guess I'd like to see it so that 'st_widgt_show_tooltip/hide_tooltip' set some sort of flag, only show/hide the tooltip if the widget is mapped, and make st_widget_map/unmap show hide the tooltip as necessary - so basically, enforce that the tooltip is visible iff. (tooltip_shown && widget_mapped)

Also, I think every call to st_widget_show_tooltip() - every time the tooltip is shown - we should raise it to the top of the stage. I don't think this version does that.

::: src/st/st-tooltip.c
@@ +302,3 @@
   tooltip_y = (int)(tip_area->y + tip_area->height);
 
+  parent = clutter_actor_get_parent ((ClutterActor *) tooltip);

Hmm, note that there is an assumption that the UI group has the same coordinates as the stage, since the tip area is passed in in stage coordinates. Probably fine.

@@ +334,3 @@
   priv->arrow_offset = tip_area->x + tip_area->width / 2 - tooltip_x;
 
+  clutter_actor_set_anchor_point ((ClutterActor*) tooltip, -tooltip_x, -tooltip_y);

This needs commentary as to why we are doing it this way.

::: src/st/st-widget.c
@@ -1645,3 @@
   if (widget->priv->tooltip)
     {
-      st_tooltip_set_tip_area (widget->priv->tooltip, &area);

I don't see what ensures that the the tooltip is positioned correctly on first show - it's probably likely that showing the tooltip will cause the widget to be reallocated, but certainly not guaranteed.

Created attachment 182966 [details] [review]
StTooltip: fix various warnings and use Clutter API correctly

Use ClutterContainer functions for adding the tooltip instead of
calling clutter_actor_set_parent behind the stage's back, and do
it inside st_widget_show_tooltip (which is a normal method) instead
of overriding st_tooltip_show, which is a vfunc and it is called
internally by Clutter, therefore it is limited in what it can safely
do.
Also, instead of positioning the tooltip with clutter_actor_set_position,
modify the anchor point when the associated widget moves, so that
only a redraw is queued.

Review of attachment 181761 [details] [review]:

Looks very good, one tiny style nit.

::: src/st/st-widget.c
@@ +2192,3 @@
+void
+st_set_ui_root (ClutterStage *stage,
+                ClutterContainer *container)

alignment

Review of attachment 182966 [details] [review]:

In testing, found one bug - need to just save the top area and not update the position if set_tip_area() is called before the tip is shown.

@@ -418,7 +418,8 @@ st_tooltip_set_tip_area (StTooltip             *tooltip,
     g_boxed_free (CLUTTER_TYPE_GEOMETRY, tooltip->priv->tip_area);
   tooltip->priv->tip_area = g_boxed_copy (CLUTTER_TYPE_GEOMETRY, area);
 
-  st_tooltip_update_position (tooltip);
+  if (clutter_actor_get_stage (CLUTTER_ACTOR (tooltip)))
+    st_tooltip_update_position (tooltip);
 }

Also a missing comment that I asked for in the last review. Good to commit with those two fixes.

::: src/st/st-tooltip.c
@@ +334,3 @@
   priv->arrow_offset = tip_area->x + tip_area->width / 2 - tooltip_x;
 
+  clutter_actor_set_anchor_point ((ClutterActor*) tooltip, -tooltip_x, -tooltip_y);

Still needs commentary about why we are doing it this way

suggest:

 /* Since we are updating the position out of st_widget_allocate(), we can't
  * call clutter_actor_set_position(), since that would trigger another
  * allocation cycle. Instead, we adjust the anchor position which moves
  * the tooltip actor on the screen without changing its allocation
  */

Comment on attachment 181761 [details] [review]
Make St aware of the UI group.

Attachment 181761 [details] pushed as afffa76 - Make St aware of the UI group.

Attachment 182966 [details] pushed as 087e86f - StTooltip: fix various warnings and use Clutter API correctly