Bug 15961

The problem you have is caused by input device hotplugging. The DragLock is     
implemented in the mouse driver, but when the device is then (automatically)    
added through evdev it is removed from /dev/input/mice.                     
So although the mouse driver still listens with draglock on, no event ever is
sent to /dev/input/mice.                       
                                                                                
Option "AutoAddInputDevices" False                                              
in your ServerLayout section should "fix" this.

Almost right :-). It needs to be in ServerFlags and is spelled
"AutoAddDevices", not "AutoAddInputDevices" (at least according to
my xorg.conf man page).

It did seem to work once I got it turned on, but I have a feeling on
my system at home which has a USB KVM switch where the devices really
do get hotplugged, I'll have a bigger problem :-).

If I change the InputDevice section to explicitly use the evdev
driver and specify the device via the name string option, can I
also include drag lock options and have it work even with hotplugging?
(I suppose I can experiment when I get home and see).

Is there some way I can also get the effect of xmodmap for swapping
button numbers around in an evdev input device definition?

> --- Comment #2 from Tom Horsley <tomhorsley@adelphia.net>  2008-05-20 09:40:05 PST ---
> Almost right :-). It needs to be in ServerFlags and is spelled
> "AutoAddDevices", not "AutoAddInputDevices" (at least according to
> my xorg.conf man page).

whoops. sorry :)

> It did seem to work once I got it turned on, but I have a feeling on
> my system at home which has a USB KVM switch where the devices really
> do get hotplugged, I'll have a bigger problem :-).

do they actually get hotplugged every time?
 
> If I change the InputDevice section to explicitly use the evdev
> driver and specify the device via the name string option, can I
> also include drag lock options and have it work even with hotplugging?
> (I suppose I can experiment when I get home and see).

the name option doesn't exist/work in evdev 1.2 and above. Use Option "Device"
"/dev/input/by-id/whatever" instead. Evdev itself doesn't do draglock yet.
 
> Is there some way I can also get the effect of xmodmap for swapping
> button numbers around in an evdev input device definition?

xmodmap -e "pointer = 3 2 1" shold just work.

Apparently it works OK with the KVM after all. I guess maybe the KVM
is pretending they are still there (or maybe the X server doesn't even
notice they are gone if also isn't looking for them to show up :-).

I was doing xmodmap, and it still works, but I thought maybe there
was a replacement since I keep reading that xmodmap is obsolete.
Maybe it isn't obsolete for button mapping, but only keyboard
mapping?

Anyway, thinks for the info.

Closing as invalid. This bug was caused by a mis-configuration.

Reopening as feature-request for evdev.

Created attachment 18119 [details] [review]
DragLock code

Here's a possible implementation of drag locking.  This patch is standalone but could rely on the ButtonToNumbers function that I mentioned in bug #4147.  In fact, I'm not sure of a sane way to make this happen without something like that.

(Hopefully I kept this patch a good deal cleaner than what I was sending to 4147.)

It the Either of the ButtonToNumbers suggestions is accepted, I will correct this and resubmit.

Comment on attachment 18119 [details] [review]
DragLock code

I'll resubmit this after the button path on 4147 is applied

Created attachment 18179 [details] [review]
DragLock code

Here's an attempt at draglock code.  This was a reimplementation rather than port from the mouse driver.  I did borrow the pertenant portions of the mouse driver man page though.

hehe, funny how the option parsing is more complicated than the rest of the
feature :)

ACK if you address the following (mostly style) issues.

