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

[Peter Hutterer]

On Tue, Jul 28, 2009 at 09:53:23PM -0700, Aaron Plattner wrote:
> ---
>  configure.ac   |    2 +-
>  fixesproto.txt |   41 +++++++++++++++++++++++++++++++++++++----
>  xfixesproto.h  |   27 +++++++++++++++++++++++++++
>  xfixeswire.h   |   27 +++++++++++++++++++++++++--
>  4 files changed, 90 insertions(+), 7 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 0d422d4..1c9136e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -24,7 +24,7 @@ dnl
>  dnl Process this file with autoconf to create configure.
>  
>  AC_PREREQ([2.57])
> -AC_INIT([FixesProto], [4.1], [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg])
> +AC_INIT([FixesProto], [5.0], [https://bugs.freedesktop.org/enter_bug.cgi?product=xorg])
>  AM_INIT_AUTOMAKE([foreign dist-bzip2])
>  AM_MAINTAINER_MODE
>  
> diff --git a/fixesproto.txt b/fixesproto.txt
> index ededc8a..57b4dbe 100644
> --- a/fixesproto.txt
> +++ b/fixesproto.txt
> @@ -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
>  			    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
> +
>  3. Basic Premise
>  
>  Requests in this extension may seem to wander all over the map of X server
> @@ -553,7 +555,38 @@ ShowCursor
>  	If the client has made no outstanding HideCursor requests
>  	a BadMatch error is generated.
>  
> -		
> +************* XFIXES VERSION 5 OR BETTER ***********
> +
> +12. Screen Layout
> +
> +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.
> +
> +12.1 Types
> +
> +	POSTYPE	{ PosAbsolute, PosLeftOf, PosAbove, PosBelow, PosRelative }
> +	LAYOUT	[
> +			x, y: CARD32		absolute screen origin position
> +			positionType: POSTYPE	relative position type
> +			relScreen: CARD16	relative screen
> +		]

I think the protocol spec needs to describe which values are valid in which
combination. Right now, I'm guessing that if PosAbsolute is the type, then
x/y is an absolute position relative to the virtual screen size (?) and and
position type is relative to the screen but ignores the coordinates? Point
is though, that I'm just guessing here.

I know that at least some of these combinations have been used in the
xorg.conf for a while now and may be considered common knowledge but
nonetheless it should be explicitly stated.

> +
> +12.2 Requests
> +
> +GetScreenLayout
> +
> +		->
> +		layout:			LISTofLAYOUT
> +
> +    The GetScreenLayout reply contains one layout description structure for each
> +    X screen.

Where do you get the number of screens from? IMO the request/reply should be
consistent within itself, i.e. the reply should tell you how many layouts
trail the reply (num_layouts or something). You can deduct that from the
length field, but having it explicitly listed doesn't hurt.

> +
> +    Errors: none
> +
>  99. Future compatibility
>  
>  This extension is not expected to remain fixed.  Future changes will
> diff --git a/xfixesproto.h b/xfixesproto.h
> index 545e325..074ee13 100644
> --- a/xfixesproto.h
> +++ b/xfixesproto.h
> @@ -2,6 +2,7 @@
>   * $XFree86: xc/include/extensions/xfixesproto.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
> @@ -500,6 +501,32 @@ typedef struct {
>  
>  #define sz_xXFixesShowCursorReq	sizeof(xXFixesShowCursorReq)
>  
> +/*************** 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;

What does screens resolve to? A pointer? or is it optimised away? This looks
a bit awkward to me, the traditional approach is to leave the trailing data
structures out of the structure. Removing the line should suffice, the
protocol spec specifies what's trailing.

> +
> +#define sz_xXFixesGetScreenLayoutReply sizeof(xXFixesGetScreenLayoutReply)
> +
>  #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>
> +
>  /*************** Version 1 ******************/
>  #define X_XFixesQueryVersion		    0
>  #define X_XFixesChangeSaveSet		    1
> @@ -85,8 +88,10 @@
>  /*************** Version 4 ******************/
>  #define X_XFixesHideCursor		    29
>  #define X_XFixesShowCursor		    30
> +/*************** Version 5 ******************/
> +#define X_XFixesGetScreenLayout		    31
>  
> -#define XFixesNumberRequests		    (X_XFixesShowCursor+1)
> +#define XFixesNumberRequests		    (X_XFixesGetScreenLayout+1)
>  
>  /* Selection events share one event number */
>  #define XFixesSelectionNotify		    0
> @@ -124,4 +129,22 @@
>  #define WindowRegionBounding		    0
>  #define WindowRegionClip		    1
>  
> +/*************** Version 5 ******************/
> +
> +/* Screen position information options */
> +
> +#define XFixesPosAbsolute		    0
> +#define XFixesPosRightOf		    1
> +#define XFixesPosLeftOf			    2
> +#define XFixesPosAbove			    3
> +#define XFixesPosBelow			    4
> +#define XFixesPosRelative		    5
> +
> +typedef struct {
> +    CARD32  x B32;
> +    CARD32  y B32;
> +    CARD16  positionType B16;
> +    CARD16  refScreen B16;
> +} xXFixesScreenLayoutRec;
> +

This struct is part of the protocol, not the client data. Should be moved to 
xfixesproto.h (You can remove the Xmd.h include then too)
This goes in hand with the libXfixes commit where this struct is simply
typedef'd, IMO wrapping it properly is better as it separates protocol and
library better.

>  #endif	/* _XFIXESWIRE_H_ */
> -- 
> 1.6.0.4

Cheers,
  Peter