Re: Multiple Display support proposed Layout

[Havoc Pennington <hp redhat com>]

Hi,

I'll give you some initial comments, I think you'll also want a round
of comments from Owen at some point. Looks pretty good in general.

Erwann Chenede <Erwann Chenede ireland sun com> writes:
 
> e.g. for gdk_visual_init I've added a gdk_visual_init_screen
> variant, gdk_visual_init called only for the default version.
> See gdkmultihead.h for the "variant" signature I've already done.
> 
> So my question is : Is the naming convention I've used OK ? 

Our usual convention would be _with_screen() probably. So 
gdk_visual_init_with_screen(), etc.
 
Specific comments on the headers, mostly nitpicks, I'll leave any
large design comments to others.

> struct _GdkDisplay 
> {
>     GObject parent_instance;
>     
> };

Need to use 2-space indentation.

> 
> struct _GdkDisplayClass
> {
>     GObjectClass 	    parent_class;
>     GdkDisplay *	    (*new_display)	    (gchar *
> display_name);	    

Instead of doing this, make display_name a G_PARAM_CONSTRUCT_ONLY
object property. Look at gtk/gtktexttag.c for an example of using
G_PARAM_CONSTRUCT_ONLY. The issue here is that a call to g_object_new
(GDK_TYPE_DISPLAY, "name", ":0", NULL) should work, which means the
display object should be valid after the instance init function.

>     gchar *		    (*get_display_string)   (GdkDisplay *dpy);

get_display_name would be a better name I think.
	    
>     gint 		    (*screen_count)	    (GdkDisplay *dpy);
>     

get_n_screens is our usual convention.

> GdkDisplay *	    gdk_display_new		(void);

I'm guessing this function is never used by app writers, one just asks
the display manager to open the display. So probably just 
call g_object_new (GDK_TYPE_DISPLAY, NULL) in the display manager
implementation, and don't have this function.

> GdkDisplay *	    gdk_display_ref		(GdkDisplay* display);
> void		    gdk_display_unref		(GdkDisplay* display);
> 

These aren't needed, since you can use g_object_ref(). The other
ref/unref calls in GDK just exist because they predate those types
being GObject subclasses.
 
> struct _GdkDisplayMgr 

This name shouldn't be abbreviated - GdkDisplayManager. Long function
names are just inevitable in C, have to bite the bullet. ;-)

> {
>   GObject	    parent_instance;
>   GdkDisplay *	    default_display;
>   GSList *	    display_list;
> };

display_list is presumably just a list of open displays, right? Maybe
call it open_displays


>   gint			    (*get_num_display)  (GdkDisplayMgr* dpy_mgr);

I assume this returns the number of opened displays, so maybe call it 
get_n_open_displays or something like that. Though I'm not sure it's
useful to find out how many displays are open anyhow.
 
> GType		gdk_display_mgr_type	    (void);
> GdkDisplayMgr * gdk_display_mgr_new	    (void);
> /*GdkDisplayMgr * gdk_display_mgr_unref	    (GdkDisplayMgr * dpy_mgr);
> void	        gdk_display_mgr_ref	    (GdkDisplayMgr * dpy_mgr);*/
>

Presumably this is a singleton object, so maybe have
gdk_display_manager_get() instead of gdk_display_manager_new()?

Also, it looks like there's no gdk_display_manager_open(). That
function will need to be able to fail gracefully by returning NULL or
the like.

> GdkAtom
> gdk_atom_intern_display(const gchar * atom_name,
> 			gboolean only_if_exists,
> 			GdkDisplay * dpy);

Maybe we can avoid this; it could mean a lot of pain in end-user
apps. You could instead keep a mapping from a client-side GdkAtom
value that's the same for all displays to the real X value.  This
would break a few apps using Xlib directly, but not too big a deal.
 
> void
>  gdk_key_repeat_restore_display(GdkDisplay * dpy);

Don't bother with this, the key repeat stuff is deprecated and bad.

> void gdk_visual_add_screen(GdkVisual * visual, GdkScreen * scr);

Do screens contain visuals or vice versa?

> void gdk_query_depths_screen(gint ** depths,
> 			     gint * count,
> 			     GdkScreen * scr);

Here and in similar cases maybe a for_screen or of_screen would be
better than with_screen as the naming convention.

Havoc