Add configurable button hold mappings

[barroit]

Add configurable mappings that trigger secondary button actions after a hold threshold.
This solves #138 (comment)

• Trigger hold actions as soon as the threshold expires
• Decouple event dispatch from per-device motion state
• Rename device event structures and fields for clarity
• Fix millisecond conversion for motion repeat timeouts
• Document button hold configuration

Example: https://github.com/barroit/etc/blob/fea3c3053a05a81c03ba1b3f0efdebced8bd339a/spacenav/spnavrc

[jtsiomb]

Thanks, I'm very much in favor of merging this feature, but might need some changes first. The most important one is that I don't like the first renaming commit, because it swamps the actual changes in a lot of noise of renaming everywhere. Drop that so that I can review the changes without distractions, and we can discuss if it's better to rename some fields later after the main thing gets merged.

[jtsiomb]

I'd prefer "holdmap" instead of "hdmap", or even better "bnhold".

[barroit]

I'd prefer "holdmap" instead of "hdmap", or even better "bnhold".

Will rename it to bnhold.

[jtsiomb]

I don't understand the renaming of dev_event to dev_event_ctx. This structure represents a device event.

[barroit]

I don't understand the renaming of dev_event to dev_event_ctx. This structure represents a device event.

I’ll drop the dev_event rename. Should we keep the field renames? The original names are a bit ambiguous.

[jtsiomb]

not in this pull request. Keep the changes and open a new one afterwards to consider them, or if you prefer discuss in an issue.

[jtsiomb]

Also for future reference, the repeat interval calculation fix should be a separate pull request. That one should be merged immediately regardless of all the rest.

[barroit]

Also for future reference, the repeat interval calculation fix should be a separate pull request. That one should be merged immediately regardless of all the rest.

Will split this.

[jtsiomb]

From a quick look in the github diff viewer it looks pretty good. I'll pull into a local branch and do a proper review as soon as possible.

[jtsiomb]

Ok I took a closer look, and it still looks good. But I have a couple of questions/notes:

It looks like the only thing this button hold mapping does, is to remap the button to a different button, which is probably not very useful by itself. I thought the main use case of button hold would be to assign either an action, or a keypress.

I see in your example configuration file that you're mapping buttons to arbitrary non-existent button numbers, and you have some fixed meaning for each one of them, which is an interesting use case, but entirely unintended and works by accident. The way button remapping is supposed to work, is as a way to swap existing buttons, it was never meant to generate buttons beyond what spnav_dev_buttons returns to applications, and I would expect some applications to break with such a config. The button numbers were not checked for validity, because the remapping feature was implemented before the number of device buttons became available to applications, or was even known by spacenavd itself, which used to just forward whatever the device reported without further checking.

Having said that. I don't mind merging a first version of the button hold feature limited to remapping buttons, but it should eventually be generalized. It could even be useful to pass long-press events to applications, but this will need some thought.

Now about the specific implementation in your pull request. The code is very well written, and could be merged as is. My only concern is that all this timeval math feels a bit awkward. Wouldn't it be better to deal with millisecond timeouts internally, and convert to timeval as needed for select?

[barroit]

Now about the specific implementation in your pull request. The code is very well written, and could be merged as is. My only concern is that all this timeval math feels a bit awkward. Wouldn't it be better to deal with millisecond timeouts internally, and convert to timeval as needed for select?

Most existing time values in the library are stored as timeval, and we get the current time using gettimeofday(). For that reason, I think keeping this as timeval is simpler and more consistent than converting between timeval and milliseconds in multiple places.

We could move the timeval arithmetic into helper functions to make it less awkward. But if you still prefer milliseconds, I'll change it.

[jtsiomb]

Most existing time values in the library are stored as timeval, and we get the current time using gettimeofday().

I don't think that's true. grepping for timeval in spacenavd, brings up two select invocations, a function for converting to millisecond intervals, and the dev_event structure, which is the only place a timeval is stored (to be later converted to milliseconds).

We could move the timeval arithmetic into helper functions to make it less awkward. But if you still prefer milliseconds, I'll change it.

If you really believe that the code will be simpler with timeval operations, leave it, and I'll experiment myself at a later time to see if I can simplify it or not. But from what I saw, changing the deadlines (would holdtime be a better name for this?) to milliseconds immediately removes TIMERCMP, simplifies the code in process_input, and next_button_timeout. Even the invocation to select will be simpler, because we're already converting the repeat interval to a timeval, so the comparison with the button timeout could just be done in milliseconds, before the conversion.

But I'll leave it up to your judgment. Either way when you're ready to merge, rebase your changes onto the current head (which has your millisecond conversion), make any final minor changes (see below), and let me know when you're ready to merge.

Possible minor changes:

• rename deadline to holdtime or hold_time, or hold_timeout, or something like that.
• consider moving the TIMERCMP operation argument in the middle (TIMERCMP(a, <=, b))
• change the comment in spnavd.c to reflect that it's not just the repeat interval that select will be waiting for any more.

[barroit]

Did the changes and rebased onto HEAD.

Recently I got my SpaceMouse Enterprise device, and it didn't work with Blender on Ubuntu. The key mapping is broken and lacks the hold feature. So I made this patch.

The new feature is implemented quite crudely. I'm not familiar with this kind of event design and didn't fully grasp the concepts, thus didn't implement it as a proper event. Instead, I just did a simple key remap like bnmap. It works, though.

[jtsiomb]

Yeah that looks fine, I'll do some minor formatting changes myself and merge

[jtsiomb]

merged

[barroit]

Thanks for the review and merge!