Created attachment 341515 [details] [review]
mir: implement window properties

Created attachment 341516 [details] [review]
mir: track focused window

Created attachment 341517 [details] [review]
mir: connect to content-hub

Created attachment 341518 [details] [review]
mir: watch for pasteboard changes

Created attachment 341519 [details] [review]
mir: copy clipboard data to content-hub

Created attachment 341520 [details] [review]
mir: paste clipboard data from content-hub

Review of attachment 341515 [details] [review]:

Looks generally sound though a little obscure in parts and could do with some comments.

::: gdk/mir/gdkmirwindowimpl.c
@@ +1560,3 @@
+  *actual_format = 8 * width;
+
+  if (*actual_type == GDK_SELECTION_TYPE_ATOM || *actual_type == gdk_atom_intern_static_string ("ATOM_PAIR"))

This section is a little hard to decipher due to the use of *actual_type, perhaps change to:

*actual_type = mir_property->type;
if (mir_property->type == GDK_SELECTION_TYPE_ATOM || mir_property->type == gdk_atom_intern_static_string ("ATOM_PAIR"))
    *actual_format = 32;
else
    *actual_format = 8 * width;

@@ +1568,3 @@
+  offset *= 4;
+
+  if (length < G_MAXULONG - width + 1)

Perhaps a comment to say this is stopping overflow?

@@ +1603,3 @@
+  GdkEvent *event;
+
+  if (type == GDK_SELECTION_TYPE_ATOM || type == gdk_atom_intern_static_string ("ATOM_PAIR"))

Perhaps a comment to mention what is special about the ATOM and ATOM_PAIR types? Also the same above.

@@ +1617,3 @@
+    }
+
+  if (type != mir_property->type || format / 8 != g_array_get_element_size (mir_property->array))

The format / 8 part is not clear - perhaps use a variable to make clear what this means?

Review of attachment 341516 [details] [review]:

::: gdk/mir/gdkmirdisplay.c
@@ +591,3 @@
+  GdkMirDisplay *mir_display = GDK_MIR_DISPLAY (display);
+
+  mir_display->focused_window = window;

Does this need to be a reference? If the window is destroyed will this be unsafe?

Review of attachment 341517 [details] [review]:

::: configure.ac
@@ +465,3 @@
 fi
 
+MIR_DEPENDENCIES="mirclient >= mirclient_required_version mircookie >= mircookie_required_version libcontent-hub-glib"

Should content-hub be a hard dependency?

Review of attachment 341516 [details] [review]:

::: gdk/mir/gdkmirdisplay.c
@@ +591,3 @@
+  GdkMirDisplay *mir_display = GDK_MIR_DISPLAY (display);
+
+  mir_display->focused_window = window;

Does this need to be a reference? If the window is destroyed will this be unsafe?

Review of attachment 341518 [details] [review]:

::: gdk/mir/gdkmirdisplay.c
@@ +113,3 @@
+                       gpointer       user_data)
+{
+  g_clear_pointer (&display->paste_data, g_variant_unref);

paste_data doesn't actually seem to be set here - should it be?

Review of attachment 341519 [details] [review]:

::: gdk/mir/gdkmirwindowimpl.c
@@ +1678,3 @@
+      paste_format = _gdk_atom_name_const (mir_property->type);
+
+      if (!strchr (paste_format, '/'))

A comment about what '/' means in a paste format would help.

Review of attachment 341520 [details] [review]:

::: gdk/mir/gdkmirdisplay.c
@@ +588,3 @@
+{
+  const gchar *target_string;
+  GdkAtom type;

Might be nice to call these dummy_* as before so they don't look like used variables.

@@ +880,3 @@
+  array = g_ptr_array_new ();
+
+  while (ptr < (const gchar *) &text[length])

This is a bit confusing a comment on what is being done?

Created attachment 341579 [details] [review]
mir: implement window properties

Created attachment 341580 [details] [review]
mir: track focused window

Created attachment 341581 [details] [review]
mir: connect to content-hub

Created attachment 341582 [details] [review]
mir: copy clipboard data to content-hub

Created attachment 341583 [details] [review]
mir: paste clipboard data from content-hub

(In reply to Robert Ancell from comment #9)
> Review of attachment 341517 [details] [review] [review]:
> 
> ::: configure.ac
> @@ +465,3 @@
>  fi
>  
> +MIR_DEPENDENCIES="mirclient >= mirclient_required_version mircookie >=
> mircookie_required_version libcontent-hub-glib"
> 
> Should content-hub be a hard dependency?

I'm not sure since it only needs the glib client library, but I guess it's a subjective decision. If only libcontent-hub-glib is available, then it just fails silently. FWIW, libcontent-hub-glib0 in Ubuntu has a Suggests on content-hub.

(In reply to Robert Ancell from comment #11)
> Review of attachment 341518 [details] [review] [review]:
> 
> ::: gdk/mir/gdkmirdisplay.c
> @@ +113,3 @@
> +                       gpointer       user_data)
> +{
> +  g_clear_pointer (&display->paste_data, g_variant_unref);
> 
> paste_data doesn't actually seem to be set here - should it be?

Yeah, this commit didn't really make sense on its own. It's merged with the paste clipboard data commit now.