+    if (option_string) {
+	int meta_button = 0;
+       [foo]

I'm a fan of early returns (or gotos if necessary), it makes the code easier
to read with lesser indentations.  In this case, 

+    if (!option_string)
+       return;
+ 
+    [foo]

This is personal preference, I'll leave it up to you to decide.

+	char *end_str = 0;

char *end_str = NULL; is the convention, likewise with all the if conditions.
Please fix this.

+		    if ((meta_button <= 32) && (meta_button >= 0 ) &&
+			(lock_button <= 32) && (lock_button >= 0)) {

s/32/EVDEV_MAXBUTTONS/g

+			/* Let the user no something was wrong
+			   with this pair of buttons */

s/no/know/
(there are a couple of other typos, this was just an obvious one)

+/* Clean up after ourselves */
+void
+EvdevDragLockUnInit(InputInfoPtr pInfo)
+{
+
+}

if it doesn't do anything, just leave it out.

+void
+EvdevDragLockLockButton(InputInfoPtr pInfo, unsigned int button)
+{
+    EvdevPtr pEvdev = (EvdevPtr)pInfo->private;
+    BOOL state=0;
+
+    xf86Msg(X_INFO, "%s: Locking button %i\n", pInfo->name, button);

spamming the log file each time we use the button is probably not a good idea.
remove this line.

+    /* update button state */
+    state = TRUE - pEvdev->dragLock.lock_state[button - 1];

sheesh. I know it works, but let's treat a bool as a bool. Either we use TRUE,
or we use 1, but please don't use TRUE as a number.


+BOOL
+EvdevDragLockFilterEvent(InputInfoPtr pInfo, unsigned int button, int value)

please add a comment what it does, and when it returns TRUE/FALSE. you can
prob. just copy the one from emuMB.c anyway.

hunk 1 of evdev.c is noise.

+		xf86Msg(X_INFO,"%s: Button Press %i\n",pInfo->name, button);

no log spamming please.

+            {
                 EvdevMBEmuPreInit(pInfo);
+		EvdevDragLockInit(pInfo);
+            }
             device->public.on = TRUE;

tabs/spaces thing?

(In reply to comment #10)
> hehe, funny how the option parsing is more complicated than the rest of the
> feature :)

Yeah, that was the fun part of this little toy.  Having the code to convert button events to button numbers made the actual functionality nearly trivial ;)

> ACK if you address the following (mostly style) issues.
> 
> +    if (option_string) {
> +       int meta_button = 0;
> +       [foo]
> 
> I'm a fan of early returns (or gotos if necessary), it makes the code easier
> to read with lesser indentations.  In this case, 
> 
> +    if (!option_string)
> +       return;
> + 
> +    [foo]

Makes sense.  Since there is no special clean up to be done, your method will actually bring this code better into spec.

> 
> +       char *end_str = 0;
> 
> char *end_str = NULL; is the convention, likewise with all the if conditions.
> Please fix this.

Drat!  I think I changed all of these. 

> 
> +                   if ((meta_button <= 32) && (meta_button >= 0 ) &&
> +                       (lock_button <= 32) && (lock_button >= 0)) {
> 
> s/32/EVDEV_MAXBUTTONS/g

This should be taken care now.

> 
> +                       /* Let the user no something was wrong
> +                          with this pair of buttons */
> 
> s/no/know/
> (there are a couple of other typos, this was just an obvious one)

This one should be fixed.

> 
> +/* Clean up after ourselves */
> +void
> +EvdevDragLockUnInit(InputInfoPtr pInfo)
> +{
> +
> +}
> 
> if it doesn't do anything, just leave it out.

I had originally thought there might be a need for this and forgot to pull it out.  This should be gone now.

> 
> +void
> +EvdevDragLockLockButton(InputInfoPtr pInfo, unsigned int button)
> +{
> +    EvdevPtr pEvdev = (EvdevPtr)pInfo->private;
> +    BOOL state=0;
> +
> +    xf86Msg(X_INFO, "%s: Locking button %i\n", pInfo->name, button);
> 
> spamming the log file each time we use the button is probably not a good idea.
> remove this line.

Testing code I missed.  Removed.

> 
> +    /* update button state */
> +    state = TRUE - pEvdev->dragLock.lock_state[button - 1];
> 
> sheesh. I know it works, but let's treat a bool as a bool. Either we use TRUE,
> or we use 1, but please don't use TRUE as a number.

This has been switched with the trinary operator to make things clearer.  

> 
> 
> +BOOL
> +EvdevDragLockFilterEvent(InputInfoPtr pInfo, unsigned int button, int value)
> 
> please add a comment what it does, and when it returns TRUE/FALSE. you can
> prob. just copy the one from emuMB.c anyway.

That should be done.


> hunk 1 of evdev.c is noise.
> 
> +               xf86Msg(X_INFO,"%s: Button Press %i\n",pInfo->name, button);
> 
> no log spamming please.

Another line of testing code I missed.  Removed.

> 
> +            {
>                  EvdevMBEmuPreInit(pInfo);
> +               EvdevDragLockInit(pInfo);
> +            }
>              device->public.on = TRUE;
> 
> tabs/spaces thing?
> 

The tabs there and in draglock.c should be replaced with spaces now.

Created attachment 18329 [details] [review]
DragLockButtons code

Here's the patch.  Hopefully, this is cleaner.  As a note, I would like to add the property code in with this as well but I'm not sure how to test any property functionality.  What would you suggest on that front?

Pushed as bd405ddc83b9ad1ceed47f572245fccae598e6bb. Thanks again for the
patches.
Property support pushed as 92c6611b6f4495103fccea38dcafc6c6bf18049a.

> Here's the patch.  Hopefully, this is cleaner.  As a note, I would like to add
> the property code in with this as well but I'm not sure how to test any
> property functionality.  What would you suggest on that front?

You'd need a x server from master to get property support, it's not in a
released version yet.