[PATCH 1/3] fixesproto: Add a query to get the screen layout.

[Aaron Plattner]

On Tue, Jul 28, 2009 at 10:34:52PM -0700, Keith Packard wrote:
> * PGP Signed by an unknown key
> 
> On Tue, 2009-07-28 at 21:53 -0700, Aaron Plattner wrote:
> 
> > @@ -1,7 +1,7 @@
> >                          The XFIXES Extension
> > -			    Version 4.0
> > -			 Document Revision 1
> > -			     2006-12-14
> > +			    Version 5.0
> > +			 Document Revision 2
> > +			     2009-07-27
> 
> Document revision should go back to 0 for the new major version.

Ah, okay, thanks.  Will do.

> >  			    Keith Packard
> >  			  keithp at keithp.com
> >  
> > @@ -29,6 +29,8 @@ developers, in particular,
> >  
> >   +	Deron Johnson for cursor visibility
> >  
> > + +	Valery Moya for X screen origin queries
> 
> So, the first question is why this is in XFixes and not in RandR --
> should it go there instead?

I didn't think RandR really dealt with multiple screens, other than
allowing you to address them.  It really deals with the positions of
outputs within a single X screen, and not with the relative positions of
the X screens themselves.

Perhaps when we have our grand unified RandR/Xinerama/Shatter
implementation, it will solve all of these problems.  In the meantime, it
doesn't really matter where this request goes, it just doesn't really seem
like it belongs in RandR.

> > +X configurator programs may want to know the relative positions of X screens in
> > +a multiple screen layout.  This configuration is described in the ServerLayout
> > +section of xorg.conf and affects how the cursor moves from screen to screen when
> > +it hits screen edges.  Configuration programs need to know this information so
> > +they can display it to users and build configuration files from the current
> > +screen layout.
> 
> Is there some reason this doesn't also allow this data to be changed at
> run-time? From what I can see, that should be trivial and would
> eliminate yet another 'must restart' case in the X server.

Just poking the data into dixScreenOrigins wasn't enough to change the
cursor behavior.  I'd have to make more invasive updates into the rest of
the server and I didn't have the time to look at what would be required.

Do you think it would be worth adding a request to change it, and then just
return BadImplementation for now?

> > +/*************** Version 5.0 ******************/
> > +
> > +typedef struct {
> > +    CARD8   reqType;
> > +    CARD8   xfixesReqType;
> > +    CARD16  length B16;
> > +} xXFixesGetScreenLayoutReq;
> > +
> > +#define sz_xXFixesGetScreenLayoutReq sizeof(xXFixesGetScreenLayoutReq)
> > +
> > +typedef struct {
> > +    BYTE    type;   /* X_Reply */
> > +    BYTE    pad1;
> > +    CARD16  sequenceNumber B16;
> > +    CARD32  length B32;
> > +    CARD32  pad2 B32;
> > +    CARD32  pad3 B32;
> > +    CARD32  pad4 B32;
> > +    CARD32  pad5 B32;
> > +    CARD32  pad6 B32;
> > +    CARD32  pad7 B32;
> > +    xXFixesScreenLayoutRec screens[];
> > +} xXFixesGetScreenLayoutReply;
> > +
> > +#define sz_xXFixesGetScreenLayoutReply sizeof(xXFixesGetScreenLayoutReply)
> 
> You have to use an explicit size (no sizeof(foo)) for every request I'm
> afraid

Okay, will do.  I'll fix the rest of the requests in a separate change.

> -- and you can't use xXFixesScreenLayoutRec in the structure,
> that's not portable C. Looks like there are other errors of this type in

Lame!

> the file. The reason for explicit sizes is that some requests are *not*
> a multiple of 64-bits long, and so sizeof will give the wrong answer. Of
> course, I encourage you to make all requests a multiple of 64-bits so
> that Xlib doesn't have to insert Noops to align the output buffer.
> 
> > +
> >  #undef Region
> >  #undef Picture
> >  #undef Window
> > diff --git a/xfixeswire.h b/xfixeswire.h
> > index 6f20270..c8c0ca1 100644
> > --- a/xfixeswire.h
> > +++ b/xfixeswire.h
> > @@ -2,6 +2,7 @@
> >   * $XFree86: xc/include/extensions/xfixeswire.h,v 1.1 2002/11/30 06:21:43 keithp Exp $
> >   *
> >   * Copyright  2006 Sun Microsystems
> > + * Copyright  2009 NVIDIA Corporation
> >   *
> >   * Permission to use, copy, modify, distribute, and sell this software and its
> >   * documentation for any purpose is hereby granted without fee, provided that
> > @@ -47,9 +48,11 @@
> >  #define _XFIXESWIRE_H_
> >  
> >  #define XFIXES_NAME	"XFIXES"
> > -#define XFIXES_MAJOR	4
> > +#define XFIXES_MAJOR	5
> >  #define XFIXES_MINOR	0
> >  
> > +#include <X11/Xmd.h>
> 
> What is Xmd.h required for here? And how did it work without this
> before?

It was needed for CARD32/CARD16 in the xXFixesScreenLayoutRec structure.
It looks like I'm going to have to create a separate redundant
XFixesScreenLayout structure and add dumb translation code to the library,
so I'll move this into xfixesproto.h and get rid of the Xmd.h include.

> > +typedef struct {
> > +    CARD32  x B32;
> > +    CARD32  y B32;
> > +    CARD16  positionType B16;
> > +    CARD16  refScreen B16;
> > +} xXFixesScreenLayoutRec;
> 
> You'll probably want to make this a multiple of 64 bits long, and then
> also provide a sz_xXFixesScreenLayoutRec for the Xlib code to use.
> 
> Also, the tradition has been to use minimal datatypes for each object,
> so positionType should probably be a CARD8. You'll then need to place
> explicit padding entries to ensure that everything sits on a natural
> boundary and that the whole structure is a multiple of 64 bits long.

Okay.  Thanks for your feedback.