I think you forgot to attach the patch :)

Created attachment 82171 [details] [review]
Syndaemon trackpoint monitoring

Well that was embarassing...

Created attachment 82173 [details] [review]
1-trackpoint-support

Created attachment 82175 [details] [review]
[minor] deleting commented code..

Comment on attachment 82173 [details] [review]
1-trackpoint-support

Review of attachment 82173 [details] [review]:
-----------------------------------------------------------------

I like the idea, the patch itself needs some work though:
* please double-check all indentations and spacing to match the rest of the file (4-space indentation, spaces around operators, etc)
* make the device variable, so that the trackpoint is the default but a user can supply another device name
* skip the device type check when searching for the device - the name is enough.
* CursorPosition should be a static struct, not a pointer (currently you don't free it anywhere either)

The approach itself works, but I think there's a better way. As of a few server versions ago, each device has it's own idle timer now. So what we should be doing here is monitor the idle counter on that device and determine activity based on that. We don't use the idle counter for keyboard activity because of the -K/-k flags but for this approach it would work nicely.

The side-effect of such an approach would be that we can allow multiple -T arguments, and it would work for both -R and polling mode. I'll attach a little sample script to show how idle timers can be used, that should give you enough of a start.

Created attachment 82201 [details]
test program for device idle counters

Thanks for the feedback (and the script). I'll work on that. 

I had initially tried a static struct but my C skills are quite rusty and wasn't sure what the best way to initialize (whilst not having to check for each property's value?) was... hence a null pointer, which I'm also not freeing (ouch!). I guess if I set the first property to -1, it would do the job. However, using the device idle time seems like a much better approach and makes this unnecessary.

I've just noticed there are quite a few number of device names:  
 > MatchProduct	"TPPS/2 IBM TrackPoint|DualPoint Stick|Synaptics Inc. Composite TouchPad / TrackPoint|ThinkPad USB Keyboard with TrackPoint|USB Trackpoint pointing device"

So now I'm hesitant to make any of these a default (I reckon any device that has "Trackpoint" in the name, but I'd rather not go for such a wildcard). I'll make the TTPS/2 Trackpoint the default for now and accept any other device name as a variable.

I'll put some more time on this later this week. Thanks for the help :)

