Re: Merging spif-2 branch

[Ray Strode <halfline gmail com>]

Hi,

I just had a quick look at your patch and I have a few
(mostly nitpicky-type) comments.
> @@ -26,64 +24,38 @@
> +#include <math.h>
> +#include "workspace.h"
> +
> +#include <math.h>
you include math.h twice.

> +#include <cm/ws.h>
> +#include <cm/wsint.h>
What is wsint.h ? My first guess from the name is it is an
internel header. If so, should you be including it directly
here?  Also, why install it at all?  It might cause abi
complications down the road if people start using it...

Just another note on libcm.  You have data structures like Node,
Extension, Extents, etc, that use relatively common names.
These should probably be namespaced.

Or is libcm a convenience library that will be used only by metacity?

> +      if (!compositor->have_composite)
> +	g_print ("no composite\n");
> +      if (!compositor->have_fixes)
> +	g_print ("no fixes\n");
> +      if (!compositor->have_damage)
> +	g_print ("have damage\n");
you should probably clean these up (either remove them or use
the metacity logging api

> +double tmp;
This should get removed.

> +#include <X11/Xlib.h>
Why's this in the middle of the code instead of at the top?

> +static MetaScreen *
> +node_get_screen (Display *dpy,
> +		 DrawableNode *node)
>  {
> -  MetaCompositor *compositor = data;
> -
> -  compositor->repair_idle = 0;
> -  do_repair (compositor);
> +  /* FIXME: we should probably have a reverse mapping
> +   * from nodes to screens
> +   */
>
> -  return FALSE;
> +  Screen *screen = XDefaultScreenOfDisplay (dpy);
> +  return meta_screen_for_x_screen (screen);
>  }
Is this going to break multihead setups?
>  #ifdef HAVE_COMPOSITE_EXTENSIONS
>  static void
>  process_configure_notify (MetaCompositor  *compositor,
>                            XConfigureEvent *event)
>  {
> +  WsWindow *above_window;
> +  DrawableNode *node = g_hash_table_lookup (compositor->window_hash,
> +					    &event->window);
> +  DrawableNode *above_node;
>    MetaScreen *screen;
> +  if (!node)
>      return;
> +  screen = node_get_screen (compositor->display->xdisplay, node);
> +
> +  above_window = ws_window_lookup (node->drawable->ws, event->above);
> +
> +  if (above_window == compositor->glw)
>      {
> +      above_node = screen->compositor_windows->data;
>      }
>    else
>      {
> +      above_node = g_hash_table_lookup (compositor->window_hash,
> +					&event->above);
>      }
> +
> +  handle_restacking (compositor, node, above_node);
>  }
can above_node ever be NULL?  If so, maybe you should check
for it and not bother calling handle_restacking.  If it can't
ever be null maybe some sort of assertion check would be a
good idea.

> @@ -773,21 +336,22 @@ process_expose (MetaCompositor     *comp
[...]
>    region = XFixesCreateRegion (compositor->display->xdisplay, &r, 1);
>
> -  merge_and_destroy_damage_region (compositor, screen, region);
> +  XFixesDestroyRegion (compositor->display->xdisplay, region);
>  }
You seem to be creating a region and then immediately destroying
it.

>  process_map (MetaCompositor     *compositor,
>               XMapEvent          *event)
>  {
[...]
> +	{
> +#if 0
> +	  g_print ("Map window 0x%lx\n", event->window);
> +#endif
> +	  meta_compositor_add_window (compositor,
> +				      event->window, &attrs);
debugging cruft here
> @@ -952,65 +515,58 @@ process_reparent (MetaCompositor      *c
[...]
>    if (meta_error_trap_pop_with_return (compositor->display, TRUE) != Success)
>      {
> +      g_print ("attrs\n");
and here
>        meta_topic (META_DEBUG_COMPOSITOR, "Failed to get attributes for window 0x%lx\n",
> -                  event->window);
> +		  event->window);
>      }
>    else
>      {
>        meta_topic (META_DEBUG_COMPOSITOR,
> -                  "Reparent window 0x%lx into screen 0x%lx, adding\n",
> -                  event->window, event->parent);
> +		  "Reparent window 0x%lx into screen 0x%lx, adding\n",
> +		  event->window, event->parent);
> +      g_print ("adding\n");
and here, too
> @@ -1023,53 +579,74 @@ meta_compositor_process_event (MetaCompo
[...]
> +#if 0
>        process_damage_notify (compositor,
> -                             (XDamageNotifyEvent*) event);
> +			     (XDamageNotifyEvent*) event);
> +#endif
Is this the "don't repaint in a loop, but instead only repaint
when damage arrives." todo?
> +#ifdef HAVE_COMPOSITE_EXTENSIONS
> +static void
> +wavy (double time,
> +      double in_x, double in_y,
> +      double *out_x, double *out_y,
> +      gpointer data)
> +{
> +  static int m;
> +  time = time * 5;
> +  double dx = 0.0025 * sin (time + 35 * in_y);
> +  double dy = 0.0025 * cos (time + 35 * in_x);
> +
> +  *out_x = in_x + dx;
> +  *out_y = in_y + dy;
> +
> +  m++;
> +}
> +#endif
What's the point of m?  Also, you don't use this function, so
maybe it should be removed?  Or are you planning on adding wavy
effects at some point?
> @@ -1079,106 +656,52 @@ meta_compositor_add_window (MetaComposit
>                              XWindowAttributes *attrs)
>  {
[...]
> +  drawable = (WsDrawable *)ws_window_lookup (compositor->ws, xwindow);
It seems a little strange to me that a function called _lookup
will actually register a window with the ws. Maybe
ws_get_window_from_xwindow or something?
> +typedef struct Info
> +{
> +  MetaScreen  *screen;
> +  WsWindow	*window;
> +} Info;
> +
> +Info *the_info;

should be static yea?  Also Info is kind of a generic name.
>  void
>  meta_compositor_manage_screen (MetaCompositor *compositor,
>                                 MetaScreen     *screen)
>  {
[...]
> +  compositor->glw = ws_window_new_gl (root);
Won't this break if you call manage_screen for more than one
screen?
> +  ws_init_composite (compositor->ws);
> +  ws_init_damage (compositor->ws);
> +  ws_init_fixes (compositor->ws);
>
> +  ws_window_redirect_subwindows (root);
> +  ws_window_set_override_redirect (compositor->glw, TRUE);
> +  ws_window_unredirect (compositor->glw);
>
> +  region = ws_region_new (compositor->ws);
> +  ws_window_set_input_shape (compositor->glw, region);
> +  ws_region_unref (region);
> +
> +  ws_window_map (compositor->glw);
> +
> +  ws_sync (compositor->ws);
> +
> +  info = g_new (Info, 1);
> +  info->window = compositor->glw;
> +  info->screen = screen;
> +
> +  g_idle_add (update, info);
> +
> +  the_info = info;
> +#endif
>  }
So MetaCompositor should be either per-Display or per-Screen.
A lot of the code assumes it is per-Display, but this function
should be called per-Screen I think.  But a lot of the stuff
that happens in this function should only happen once if
MetaCompositor is per-Display.  I think things need to be
restructured a bit if multihead is going to work.  Correct me if
I'm wrong.

--Ray