[PATCH 2/2] XKB: Add debug key actions for grabs & window tree

[Daniel Stone]

Hi,

On Thu, Jun 16, 2011 at 11:01:58AM +1000, Peter Hutterer wrote:
> On Tue, Jun 14, 2011 at 07:30:49PM +0100, Daniel Stone wrote:
> > Add four new private XKB actions for debugging:
> >     * PrGrbs: print active grabs to the log file
> >     * DeaGrb: deactivate active grabs
> 
> compat/xfree86 still has the stuff for "Ungrab" and "ClsGrab".
> 
> #define XF86XK_Ungrab           0x1008FE20   /* force ungrab */
> #define XF86XK_ClearGrab        0x1008FE21   /* kill application with grab */
> 
> we should keep using them if possible.

Sounds good to me, as well as the two new keysyms you posted.  I've
updated the commit message to reflect these.

> Also, everywhere in the protocol and man pages we use "Ungrab" instead of
> "deactivate", it'd be better to use that terminology here too anyway.

Your wish is my command.  I've also renamed KilGrb to ClsGrb, in line
with the old XFree86-Misc action.

> > +void
> > +PrintGrabInfo(void)
> > +{
> > +    DeviceIntPtr dev;
> > +    ClientPtr client;
> > +    LocalClientCredRec *lcc;
> > +    int i, j;
> 
> A "Printing active grabs" here would be nice. It would tell us whether there
> simply are no grabs or whether this function isn't actually called.

Hm, all the callers already do this, except PrGrbs, which I've now
fixed.

> > +    for (dev = inputInfo.devices; dev; dev = dev->next)
> > +    {
> > +        if (!dev->deviceGrab.grab)
> > +            continue;
> 
> using a tmp variable for dev->deviceGrab would be good here

Fair, done.

> > +        DebugF("[dix] Active grab 0x%lx (%s) on device '%s' (%d):",
> 
> any reason you use DebugF here but ErrorF in the print window tree patch?
> it's unlikely that users reporting stuck grabs will have debugging enabled.

Fixed.

> > +        if (dev->deviceGrab.grab->deviceMask)
> > +            DebugF("      xi1 event mask 0x%lx\n",
> > +                   dev->deviceGrab.grab->deviceMask);
> 
> only for implicit passive grabs, otherwise eventMask is the device's grab
> mask see the massive comment above struct _GrabRec in inputstr.h

I see.  I've just changed the conditional to
if (devGrab->implicitGrab && grab->deviceMask) - does that suit?

> > +        DebugF("      owner-events %s, kb %d ptr %d, confine %x, cursor 0x%x\n",
> > +               dev->deviceGrab.grab->ownerEvents ? "true" : "false",
> > +               dev->deviceGrab.grab->keyboardMode,
> > +               dev->deviceGrab.grab->pointerMode,
> > +               dev->deviceGrab.grab->confineTo ?
> > +                dev->deviceGrab.grab->confineTo->drawable.id :
> > +                0,
> > +               dev->deviceGrab.grab->cursor ?
> > +                dev->deviceGrab.grab->cursor->id :
> > +                0);
> 
> using a tmp variable for dev->deviceGrab.grab would be nice.

Yep, again. ;)

> > +void
> > +KillGrabbingClients(void)
> > +{
> > +    DeviceIntPtr dev;
> > +    ClientPtr client;
> > +
> > +    DebugF("[dix] Killing all clients with active grabs, listed below:\n");
> 
> these really needs to be ErrorF. same with the one above. both
> cause weird behaviour in applications so a little warning is always nice to
> have.

Good point.

> > @@ -29,6 +30,14 @@ XkbDDXPrivate(DeviceIntPtr dev,KeyCode key,XkbAction *act)
> >              xf86ProcessActionEvent(ACTION_PREV_MODE, NULL);
> >          else if (strcasecmp(msgbuf, "+vmode")==0)
> >              xf86ProcessActionEvent(ACTION_NEXT_MODE, NULL);
> > +        else if (strcasecmp(msgbuf, "prgrbs")==0)
> > +            PrintGrabInfo();
> > +        else if (strcasecmp(msgbuf, "deagrb")==0)
> 
> s/deagrb/ungrab/

Done.

> > +            DeactivateAllGrabs();
> > +        else if (strcasecmp(msgbuf, "kilgrb")==0)
> 
> s/kilgrb/clsgrb/
> 
> so we can re-use the existing ones

Ah yep, done.

v2 inbound shortly.

Cheers,
Daniel