Review of patch for #79974, #79961, #79959

[Owen Taylor <otaylor redhat com>]

This patch is a bit to review in bugzilla, so I'll do on the
mailing list instead. Looks basically fine to commit. Various 
minor issues below.

===
 @@ -275,24 +275,24 @@
   * gdk_screen_get_monitor_geometry:
   * @screen : a #GdkScreen.
   * @monitor_num: the monitor number. 
 + * @dest : a #GdkRectangle to be filled with the monitor geometry
   *
 - * Returns a #GdkRectangle representing the size and start
 + * retrieve #GdkRectangle representing the size and start
===

Should be "Retrieves the"

===
 @@ -328,9 +328,27 @@
   **/
  gint 
  gdk_screen_get_monitor_at_window (GdkScreen      *screen,
 -				  GdkNativeWindow anid)
 +				  GdkWindow	 *window)
  {
 +  gint dummy, num_monitors, i, sum = 0, screen_num = 0;
 +  GdkRectangle win_rect;
    g_return_val_if_fail (GDK_IS_SCREEN (screen), -1);

 -  return GDK_SCREEN_GET_CLASS (screen)->get_monitor_at_window (screen, anid);
 +  gdk_window_get_geometry (window, &win_rect.x, &win_rect.y, &win_rect.width,
 +			   &win_rect.height, &dummy);
===

GTK+ univerally allows NULL for out arguments in order to avoid
having to use dummy variables like this.

 +  gdk_window_get_position (window, &win_rect.x, &win_rect.y);

I suspect that you want to call get_origin(). For better or worse, get_position()
doesn't reflect the results of a gdk_window_move() until a ConfigureNotify
is received back from the X server.

Also, calling get_origin() will make this work for both toplevel windows
and subwindows; if it only worked for toplevels, then that should be
noted in the docs and checked with a g_return_if_fail().

===
 +  num_monitors = gdk_screen_get_n_monitors (screen);
 +  
 +  for (i=0;i<num_monitors;i++)
 +    {
 +      GdkRectangle tmp_monitor, intersect;
 +      gdk_screen_get_monitor_geometry (screen, i, &tmp_monitor);
 +      gdk_rectangle_intersect (&win_rect, &tmp_monitor, &intersect);
 +      if ((intersect.width * intersect.height) > sum)
 +	{ 
 +	  sum = intersect.width * intersect.height;
 +	  screen_num = i;
 +	}
===

Indentatation problems here. 

As a matter of taste, I find the parentheses in "(intersect.width *
intersect.height) > sum" excessive, and might like to see a few more
blank lines.  (We usually put a blank line between variable
declarations and code.)

One of the open question that I'm still pondering is whether
gdk_screen_get_monitor_at_window() should return 0 or -1 when
the screen isn't on any monitor. 0 as you have it here should do
for the moment.

 +  gint          (*get_monitor_at_point)  (GdkScreen    *screen,
 +					  gint          x,
 +					  gint          y);

Hmmm, strikes me now that this one should probably move into gdkscreen.c

====
        result = XineramaGetInfo (GDK_SCREEN_XDISPLAY (screen),
                                 gdk_screen_get_number (screen),
                                 monitors, hints,
                                 &screen_x11->num_monitors);
 -      if (result != Success)
 +      /* Yes I know it should be Success but the current implementation 
 +          returns the num of monitor*/
 +      if (result != screen_x11->num_monitors)
====

Does the current implementation fill in the number of monitors on
failure? What does it put in there? This check seems a little odd ...
shouldn't it be something like 'if (result > 0)'.

====
 +static void
 +gdk_screen_x11_get_monitor_geometry (GdkScreen    *screen, 
 +				     gint          num_monitor,
 +				     GdkRectangle *dest)
  {
 -  g_return_val_if_fail (GDK_IS_SCREEN (screen), NULL);
 -  g_return_val_if_fail (num_monitor < GDK_SCREEN_X11 (screen)->num_monitors, NULL);
 -  
 -  return &GDK_SCREEN_X11 (screen)->monitors[num_monitor];
 +  GdkScreenX11 *screen_x11 = GDK_SCREEN_X11 (screen);
 +  g_return_if_fail (GDK_IS_SCREEN (screen));
====

This check falls into the general category of checks that we omit in
virtual functions (if this function got called with a non-screen,
something would be highly messed up internal to GObject); if it wasn't
omitted than it would need to go before:

 +  GdkScreenX11 *screen_x11 = GDK_SCREEN_X11 (screen);

Since checks on the validity of a parameter should always go before
usage of the parameter.

====
 +  g_return_if_fail (num_monitor < GDK_SCREEN_X11 (screen)->num_monitors);
 +  
 +  dest->x = screen_x11->monitors[num_monitor].x;
 +  dest->y = screen_x11->monitors[num_monitor].y;
 +  dest->width = screen_x11->monitors[num_monitor].width;
 +  dest->height = screen_x11->monitors[num_monitor].height;
  }
=====

You can write this more compactly as:

  *dest = screen_x11->monitors[num_monitor];

===
 --- tests/testxinerama.c	30 Apr 2002 18:32:08 -0000	1.2
 +++ tests/testxinerama.c	30 Apr 2002 19:50:08 -0000
 @@ -11,18 +11,19 @@
  {

    gchar *str;
    gint i = gdk_screen_get_monitor_at_window (gtk_widget_get_screen (widget),
 -					     GDK_WINDOW_XWINDOW (widget->window));
 +					     widget->window);
====

You can also remove the gdkx.h include from this file.

===
    num_monitors = gdk_screen_get_n_monitors (screen);
    if (num_monitors == 1)
 -    g_warning ("The current display has only one monitor.");
 +    g_warning ("The current display does not support xinerama.");
===

To me, the concept of "does not support Xinerama" isn't something we should
expose to the programmer or user. Either your screen has only one monitor,
or it has more than one monitor. One monitor is just the trivial special
case of "Xinerama", whether or not the extension is actually supported.

Regards,
                                        Owen