[PATCH] Making MPX work with more than one screen (api breaks!)

[Peter Hutterer]

On 18/04/2007, at 23:00 , Paulo Ricardo Zanoni wrote:
> As suggested by whot at irc, I've changed all those broken parts to  
> use inputInfo.pointer as their pointer's current window. This  
> should be fixed in the future.

I finally found time to review your patch. here's my comments. I'm  
happy with the patch in general, few things to fix. If you can get a  
revised patch to me, I'll push it.

> diff --git a/XTrap/xtrapdi.c b/XTrap/xtrapdi.c
> index bc15bbd..f5106aa 100644
> --- a/XTrap/xtrapdi.c
> +++ b/XTrap/xtrapdi.c
> @@ -102,7 +102,7 @@ globalref int_function XETrapProcVector[
>  #ifndef VECTORED_EVENTS
>  globalref int_function EventProcVector[XETrapCoreEvents];
>  #else
> -extern WindowPtr GetCurrentRootWindow();
> +extern WindowPtr GetCurrentRootWindow(DeviceIntPtr);
>  globalref int_function EventProcVector[128L];
>  #endif
>  static int_function keybd_process_inp = NULL;  /* Used for  
> VECTORED_EVENTS */
> @@ -1620,7 +1620,10 @@ int XETrapEventVector(ClientPtr client,
>                  (x_event->u.u.type <= MotionNotify) &&
>                  (!x_event->u.keyButtonPointer.sameScreen)))
>              {   /* we've moved/warped to another screen */
> -                WindowPtr root_win = GetCurrentRootWindow();
> +		/* XXX: We're getting inputInfo.pointer here, but I guess
> +		 * it might be really wrong. Which mouse's current window
> +		 * do we want here? */
> +                WindowPtr root_win = GetCurrentRootWindow 
> (inputInfo.pointer);

If you're inside a function that knows about the client, use the  
PickPointer(client) or PickKeyboard(client) to get the device to work  
on.

> +#define rootWindow spriteTrace[0]

This is a matter of personal taste, but I'd prefer if you'd change  
the macro. I'm a fan of having macros that look like macros. So  
RootWindow(dev) which resolves to dev->whatever->whatever is fine.  
dev->rootWindow to be a macro to resolve to dev->spriteTrace seems  
confusing. One wonders why you'd have a macro if it's just a rename  
anyway.

also RootWindow(dev) is easier to read than dev->spriteInfo->sprite- 
 >rootWindow.

btw, if you can find a better name for spriteTrace, I'm happy to  
merge that too :)

>  static void
> @@ -974,9 +967,11 @@ PostNewCursor(DeviceIntPtr pDev)
>  }
>
>  _X_EXPORT WindowPtr
> -GetCurrentRootWindow(void)
> +GetCurrentRootWindow(DeviceIntPtr dev)
>  {
> -    return ROOT;
> +    /* Since there is more than one mouse, there is one current  
> root window for
> +     * each mouse */
> +    return dev->spriteInfo->sprite->rootWindow;
>  }

please put comments like this into the doxygen comments.

> @@ -2312,8 +2311,35 @@ InitializeSprite(DeviceIntPtr pDev, Wind
>      {
>          pSprite->current = wCursor(pWin);
>          pSprite->current->refcnt++;
> -    } else
> + 	pSprite->spriteTrace = (WindowPtr *)xalloc(32*sizeof(WindowPtr));
> +	/* FIXME; spriteTrace[0] needs to be NULL, otherwise
> +	* GetCurrentRootWindow() in EnableDevice() may return a
> +	* invalid value. (whot) */
> +	/* If I interpreted correctly what you wrote, I guess it's
> +	 * fixed (PauloZanoni). */
> +	memset(pSprite->spriteTrace, 0, 32 * sizeof(WindowPtr));
> +	if (!pSprite->spriteTrace)
> +	    FatalError("Failed to allocate spriteTrace");
> +	pSprite->spriteTraceSize = 32;
> +	memset(pSprite->spriteTrace, 0, 32 * sizeof(WindowPtr));

you are memsetting spriteTrace to 0 twice. Just use xcalloc, then you  
can skip the memsets.
you can remove my comment too :)

