Bug 25915

Thanks for the patch, a few comments:
- coding style is incoherent with the rest of the driver. 4 spaces indent,
  no space after (, etc.
- 2 typos in the second hunk
- don't bother including XI2.h, just XI.h is enough. If that hunk is added,
  inputproto is required in configure.ac (see 6f4634111a83808bc52e7e53733cf2d3bab0cccd)
- the matching man page entry is missing from the patch
- please resubmit as signed-off, git formatted patch with a descriptive
  commit message about how the patch works.

+#define EVDEV_RELATIVE_MODE    (1 << 11) /* Force absplute events, if
device suporrt it */

huh? RELATIVE_MODE forces absolute events? this comment is confusing.

+   xf86Msg(X_INFO, "%s: force absolute mode\n", pInfo->name);

not really necessary, we expect the device to do what we tell it to, no
extra log message is necessary here. same for the relative one of course.

+    mode = xf86SetStrOption(pInfo->options, "Mode", "auto");

please don't add an "auto" mode. auto should be implicit if the option isn't
set.

+    if ( !strcmp("absolute", mode) )
+    {
+      xf86Msg(X_INFO, "%s: force absolute mode\n", pInfo->name);
+      pEvdev->flags &= ~EVDEV_RELATIVE_MODE;
+    }

strcasecmp, and again here the output is superfluous. the server will output
the option in the log anyway if it's given, so that just duplicates it.

Created attachment 32496 [details] [review]
Implements switch_mode for devices with absolute axis

I hope it's better now.

Thanks for the update, patch isn't quite there yet but it looks better already. There's a bunch of minor issues with the coding style, nothing with the approach itself.

- the new requires for inputproto should be mentioned in the commit message
- typo "derfault" in the man page.
- man page: "for touchpad" should be "for touchpads"
- man page: "sets the mode" -> "Sets the mode"
- man page: no comma in the first sentence, should explicitly mention that the option has no effect on devices without absolute axes.
- man page and EVDEV_RELATIVE_MODE define -> s/axis/axes/ when speaking of plural
- EvdevSwitchMode: squash pInfo and pEvdev assignment into the declaration, no need for the separate lines here
- EvdevSwitchMode: no spaces after/before () in if conditions
- as I said previously, please remove the xf86Msg, it's just spamming the log
- EvdevSwitchMode: don't return BadMatch, this function should return BadMode (see the XSetDeviceMode man page)
- EvdevAddAbsClass: no {} for single-line blocks
- EvdevAddAbsClass: else on same line as closing }
- EvdevAddAbsClass: no spaces in if condition
- EvdevAddAbsClass: tabs/spaces mixup in else block
- EvdevAddAbsClass: period after "using default" in the log message
- commit message should be present tense "Implement" instead of "Implements"

@@ -567,7 +599,6 @@ EvdevProcessKeyEvent(InputInfoPtr pInfo, struct input_event *ev)
             break;
~
         case BTN_TOUCH:
-            pEvdev->tool = value ? ev->code : 0;
             if (!(pEvdev->flags & (EVDEV_TOUCHSCREEN | EVDEV_TABLET)))
                 break;


This isn't supposed to be part of this patch, is it? It doesn't affect the mode handling at all.

Created attachment 32551 [details] [review]
[PATCH] Implement XSetDeviceMode request handler

(In reply to comment #4)
> Created an attachment (id=32551) [details]
> [PATCH] Implement XSetDeviceMode request handler
> 

thank you again. Two more changes, then it's ready.

- skip the block of returning BadMatch. BadMatch may only be returned if the device has no valuators and that's checked in the server.
- no spaces before/after "mode" in the switch statement.

FYI, see also this thread on the list, turns out the SwitchMode error reporting is buggy in the server.
http://lists.freedesktop.org/archives/xorg-devel/2010-January/004864.html

Created attachment 32583 [details] [review]
Implement XSetDeviceMode request handler

sorry for the delay. Pushed as e81cd935cfff18d3c387eed3e8083977c19c92f0. Thanks for your patience.