http://cgit.collabora.com/git/user/pochu/empathy.git/log/?h=move-video-preview-656268

Created attachment 193528 [details] [review]
empathy-call: don't run g_option_context_parse

It's not that important and it's breaking clutter events (!).

Created attachment 193529 [details] [review]
CallWindow: add top and right margin to the preview container

So that when it's placed on another corner, it still has some
margin.

Created attachment 193530 [details] [review]
CallWindow: allow to move the video preview

Created attachment 193531 [details] [review]
CallWindow: show drop zones when dragging the preview

Created attachment 193532 [details] [review]
Factor out empathy_call_window_get_preview_position()

Created attachment 193533 [details] [review]
CallWindow: highlight drop zones when hovered

Created attachment 193534 [details] [review]
Move empathy_call_window_get_preview_position around

Created attachment 193535 [details] [review]
Factor out empathy_call_window_get_preview_rectangle

Created attachment 193536 [details] [review]
Factor out empathy_call_window_darken_preview_rectangle

Created attachment 193537 [details] [review]
Highlight the preview when hovered

Created attachment 193538 [details] [review]
Don't darken the preview when dragging it

Created attachment 193539 [details] [review]
Darken the rectangles when starting a drag operation

So that if we have just dragged it, the rectangle where
it was before isn't highlighted.

Created attachment 193540 [details] [review]
Use self->priv->preview_pos to determine the preview position

Review of attachment 193528 [details] [review]:

I'm pretty skeptical of this. Are you calling clutter_init() somewhere?

Review of attachment 193529 [details] [review]:

::: src/empathy-call-window.c
@@ +540,3 @@
   priv->video_preview = clutter_box_new (layout);
   clutter_actor_set_size (priv->video_preview,
+      SELF_VIDEO_SECTION_WIDTH + 20, SELF_VIDEO_SECTION_HEIGTH + 20);

Magic numbers in code are bad. Move this value into a #define so we know what it is.

Review of attachment 193531 [details] [review]:

::: src/empathy-call-window.c
@@ +525,3 @@
 static void
+empathy_call_window_create_preview_rectangle (EmpathyCallWindow *self,
+    ClutterActor **rectangle,

This is really strange... why not return ClutterActor * ?

@@ +560,3 @@
+static void
+empathy_call_window_create_preview_rectangles (EmpathyCallWindow *self)
+      1);

Would it be worth packing these rectangles into a group. Then you only need to hold a pointer to the group, you only need to set the group visible, you only need to destroy the group, etc.

(In reply to comment #15)
> Review of attachment 193528 [details] [review]:
> 
> I'm pretty skeptical of this. Are you calling clutter_init() somewhere?

gtk_clutter_init() calls it (and its documentation says you shouldn't call clutter_init directly if you call gtk_clutter_init). I've found the real problem now though, so I'm not doing that hack anymore but calling gdk_disable_multidevice(), which is the right thing to do. See the new commit message.

(In reply to comment #16)
> Review of attachment 193529 [details] [review]:
> 
> ::: src/empathy-call-window.c
> @@ +540,3 @@
>    priv->video_preview = clutter_box_new (layout);
>    clutter_actor_set_size (priv->video_preview,
> +      SELF_VIDEO_SECTION_WIDTH + 20, SELF_VIDEO_SECTION_HEIGTH + 20);
> 
> Magic numbers in code are bad. Move this value into a #define so we know what
> it is.

Done.

(In reply to comment #17)
> Review of attachment 193531 [details] [review]:
> 
> ::: src/empathy-call-window.c
> @@ +525,3 @@
>  static void
> +empathy_call_window_create_preview_rectangle (EmpathyCallWindow *self,
> +    ClutterActor **rectangle,
> 
> This is really strange... why not return ClutterActor * ?

Huh, I wonder why I didn't do that! Changed (amended).

> 
> @@ +560,3 @@
> +static void
> +empathy_call_window_create_preview_rectangles (EmpathyCallWindow *self)
> +      1);
> 
> Would it be worth packing these rectangles into a group. Then you only need to
> hold a pointer to the group, you only need to set the group visible, you only
> need to destroy the group, etc.

We still need pointers to each rectangle as we want to highlight one of them when dragging the preview. I'm also not sure about that, as I think it would receive events if it was on top of other actors, and we don't want that for e.g. the preview button or the floating toolbar.

Branch updated.

++

This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.