(In reply to comment #7)
> I had initially tried a static struct but my C skills are quite rusty and
> wasn't sure what the best way to initialize (whilst not having to check for
> each property's value?)

static structs/variables are always initialized to zero, but you can do 
static CursorPosition a = {-1, -1}; as well.

> I've just noticed there are quite a few number of device names:  
>  > MatchProduct	"TPPS/2 IBM TrackPoint|DualPoint Stick|Synaptics Inc.
> Composite TouchPad / TrackPoint|ThinkPad USB Keyboard with TrackPoint|USB
> Trackpoint pointing device"
> 
> So now I'm hesitant to make any of these a default (I reckon any device that
> has "Trackpoint" in the name, but I'd rather not go for such a wildcard).
> I'll make the TTPS/2 Trackpoint the default for now and accept any other
> device name as a variable.

A wildcard is the ideal default, IMO because it can be documented well enough and it is just the default (for the flag). so syndaemon -T matches *TrackPoint* etc., -T "foo device" matches only "foo device". I think that's a good approach, don't you?

Apologies for the late reply.

I like the wildcard approach, it matches a wide range of devices. I've used "trackpoint"  (case-insensitive regex) to match the device name. One problem: the getopt() two colon syntax for optional arguments is a GNU extension - I believe this will be an issue on other platforms (bsd, darwin)? Having -T match *trackpoint* and -T "device-name" match "device-name", without getopt_long or getopt with ::, has proven non-trivial for me! (which says a lot of my C fluency).

The second issue is with the sync extensions. Every change in state in any of the buttons changes the device state:

> $ xinput query-state "TPPS/2 IBM TrackPoint"
> 2 classes :
> ButtonClass
> 	button[1]=up
> 	button[2]=up
> 	button[3]=down/down
> 	button[4]=up
> 	button[5]=up
> 	button[6]=up
> 	button[7]=up
> ValuatorClass Mode=Relative Proximity=In
> 	valuator[0]=446
> 	valuator[1]=1489

(unless there's a way to watch certain values of the device only, maybe via XSyncWaitCondition?)

So, that leaves me more or less where I was. I've made the changes suggested in trackpoint_activity() and I'm using regcomp/regexec to match the device name.

the last paragraph of the getopt man page DESCRIPTION section may help (just above getopt_long). If the first character of optstring is ":", then getopt returns ':' if an argument is missing. Look at the man page example, change optstring to ":nt:" and then add               
    case ':':
         printf("missing argument for: %c\n", optopt);
         break;
This handles the case we need, without needing GNU extensions. Hope that makes it clearer


(In reply to comment #9)
> The second issue is with the sync extensions. Every change in state in any
> of the buttons changes the device state:
> 
> (unless there's a way to watch certain values of the device only, maybe via
> XSyncWaitCondition?)

I'm not sure I understand the question here. each device has an idlecounter that is reset as soon as the device sends events. for this here to work is you'd put a negative transition counter on the trackpoint and disable the touchpad whenever the idle counter hits 0. and then re-enable it whenever the idle counter goes past the timeout (positive transition). Is that what you're doing at the moment? If you're unsure, attach the patch(es) you already have and I can have a look.

Created attachment 94095 [details] [review]
Proposed patch for syndaemon

Proposed patch for adding trackpoint handling to syndaemon.
Depends on the other patches in #74544 to build properly.

Comment on attachment 94095 [details] [review]
Proposed patch for syndaemon

Review of attachment 94095 [details] [review]:
-----------------------------------------------------------------

Thanks for the patch! A few comments, see below:

You can skip the configure.ac bit. libXext is pulled in by libXi which is unconditional. It'd be enough to just add 'xext' explicitly to the PKG_CHECK_MODULES(XI... line.
That also means you can skip the #ifdefs for sync.h, it's available in every version we depend on.

Adding trackpoint monitoring to the non-record code would be simple enough too - just add a XPending() XNextEvent() before/after keyboard_activity() and handle it like with the xrecord parts.

For the select code, please calculate max_fd first instead of that ternary condition. Since the fd's don't change anyway, this will make the code easier to read.
In the alarm event handling code, I'd like an if condition for the second alarm too and an error if neither is hit, because that would indicate a bug that shouldn't happen.

pstick_name should be const char* to avoid errors.

+ set_idle_state_option(enum TouchpadIdleMode mode, const char * state) 
why not let that return the idle mode (or -1 on error), rename it to "..parse..." and the code would be
  idle_state[KeyboardIdle] = parse_idle_mode(argv[optind]);
I think that's easier to understand.

+ if (pstick_id == 0) {
just print an error of ("couldn't find pointing stick %s", pstick_name)

I wonder if it'd be simpler to keep two idle state variables for keyboard and pstick and then calculate the touchpad mode based on them. So instead of set_touchpad_mode(KeyboardIdle), you'd have
   tp_state[KeyboardIdle] = Idle;
   update_touchpad_state();
And in there you look at both values and decide on the actual state we want. This would provide us with a nice matrix of what to set when which is a lot easier follow (and document :) than the current code.

Other general notes:
- commit messages should generally be as an action, i.e. "Add support for monitoring...", since that's what the patch does. Minor nitpick, not the end of the world. We do require you sign-off your patch though
- I'm beginning to think we should add proper option parsing instead of a quite confusing array of shortopts. Something like --monitor=keyboard,trackstick or so would be a lot more obvious that knowing that -P disables -M unless -R...
- I'd make the -I default time to be identical to the -i time instead of a separate default. I'm not sure we even need a separate -I, at least not for now. Benjamin mentioned in an email that we probably need to make the trackstick mode without a timeout - i.e. it stays on until the touchpad is used as touchpad again (would require some more driver cooperation). So for now, I'd say take the -I parts out and just re-use the keyboard idle time, we can sort the rest out later

(In reply to comment #12)
> Comment on attachment 94095 [details] [review] [review]
> Proposed patch for syndaemon
> 
> Review of attachment 94095 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch! A few comments, see below:
> 
> You can skip the configure.ac bit. libXext is pulled in by libXi which is
> unconditional. It'd be enough to just add 'xext' explicitly to the
> PKG_CHECK_MODULES(XI... line.
> That also means you can skip the #ifdefs for sync.h, it's available in every
> version we depend on.
> 
> Adding trackpoint monitoring to the non-record code would be simple enough
> too - just add a XPending() XNextEvent() before/after keyboard_activity()
> and handle it like with the xrecord parts.
> 
> For the select code, please calculate max_fd first instead of that ternary
> condition. Since the fd's don't change anyway, this will make the code
> easier to read.
> In the alarm event handling code, I'd like an if condition for the second
> alarm too and an error if neither is hit, because that would indicate a bug
> that shouldn't happen.
> 
> pstick_name should be const char* to avoid errors.
> 
> + set_idle_state_option(enum TouchpadIdleMode mode, const char * state) 
> why not let that return the idle mode (or -1 on error), rename it to
> "..parse..." and the code would be
>   idle_state[KeyboardIdle] = parse_idle_mode(argv[optind]);
> I think that's easier to understand.
> 
> + if (pstick_id == 0) {
> just print an error of ("couldn't find pointing stick %s", pstick_name)
> 
> I wonder if it'd be simpler to keep two idle state variables for keyboard
> and pstick and then calculate the touchpad mode based on them. So instead of
> set_touchpad_mode(KeyboardIdle), you'd have
>    tp_state[KeyboardIdle] = Idle;
>    update_touchpad_state();
> And in there you look at both values and decide on the actual state we want.
> This would provide us with a nice matrix of what to set when which is a lot
> easier follow (and document :) than the current code.
> 
> Other general notes:
> - commit messages should generally be as an action, i.e. "Add support for
> monitoring...", since that's what the patch does. Minor nitpick, not the end
> of the world. We do require you sign-off your patch though
> - I'm beginning to think we should add proper option parsing instead of a
> quite confusing array of shortopts. Something like
> --monitor=keyboard,trackstick or so would be a lot more obvious that knowing
> that -P disables -M unless -R...
> - I'd make the -I default time to be identical to the -i time instead of a
> separate default. I'm not sure we even need a separate -I, at least not for
> now. Benjamin mentioned in an email that we probably need to make the
> trackstick mode without a timeout - i.e. it stays on until the touchpad is
> used as touchpad again (would require some more driver cooperation). So for
> now, I'd say take the -I parts out and just re-use the keyboard idle time,
> we can sort the rest out later

Regarding the changes you want, I'll fix up the patch and resubmit it asap.
Regarding the options, yeah, I agree we should add some proper functions. Once I fix up the original patch, I'll work on adding that in a subsequent patch (or I can work it into this patch if you want).

Created attachment 94850 [details] [review]
Proposed patch for syndaemon

Adds pointing stick support to syndaemon.

Created attachment 94851 [details] [review]
Proposed patch for syndaemon

Updated patch to be against latest master

Created attachment 94854 [details] [review]
Proposed patch for syndaemon

Used the wrong timeout value with poll(), causing it to return instantly. Somehow I managed not to notice this when I was testing the changes. Fixed now.

Created attachment 94855 [details] [review]
Proposed patch for syndaemon

Please excuse me, this sort of thing usually doesn't happen
Fixed -P only working if -M was enabled.

Created attachment 94858 [details] [review]
0001-Add-support-for-monitoring-pointing-sticks.patch

I figured it's easier and quicker if I just hack a bit instead of trying to explain everything I wanted :)

Here's a new diff, changes to your last version:
* re-localised a few variables that you made global
* changed tp_state and last_tp_state to is_idle and was_idle, makes it easier to read (IMO, anyway)
* changed update_touchpad_state() to check all permutations, I find this easier to read
* extra check to ignore non-alarm events
* fixed the sleep() back to usleep(). sleeping for 200s is not useful ;)
* fixed a crasher if -T was given without an option
* removed a hunk of duplicate code in main(), looked like a copy/paste error
* moved setup_xsync to before issuing the actual counters
* unconditionally check for alarm events - the pollfd is the same that xlib uses, so Xlib may read the events off the fd to get to the XQueryKeymap replies. Then poll won't trigger, so we never notice the trackstick movements.

I've tested this in normal mode, but still needs more testing, especially in record mode (haven't tested that at all yet). Please give it a try

Created attachment 94895 [details] [review]
Proposed patch for syndaemon

Looks like in the ternary statement on line 674 you forgot to change that when you changed tp_state to is_idle, so it was only setting the timeout when there wasn't activity and as a result getting stuck on mode 1. Fixed that in the patch, other then that it seems to work perfectly on my system.

Created attachment 94995 [details] [review]
0001-Add-support-for-monitoring-pointing-sticks.patch

Two minor changes: the timespec handling wasn't correct for some cases, added helper functions to handle this for us. And replaced a if condition with the #define max()

Otherwise it seems to work fine here.

Created attachment 95285 [details] [review]
0002-syndaemon-exit-if-the-device-idle-timer-failed-to-in.patch

will be squashed with the other patch, but separate to make for an easier review.

Created attachment 95286 [details] [review]
0003-syndaemon-when-failing-to-find-the-default-trackstic.patch

Don't fail when we don't find the default trackstick. This makes starting syndaemon in a generic manner easier, since can just provide -P without having to check first if a trackstick is present.

Created attachment 95313 [details] [review]
Alternative to patching syndaemon

Fixes the top soft button behavior, making TrackPoints without physical buttons easily usable without requiring syndaemon.

GNOME bug filed here: https://bugzilla.gnome.org/show_bug.cgi?id=726011

Created attachment 95552 [details] [review]
Alternative to patching syndaemon

Inhibits movements by changing the use_cumulative_coords variable instead of running a check when ignore_motion is set.

Created attachment 95564 [details] [review]
Alternative to patching syndaemon [1/2]

Split the commit into two before applying some other changes at the request of whot

Created attachment 95565 [details] [review]
Alternative to patching syndaemon [2/2]

Now ignores movement starting from the top soft button area regardless of where it ends, in case the user accidentally moves their finger outside of the top soft button area when making a click with their trackpoint. Idea originally from Hans.

Created attachment 95627 [details] [review]
Alternative to patching syndaemon [1/2]

Rebased against lastest upstream

Created attachment 95629 [details] [review]
Alternative to patching syndaemon [2/2]

Changed back to original behavior with ignoring movement starting in the top soft buttons area at the request of Hans de Geode. Also rebased against the latest upstream master.

with current git master 17bbcad28000bbd896a33047c0720ada89e05f5d no patches to syndaemon should be necessary. I'm going to close this bug as WONTFIX, we'll re-open if the current solution proves unworkable.