> ++	pSprite->rootWindow = pWin;
> +	pSprite->spriteTraceGood = 1;
> +
> +	/* Maybe the right option here should be 'pScreen', but since the
> +	 * enqueue/dequeue screens were alwyas initialized with
> +	 * screenInfo.screens[0], I'll keep with it. */
> +	pSprite->pEnqueueScreen = screenInfo.screens[0];
> +	pSprite->pDequeueScreen = pSprite->pEnqueueScreen;
> +
> +    } else {
>          pSprite->current = NullCursor;
> +	pSprite->spriteTrace = NULL;
> +	pSprite->spriteTraceSize = 0;
> +	pSprite->spriteTraceGood = 0;
> +	pSprite->pEnqueueScreen = screenInfo.screens[0];
> +	pSprite->pDequeueScreen = pSprite->pEnqueueScreen;
> +    }
>

In the original code, screens[0] was chosen because that's where all  
the sprites would end up initially. so using pScreen is fine as this  
should be the screen the sprite is on right now.

> @@ -2946,7 +2973,9 @@ #endif
>          {
>            xeviekb = keybd;
>            if(!rootWin) {
> -	      rootWin = GetCurrentRootWindow()->drawable.id;
> +	      /* XXX I guess this really doesn't make sense, but I'm not  
> sure
> +               * about what to do exactly! */
> +	      rootWin = GetCurrentRootWindow(keybd)->drawable.id;

you want the root window of the keyboard, cos you're using it to send  
focus event. so what you're doing seems good.
>

> @@ -4445,9 +4463,9 @@ #endif
>  void
>  CloseDownEvents(void)
>  {
> -  xfree(spriteTrace);
> -  spriteTrace = NULL;
> -  spriteTraceSize = 0;
> +    /* This function was used to free spriteTraces, but now they are
> +     * freed when SpriteRec is freed.
> +     * Should this function be removed? */
>  }

leave the stub, leave the comment, maybe add a X_DEPRECATED.

> diff --git a/dix/main.c b/dix/main.c
> index f5b89ba..d2bf57b 100644
> --- a/dix/main.c
> +++ b/dix/main.c
> @@ -498,7 +498,7 @@ #endif
>  	    FreeScreen(screenInfo.screens[i]);
>  	    screenInfo.numScreens = i;
>  	}
> -  	CloseDownEvents();
> +	CloseDownEvents();

remove the call if it's deprecated anyway.

> diff --git a/randr/rrpointer.c b/randr/rrpointer.c
> index fec5d45..8bb805e 100644
> --- a/randr/rrpointer.c
> +++ b/randr/rrpointer.c
> @@ -135,10 +135,17 @@ RRPointerMoved (ScreenPtr pScreen, int x
>  void
>  RRPointerScreenConfigured (ScreenPtr pScreen)
>  {
> -    WindowPtr	pRoot = GetCurrentRootWindow ();
> +    WindowPtr	pRoot = GetCurrentRootWindow (inputInfo.pointer);
>      ScreenPtr	pCurrentScreen = pRoot ? pRoot->drawable.pScreen :  
> NULL;
>      int		x, y;
>
> +    /* XXX: GetCurrentRootWindow now revices an argument, I've put
> +      * inputInfo.pointer, but I really think this is wrong...
> +     * What do we do here? I've made this so that it can compile,
> +     * but I don't think randr should assume there is just one  
> pointer.
> +     * There might be more than one pointer on the screen! So,  
> what to do?
> +     * What happens? */
> +
>      if (pScreen != pCurrentScreen)
>  	return;
>      GetSpritePosition(inputInfo.pointer, &x, &y);

generally one thing I also noted: when you write a comment, describe  
the __current state__ of the code, not it's history. most people  
(including me) wouldn't know what the code looked like before and  
even less why it looked the way it did. comments like "moved this  
from there because we changed the api and now it's not there anymore"  
don't help. comments in the form of "set this to NULL or hell will  
freeze over and we'll all need a new set of mittens" are helpful.

otherwise, good work!

Cheers,
   Peter

--
Multi-Pointer X Server
http://wearables.unisa.edu.au/mpx