csw review notes

[Adam Jackson <ajax redhat com>]

(Originally sent to alex directly, sending to list for wider distribution)

@@ _gdk_display_enable_motion_hints
+    {
+      serial = _gdk_windowing_window_get_next_serial (display);
+      /* We might not actually generate the next request, so
+        make sure this triggers always, this may cause it to
+        trigger slightly to early, but this is just a hint
+        anyway. */

s/slightly to/slightly too/

@@ gdk_window_real_window_get_pointer
+  normal_child = GDK_WINDOW_IMPL_GET_IFACE (private->impl)->get_pointer (window,
+                                                                        &tmpx, &tmpy,
+                                                                        &tmp_mask);
+  /* We got the coords on the impl, conver to the window */

convert

+/* returns old clip region */
+void
+_gdk_gc_add_drawable_clip (GdkGC     *gc,
+                          guint32    region_tag,
+                          GdkRegion *region,
+                          int        offset_x,
+                          int        offset_y)

No it doesn't.

@@ gdk_gc_copy
+  if (dst_priv->old_clip_region)
+    gdk_region_destroy (dst_priv->old_clip_region);
+
+  if (src_priv->old_clip_region)
+    dst_priv->old_clip_region = gdk_region_copy (src_priv->old_clip_region);
+  else
+    dst_priv->old_clip_region = NULL;
/* etc */

This is a style thing, but gdk_region_destroy() is safe to call with a
NULL argument.  Likewise, gdk_region_copy() returns NULL when called
with a NULL argument.  g_object_ref() isn't safe like this if
G_DISABLE_CHECKS, which is kind of mad.

+void _gdk_syntesize_crossing_events_for_geometry_change (GdkWindow *changed_window);

synthesize

+static void miSetExtents (GdkRegion       *pReg);

Booo forward decls.  Could just move the def up.

+      /* The reparent will have put the native window topmost in the native parent,
+       * which may be wrong wrt other native windows in the non-native hierarchy,
+       * so restack */
+      above = find_native_sibling_above (private->parent, private);
+      if (above)
+       {
+         listhead.data = window;
+         GDK_WINDOW_IMPL_GET_IFACE (private->impl)->restack_under ((GdkWindow *)above,
+                                                                   &listhead);
+       }

This is duplicated in gdk_window_reparent and gdk_window_ensure_native.
Might be worth unifying.

+  /* We approach this by taking the new move and pushing it ahead of moves
+     starting at the end of the list and stopping when its not safe to do so.
+     Its not safe to push past a move is either the source of the new move
+     is in the destination of the old move, or if the destination of the new
+     move is in the source of the new move, or if the destination of the new
+     move overlaps the destination of the old move. We simplify this by

It's not safe.  s/move is either/move if either/

+  if (1) /* Enable flicker free handling of moves. */
+    append_move_region (impl_window, region, dx, dy);
+  else
+    do_move_region_bits_on_impl (impl_window,
+                                region, dx, dy);

Safe to remove the if (1) now?

+             /* Right order, since native_chilren is bottom-opmost first */

children, topmost

Seems like big chunks of gdk_window_move_resize_internal and
gdk_window_scroll are duplicated but not exactly.  Might be worth
refactoring?

@@ gdk_pointer_grab
+  /* We need a native window for confine to to work, ensure we have one */
+  if (confine_to)
+    {
+      if (!gdk_window_ensure_native (confine_to))
+       {
+         g_warning ("Can't confine to grabbed window, not native");
+         confine_to = NULL;
+       }
+    }

This kinda sucks.  Might be possible to create an input-only window
coincident with the csw you're trying to confine to, and then
garbage-collect it when the grab goes away.  Although, you might have to
suppress crossing events when it unmaps?  Hmm.  Also I have no idea if
non-X11 backends have input-only windows.

+      /* We synthesize all crossing events due to grabs outselves,

ourselves

@@ gdk_x11_drawable_get_xid
+      /* Try to ensure the window has a native window */
+      if (!_gdk_window_has_impl (window))
+       {
+         gdk_window_ensure_native (window);
+         
+         /* We sync here to ensure the window is created in the Xserver when
+          * this function returns. This is required because the returned XID
+          * for this window must be valid immediately, even with another
+          * connection to the Xserver */
+         gdk_display_sync (gdk_drawable_get_display (window));

It's not clear to me that _gdk_window_has_impl() succeeding implies that
the XID is known by the server yet.  So only syncing if it fails might
not be enough anyway?  Looking at gdk_window_new() it seems like you can
return from that without round-tripping.

- ajax

Attachment: signature.asc
Description: This is a digitally signed message part