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

[Erwann Chenede <Erwann Chenede Sun COM>]

Hi Owen,

>
>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.

Thanks for the review, I've changed the patch to integrate your comments  
and committed it.

[...]

>
> +  gint          (*get_monitor_at_point)  (GdkScreen    *screen,
> +					  gint          x,
> +					  gint          y);
>
>Hmmm, strikes me now that this one should probably move into gdkscreen.c

I've logged a bug against that issue (#80480)

>====
>        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)'.

On solaris the code either return 0 for failure or the number of framebuffers. 
I've changed it 'if (result == 0)'

[...]


>===
>    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.

Ok, but here it is just a warning in a test program :)

> 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.

Yes, but running this test app on a multi screen machine without xinerama support,
you'll get this warning, which is a bit odd when you have (like me) two or more 
separate screens on a single display.

Regards,
		Erwann
	
 Erwann Chénedé, Sun Microsystems, Ireland
 Phone  : +353 1 8199031         xt: 19031
[ I speak for myself, not for my employer ]