Bug 15961

Summary: DragLockButtons option has ceased functioning
Product: xorg Reporter: Tom Horsley <horsley1953>
Component: Input/evdevAssignee: Xorg Project Team <xorg-team>
Status: RESOLVED FIXED QA Contact: Xorg Project Team <xorg-team>
Severity: minor    
Priority: low CC: chrissalch
Version: git   
Hardware: All   
OS: All   
Whiteboard:
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 16699    
Attachments:
Description Flags
DragLock code
none
DragLock code
none
DragLockButtons code none

Description Tom Horsley 2008-05-16 06:36:27 UTC
I reported this against fedora 9, but as it is probably an upstream issue,
I figured I ought to add it here (if bugs.freedesktop.org stays up long
enough :-).

Details are at: https://bugzilla.redhat.com/show_bug.cgi?id=446780

The X log still reports the same message about making button 2
a drag lock, but when I click button 2, it acts like plain old
button 2, no drag locking is taking place.

This is monumentally painful when you use a trackball.

I'd be happy if this got added to the 7.4 blockers, because it will kill
me if it isn't fixed.

I'd also be happy to learn there is some completely new X input module
way I ought to specify drag lock, but I haven't found it if there
is one.
Comment 1 Peter Hutterer 2008-05-16 15:32:06 UTC
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.
Comment 2 Tom Horsley 2008-05-20 09:40:05 UTC
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 3 Peter Hutterer 2008-05-20 16:52:34 UTC
> --- 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.
Comment 4 Tom Horsley 2008-05-20 17:26:26 UTC
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.
Comment 5 Peter Hutterer 2008-05-27 22:17:15 UTC
Closing as invalid. This bug was caused by a mis-configuration.
Comment 6 Peter Hutterer 2008-07-14 00:52:21 UTC
Reopening as feature-request for evdev.
Comment 7 Chris Salch 2008-08-04 20:35:30 UTC
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 8 Chris Salch 2008-08-05 17:17:37 UTC
Comment on attachment 18119 [details] [review]
DragLock code

I'll resubmit this after the button path on 4147 is applied
Comment 9 Chris Salch 2008-08-07 19:44:31 UTC
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.
Comment 10 Peter Hutterer 2008-08-14 22:17:19 UTC
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?
Comment 11 Chris Salch 2008-08-17 10:45:02 UTC
(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.


Comment 12 Chris Salch 2008-08-17 10:47:36 UTC
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?
Comment 13 Peter Hutterer 2008-08-17 23:19:04 UTC
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.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.