partial-absolute-wheel-support #2669
Conversation
Sublimelime
left a comment
There was a problem hiding this comment.
Looks sane so far to me, pending someone who knows a bit more doing their review.
… comment to make purpose clearer, as suggested in review
… individual bindings (Like mousewheel), instead of piggybacking AuxCollection
…or counterclockwise
X9VoiD
left a comment
There was a problem hiding this comment.
Lookin real nice now! 🍞 Still some really smol stuff left but nothing that I can't fix myself before merge.
|
Slight update, now also supports just binding the Wheel as if it was a button. |
|
Revert the last commit. That'll only be confusing especially on tablets that do not support "sensing" when the wheel is touched but not rotated yet. |
This reverts commit d1fa24e.
|
And Done
Den søn. 2. apr. 2023 kl. 14.52 skrev X9VoiD ***@***.***>:
… Revert the last commit. That'll only be confusing especially on tablets
that do not support "sensing" when the wheel is touched but not rotated yet.
—
Reply to this email directly, view it on GitHub
<#2669 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABB7TZHFWDQ3BTAEABQO64TW7FZBTANCNFSM6AAAAAAWAJNEJY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Any update on this? |
| /// <param name="report">The report from the device</param> | ||
| /// <param name="bindings">The collection of bindings we are updating</param> | ||
| /// <param name="newStates">New states of the updated bindings</param> | ||
| /// <param name="offset">An offset into <paramref name="bindings"/> before <paramref name="newStates"/> applies</param> |
There was a problem hiding this comment.
This parameter is missing from the function, so this parameter annotation should be removed (or the functionality should be added to the function if still needed)
| /// <param name="offset">An offset into <paramref name="bindings"/> before <paramref name="newStates"/> applies</param> |
| }, | ||
| "AuxiliaryButtons": { | ||
| "ButtonCount": 12 | ||
| "ButtonCount": 13 |
There was a problem hiding this comment.
I don't think this should be changed. If it's incremented because it has a wheel that can be clicked this should be added as an attribute to the Wheel properties instead, e.g. IsClickable or similar naming.
There was a problem hiding this comment.
I received a reply on Discord:
Yeah, probably not actually related to the core changes, but the tablet does actually have a 13th button in the center of the wheel
I think I would still prefer wheel to have a separate declaration. Thoughts @X9VoiD ?
|
Looks generally pretty good to me, though I would have to play around with the PR myself and see how it behaves. |
This comment was marked as spam.
This comment was marked as spam.
|
This will be superseded by a forward-port of #3497 to ensure the codebase doesn't deviate too much. Thanks for the well-attempted contribution, and sorry for letting this hang for so long. |
This commit adds some support for tablets with rings (#1367), by treating the ring as a bunch of buttons.
It most certainly needs a second pass, but its far enough along that I would like some thoughts on the general approach/structure.
In short, this pull adds support for treating the ring as positions + 2 individual buttons, the first two buttons being clockwise and anticlockwise buttons. This can be configured in a new "Wheel" UI tab.
This is a different approach than what #2256 does, which I was not aware of when I started on this. A final solution probably includes content from both.