Introduce new LongPress plugin #1423
Conversation
|
As written above I have adapted the configuration to provide long-press behaviour on I saw different approaches to achieve this. The first one was to provide different configuration options for It leads to duplicated data store and methods in the code and (more importantly) harder to read LongPress configuration. Therefore I adapted it to have only one config object (LongPressMapping) that allows either a the first one would take precedence over the second one (long-pressing would produce a This could be handled by sorting the configured LongPressMappings to always put the configuration on KeyAddr first, regardless of the order they were defined in. I already tried that for this PR, but unfortunately failed on the nitty-gritty details of C++ for applying a qsort() on the list of LongPressMappings. The second (uncompilable) commit shows what I tried. It fails with: So apparently the Can you please help me on how to change it to correctly sort that list. Or do you have some other idea on how to solve the basic problem? |
|
Maybe I am overthinking this a bit. I am running this LongPress plugin now for a few days (without the last commit regarding sorting of course) and am rather happy with it. I even have such a combination of configuration on What do you think? |
|
I think that having the documentation clearly state which one "wins" is just great. |
hupfdule
force-pushed
the
longpress-plugin
branch
from
b42888b
to
2333dd8
Compare
|
(It does look like the README needs to be updated away from 'AutoShift' to 'LongPress') |
Yes, I am working on that right now. Will write a note here when I am done with it. |
|
Do have some hints on how I can resolve the failing tests? |
|
do those tests pass for you locally? Are the timings the tests show right? |
For some reason I can’t execute the tests locally. I activated them in the Github Actions of my fork and they fail in the same way. Apparently the testing framework doesn’t work as I expect. The failing tests all test the case that no long-press behaviour has been defined for a key and a long-press does not produce a different key than a short tap. But this seems to produce these timing problems. |
|
I'd love to help get the simulator tests working locally for you. That'll make testing much less painful. What OS are you on? How does 'make simulator-tests' fail? |
|
I have now updated the README to correctly describe the LongPress plugin. At the same time I have moved the I also rename the I leave these changes as separate commits for now for easier tracking of the changes. Before this plugin gets merged I would like to squash it into a single commit. But we need to resolve the failing test beforehand anyway… |
It’s a bit complicated… I am using a Debian Linux. But that one is running inside a virtual machine on a Windows host. I will try to avoid docker and install the necessary packages locally to run the tests without docker. I will get back to you when I have done that. But this will be tomorrow. It is 22:45 here and I need to get to bed ;-) |
|
"make simulator-tests" on debian is going to hurt a lot less than running
it in docker. (It's also going to be *much* faster)
Sleep well.
…On Tue, May 21, 2024 at 1:46 PM hupfdule ***@***.***> wrote:
I'd love to help get the simulator tests working locally for you. That'll
make testing much less painful. What OS are you on? How does 'make
simulator-tests' fail?
It’s a bit complicated… I am using a Debian Linux. But that one is running
inside a virtual machine on a Windows host.
As I don’t want to clutter my OS with too many tools only for testing I
tried to use make docker-simulator-tests, but it fails in a very strange
way. It always fails when trying to install the necessary tools for
building the docker container. It seems that it is able to download some of
the packages, but at some point the network connection collapses and it
runs into a timeout. What is even more strange is that this broken network
connection is not only broken inside the docker container, but also in my
Debian OS, although it automatically comes back without intervention in
less than a minute.
I will try to avoid docker and install the necessary packages locally to
run the tests without docker. I will get back to you when I have done that.
But this will be tomorrow. It is 22:45 here and I need to get to bed ;-)
—
Reply to this email directly, view it on GitHub
<#1423 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAALC2FI4HEEDZIY2FRIYGTZDOXDJAVCNFSM6AAAAABH4VWBOKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRTGQYTINJWGA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
I just tried it, but running the tests does not work: and so on. When trying it to restrict it to testing the LongPress plugin it fails differently: And it goes on like this and seems to never stop. |
|
@hupfdule - what debian are you on? I'm very happy to set up a VM to try to replicate this, since we should at least be failing more sanely. |
|
I was just starting to dig into the fails, but I see that you're actively pushing updates. |
It’s a normal Debian Bookworm. However, the result is the same. The tests fail with a timestamp deviation. @obra Are you able to help me correct these tests? I describe my intentions for this example (I have reduced it to the minimum): In this case I want to test that the plugin has no effect if a key is only tapped instead of long-pressed. However, the test fails with this message: I played around a bit with the order of the test statements, but I just don’t understand what is the problem here. |
Ah yes, sorry. Just found the problem with two of them. But the last one is still a mystery to me. |
|
This is the test script as I'm testing: With this, I get the same results you do. Digging into how to debug this: .ktest files are transformed into googletest files. In this case, The generated content of that is The That's defined in In this case, a keyboard report is at issue: The issue we're running into is that the...first? key report is being outputted 5ms later than implied by the test's code. And thinking about it a little bit... LongPress would have to be delaying that initial keypress of A until after it figures out it's a tap, right? It shouldn't be sending output to the host until it's decided what to do. It looks like what it's doing is waiting until the switch is released and then sending both usb key reports in the same device cycle, right? |
|
Many thanks for your help and the thorough explanation!
You are absolutely right. It was a misinterpretation on my side. My mistake was that I was thinking like that key would not be configured at all in the LongPress plugin. But actually it is (auto-shifting is active for all letter keys). And in that case, the plugin needs to wait until either the long-press timeout occurs (20ms) or the key has been released again. If I change the test case to use Key_1 (which has no configured long-press behaviour in this test case) the test runs correctly like defined above. The plugin does not affect the key at all and therefore the key report is sent after the first cycle. Many thanks! I will now fix the test case, squash the commits and then the PR should be ready for review. |
hupfdule
force-pushed
the
longpress-plugin
branch
from
e9e0acc
to
0eaa93e
Compare
|
Hmm, unfortunately there is still a problem. Compared to the AutoShift plugin I moved one class out of the main plugin into a separate header file (and a separate namespace). I have done this to not clutter the main LongPress.h file with too much auto-shifting functionality and therefore improve its readability. Compilation of Kaleidoscope with But when calling It seems that the namespace cannot be used as I intended. Can someone suggest on how to resolve this? Why does it fail only when compiling the simulator-tests, but not on a normal build? Any help is appreciated! |
|
If your code is all pushed up, I'll try to have a look at it this evening. |
Yes it is. Many thanks in advance! |
|
So sorry. I didn't manage this before the weekend. looking now. |
|
Ok. I think the issue you're running into is that you've named your header file the same thing as the header in the AutoShift plugin. And so GCC can't figure out which one you mean. If you rename AutoShift.h to LongPressAutoShift.h, everything works. |
hupfdule
force-pushed
the
longpress-plugin
branch
from
0eaa93e
to
3cc5c9c
Compare
|
Many thanks! I have changed that. |
| LongPress.setAutoshiftEnabled(LongPress.letterKeys()); | ||
|
|
||
| LONGPRESS( | ||
| // ATTENTION! The order matters here! |
There was a problem hiding this comment.
Instead of saying the order matters, perhaps explain how it matters?
| To define the keys that should behave differently on long-press use include a definition like the following: | ||
|
|
||
| ```c++ | ||
| LONGPRESS( |
There was a problem hiding this comment.
Perhaps a naive question - how does the configuration by KeyAddr interact with multiple layers on the keyboard? Is long press behavior scoped to a single layer or does it override the behavior no matter what layer someone is on?
Ideally, long-press behavior would be configurable by layer, I think.
|
Added a couple of notes/questions. In general, the code is well documented and well commented and appears pretty well tested. I think the only big thing is understanding how the configuration by KeyAddr interacts with layers. If it is the case that keyaddr based configuration currently ignores the layer system, that's probably worth getting described in the README. I think that it would be better if keyaddr based configuration respected layers, but I'd be ok with that being a "version 2" feature. |
Yes, the LongPress plugin currently ignores the layers. And I agree that it may be desirable to be able to apply it to specific layers. I will look into it and then make a proposal of whether to try to include it from the start or postpone it to a later version. Many thanks for your review! I write here when I have reworked it. One question: Do you prefer that I do the changes in separate commits for easier review of those changes or shall I squash them into a the already existing commit? |
|
I don't have strong opinions on you rewriting the history while you're
working. Whatever ends up easier for you.
…-Jesse
On Thu, May 30, 2024 at 12:04 AM hupfdule ***@***.***> wrote:
Added a couple of notes/questions. In general, the code is well documented
and well commented and appears pretty well tested. I think the only big
thing is understanding how the configuration by KeyAddr interacts with
layers. If it is the case that keyaddr based configuration currently
ignores the layer system, that's probably worth getting described in the
README. I think that it would be *better* if keyaddr based configuration
respected layers, but I'd be ok with that being a "version 2" feature.
Yes, the LongPress plugin currently ignores the layers. And I agree that
it may be desirable to be able to apply it to specific layers. I will look
into it and then make a proposal of whether to try to include it from the
start or postpone it to a later version.
Many thanks for your review! I write here when I have reworked it.
One question: Do you prefer that I do the changes in separate commits for
easier review of those changes or shall I squash them into a the already
existing commit?
—
Reply to this email directly, view it on GitHub
<#1423 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAALC2GPR4M433FXFJZA2QDZE3FOBAVCNFSM6AAAAABH4VWBOKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZYHAZDCMBZHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
hupfdule
force-pushed
the
longpress-plugin
branch
from
3cc5c9c
to
e00f189
Compare
I looked over it and it seemed rather easy and straight-forward to support restricting it to specific layers. I would also like to introduce that now as it has an influence on the public API. I changed it now to allow to restrict the long-press behaviour to a single layer. Other than requested by you I did not only do this for mappings on
It is still possible to apply these mappings to all layers. There are two options to do this (as also described in the plugins README now):
So these are the possibilities for configuring a long-press key: LONGPRESS(
// Key at 1,0 should produce a Z on long press on all layers
kaleidoscope::plugin::LongPressMapping( KeyAddr(1, 0), Key_Z),
// Key at 1,0 should produce a Z on long press on all layers
kaleidoscope::plugin::LongPressMapping(ALL_LAYERS, KeyAddr(1, 0), Key_Z),
// Key at 1,0 should produce a Z on long press on the first layer
kaleidoscope::plugin::LongPressMapping(0, KeyAddr(1, 0), Key_Z),
// Keys generating a B should produce a Y on long press on all layers
kaleidoscope::plugin::LongPressMapping( Key_B, Key_Y),
// Keys generating a B should produce a Y on long press on all layers
kaleidoscope::plugin::LongPressMapping(ALL_LAYERS, Key_B, Key_Y),
// Keys generating a B should produce a Y on long press on the first layer
kaleidoscope::plugin::LongPressMapping(0, Key_B, Key_Y),
)I have one additional question: |
|
I have also updated the README and the tests to cover the restriction to a single layer. So please have another look on it now. |
I'd probably keep it as a namespaced constant for now until we're more sure how we'll use it globally. In the code, I see this: I think that's probably not the case currently? Also, the README I'm seeing doesn't seem to have the updates for always having layer number defined? Are you fully pushed up? |
hupfdule
force-pushed
the
longpress-plugin
branch
from
e00f189
to
bd0b058
Compare
This commit provides a new plugin “LongPress” that allows producing different Keys when keys are held for a short time instead of only tapped. It is based on the existing “AutoShift” plugin and contains its functionality, but extends it for a broader area of application. Signed-off-by: Marco Herrn <[email protected]>
hupfdule
force-pushed
the
longpress-plugin
branch
from
bd0b058
to
a1fb23d
Compare
That would be a rather long and unwieldy name for such an optional constant. Therefore I decided to only use it internally. It means that to apply a LongPressKey to all layers the layer must be omitted (see the README). While the constant If we in the future decide to advertise that constant, no change to the public API is necessary. That is even the case if this constant is being moved to
Yes, that was a leftover from playing around with it. I have removed that reference now.
It is mentioned here as an example and later in text form. Everything is pushed now again. Please have a look. |
|
I feel like an optional first parameter is going to lead to user confusion. Is the most common use-case here really "all layers"? I'm having trouble envisioning when users would really want that as the default behavior. Can you talk about the use cases you're envisioning? If the "all layers" case is relatively rare, having it be a longer constant seems preferable to me, since the behavior feels like it could end up somewhat surprising. |
|
Hmm, actually I don’t think it is confusing to have a first optional parameter. We could put it to the end of the parameterlist, but that would be different than the configuration in other plugins (and in fact I would expect it to be the first parameter) which I think would be even more confusing. I think just omitting it is clearly enough indicating that is applied to all layers. It can be expressed more clearly in the documentation though. I am also not very keen on making it a mandatory parameter. You are absolutely right that the most common use case (as long as I imagine them) is applying a LongPress configuration to a specific layer (for example to generate a different printable key) and only rarely on all layers (for example to execute a common key like Esc or Enter or some key sequence like Ctrl-C). OK, so we have expressed our arguments/reasoning here. I think it is up to you now to decide what to do. I will then change the code accordingly. |
|
(Sorry I'm running slow. This week has been busy.)
actually I don’t think it is confusing to have a first optional
parameter. We could put it to the end of the parameterlist, but that would
be different than the configuration in other plugins (and in fact I would
expect it to be the first parameter) which I think would be even more
confusing. I think just omitting it is clearly enough indicating that is
applied to all layers. It can be expressed more clearly in the
documentation though.
The problem is that it's not immediately obvious that it's missing when
it's missing. I had to read your examples multiple times before I noticed
the leading optional parameter.
I do genuinely believe that for the most part, omitting the layer number is
going to be something done unintentionally by a user, rather than
intentionally. I'd rather the less common case that isn't what users are
likely to want is the one that requires some positive action to invoke.
I agree with you that sticking the layer last instead of first is not great
because it loses parallelism with other bits of the API.
Especially since this name is then part of the public API, which would
mean that changing that later on to a public constant (without an explicit
namespace) would be a breaking change (making that unlikely to happen).
It doesn't need to be a breaking API change to also expose an "all layers"
constant globally -- If we do that, it'd be very reasonable to then make
the LongPress version of the constant an alias to the new global constant
and to leave it there ~forever.
Given that the global use case is relatively rare, one option we haven't
considered yet is to break it out as a separate method that isn't too long,
doesn't require an ugly constant and is clearly different. Like
GlobalLongPressKey...Although, looking at the current code examples, I'm
trying to find other examples where plugin configs are sticking multiple
methods into the Kaleidoscope::plugin:: namespace for sketches and not
seeing a lot. Thoughts?
…On Tue, Jun 4, 2024 at 8:01 AM hupfdule ***@***.***> wrote:
Hmm,
actually I don’t think it is confusing to have a first optional parameter.
We could put it to the end of the parameterlist, but that would be
different than the configuration in other plugins (and in fact I would
expect it to be the first parameter) which I think would be even more
confusing. I think just omitting it is clearly enough indicating that is
applied to all layers. It can be expressed more clearly in the
documentation though.
I am also not very keen on making it a mandatory parameter. You are
absolutely right that the most common use case (as long as I imagine them)
is applying a LongPress configuration to a specific layer (for example to
generate a different printable key) and only rarely on all layers (for
example to execute a common key like Esc or Enter or some key sequence like
Ctrl-C).
Still it is not really nice to need to specify that long constant in that
case. Especially since this name is then part of the public API, which
would mean that changing that later on to a public constant (without an
explicit namespace) would be a breaking change (making that unlikely to
happen).
OK, so we have expressed our arguments here. I think it is up to you now
to decide what to do. I will then change thi code accordingly.
—
Reply to this email directly, view it on GitHub
<#1423 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAALC2AA5WKK2DF47DE4VOTZFXJDRAVCNFSM6AAAAABH4VWBOKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBXG43DCNBRGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Yes, it seems that isn’t used in other plugins at the moment. I would say there are mostly these categories of typical possible configurations Configuration on
Looking at some of them I see: Qukeys
Qukeys are always configured on CharShift
CharShift is always configured on While I could imagine a similar functionality as CharShift to be applied to Chords
Chords are always configured on I think it may also be useful to restrict chords to certain layers, but MagicCombo
MagicCombos are always configured on MagicCombo seems to be obsoleted by Chords (I cannot imagine any use case ShapeShifter
ShapeShifter is always configured on Basically ShapeShifter is similar to CharShift with a different approach While I can imagine it being restricted to certain layers I am not sure if “Conclusion”I don’t see a similar example where a plugin allows configuration per layer What I do see clearly by looking at all these plugins is that nearly all of Looking into the alternatives of either a I am sorry, I have written a long sermon now, but still have no conclusion. So (again) sorry, I couldn’t bring the discussion further here. |
|
It's going to take me a bit of time to properly digest this, but this is awesome. You have nothing to apologize for here. I would be absolutely delighted for us to figure out a good pattern for this kind of thing so that at least all new plugins could match it (and then maybe we could backfill matching apis for everything else) Thank you. I really appreciate all the effort you've put into this so far. |
This PR replaces #1340 by providing a new “LongPress” plugin instead of
modifying the exisiting “AutoShift“ plugin.
It is still based on AutoShift, but has a few adaptations:
keys. Autoshifting is only one such use case (with special support for).
shift” is only used where such functionality is affected.
via
enableAutoshift(…)orsetAutoshiftEnabled(…).Compared to PR #1340 there are also some changes:
KeyAddrand for aKey. Both variants make sense for different use cases. For example formirroring the number row (generating a
0by long-pressing1) it makessense to configure that behaviour for a specific
KeyAddras the samelong-press behaviour may not be appropriate for the separate NumPad layer.
On the other hand to generate an
ëby long-pressingeis likelyconfigured on
Key_E, regardless of which physical key that is generatedwith.
The PR is not finished yet. For example the documentation for LongPress in
not yet written. Also support for configuration via Chrysalis is not yet
provided as it is unclear to me, what needs to be done there. Also it probably
needs some cleanup (like additional comments).
But it is in a state to be reviewed.
There are a few things I still need help with (see below).