Created attachment 174223 [details] [review]
dnd: Fix DND when the magnifier is active

As DND relies on actors being monkey-patched with a _delegate object,
it breaks with the magnifier which acts on a Clutter.Clone of the UI.
To fix, temporarily hide the zoomed region as already done with the
dragged actor.

Review of attachment 174223 [details] [review]:

My initial feeling was "wow, this is ugly, but I don't have a better idea". Then I had a better idea. My initial review is below in case the better idea doesn't work out.

 static void
 stop_pick (ClutterActor          *actor,
            const ClutterColor    *color)
 {
      g_signal_stop_emission_by_name (self, "pick");
 }

 void
 shell_set_hidden_from_pick (ClutterActor *actor,
                             gboolean      hidden)
 {
   if (hidden)
      g_signal_connect (actor, "pick", G_CALLBACK (stop_pick), NULL);

   else
      g_signal_handlers_disconnect_by_func (actor,
                                            (gpointer)stop_pick,
                                            NULL);
 }

Maybe with g_object_set_data() usage so you can call it twice on an actor without getting two connections, or call it with FALSE on an actor that you've never called with TRUE and not get a warning.

In other words, the ability to make a "super non-reactive" actor that hides itself and its children from CLUTTER_PICK_ALL. We can use that for both the icon and the ZoomRegions.

[ Alternately a StNoPickBin that doesn't propagate pick paints to its children... but that's a lot more code, and it's sort of nice to be able to decorate an arbitrary actor ]

::: js/ui/magnifier.js
@@ +623,3 @@
+            this._magView.show();
+        else
+            this._magView.hide();

This actually conflicts with the major refactoring of ZoomRegion in bug 633582 where this._magView doesn't always exist. Probably makes sense to try and land that first.

In the current state where magView exists but is hidden, you are falling victim to a bug in clutter - clutter_actor_hide() on a hidden actor queues a redraw on the parent actor so will be quite expensive. (I had a suspicion, so checked the code and consequently filed http://bugzilla.clutter-project.org/show_bug.cgi?id=2422)

Beyond that, my feeling is that visible and active should be orthogonal so

 setVisible(false);
 setActive(true);

Doesn't result in a visible magview - it obviously doesn't matter for your temporary usage but I'd rather have apis that don't only work when called in only one way.

Created attachment 174682 [details] [review]
shell-util: Add helper to hide actors from pick

At times it is desireable to hide actors from being picked even
with a mode of CLUTTER_PICK_ALL.

Currently we use a pattern of
    clutter_actor_hide();
    clutter_stage_get_actor_at_pos();
    clutter_actor_show();
in these cases, which gets hideous if the actor we want to exclude
from the pick is located in another module.

A more elegant solution is to connect a handler to the ::pick signal,
which stops further emission.

Credit for the idea goes to Owen Taylor.

Created attachment 174683 [details] [review]
dnd: Hide drag actor from pick

Instead of hiding the drag actor temporarily to determine the actor
beneath it, make it invisible to picks while dragging using the new
shell_utils_set_hidden_from_pick().

Created attachment 174684 [details] [review]
magnifier: Fix DND when the magnifier is active

Hide the zoom regions from pick so that they do not interfere with
the picking done by DND in search for _delegate objects.

Created attachment 174694 [details] [review]
dnd: Hide drag actor from pick

Fix resetting the drag actor in all cases.

Created attachment 174707 [details] [review]
magnifier: Fix DND when the magnifier is active

Patch done on top of bug 633582.

Review of attachment 174682 [details] [review]:

Looks good except for some minor style points.

::: src/shell-util.c
@@ +417,3 @@
+ */
+
+void

Bit of an odd blank line

@@ +432,3 @@
+      id = g_signal_connect (actor, "pick", G_CALLBACK (stop_pick), NULL);
+      g_object_set_data (G_OBJECT (actor),
+                         "stop-pick-id", GUINT_TO_POINTER (id));

- generally object data should be namespaced - so shell-stop-pick
- Any reason not to just GUINT_TO_POINTER(1)?

Review of attachment 174694 [details] [review]:

Looks good

Review of attachment 174707 [details] [review]:

Likely going to conflict in a minor way with my outstanding big patch, but I'll fix the conflicts on that side.

(In reply to comment #8)
> - Any reason not to just GUINT_TO_POINTER(1)?

No.

Last patch is on top of bug 633582, so leaving it for now.


Attachment 174682 [details] pushed as 1c8955b - shell-util: Add helper to hide actors from pick
Attachment 174694 [details] pushed as be6e189 - dnd: Hide drag actor from pick

Attachment 174707 [details] pushed as 35b5cb9 - magnifier: Fix DND when the magnifier is active