keyboardio
Introduce new LongPress plugin
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:
- The main focus of functionality is defining long press behaviour for keys. Autoshifting is only one such use case (with special support for).
- Some methods have been renamed to reflect this change. The term “auto shift” is only used where such functionality is affected.
- Autoshifting is not enabled by default anymore, but needs to be activated
via
enableAutoshift(…)orsetAutoshiftEnabled(…).
Compared to PR #1340 there are also some changes:
- It is possible to define long-press behaviour for a
KeyAddrand for aKey. Both variants make sense for different use cases. For example for mirroring the number row (generating a0by long-pressing1) it makes sense to configure that behaviour for a specificKeyAddras the same long-press behaviour may not be appropriate for the separate NumPad layer. On the other hand to generate anëby long-pressingeis likely configured onKey_E, regardless of which physical key that is generated with.
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).
As written above I have adapted the configuration to provide long-press behaviour on KeyAddr as well as on Key objects. The idea is that configuration on KeyAddr is the most specific one and should override behaviour defined on Key (and long-press configuration on Key should override auto-shifting).
I saw different approaches to achieve this. The first one was to provide different configuration options for KeyAddr and Key. It would look something like this in the .ino file:
LONGPRESS_ADDR(
kaleidoscope::plugin::LongPressAddr(KeyAddr(1, 0), Key_Y),
[…]
)
LONGPRESS_KEY(
kaleidoscope::plugin::LongPressKey(Key_E, Key_Z),
[…]
)
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 KeyAddr or a Key to be configured on. This is what is currently implemented in this PR. It is much cleaner in my opinion, but has one serious drawback. It is dependend on the order the LongPressMappings are defined in. So the note above about configuration on KeyAddr always having priority over configuration on Key does not apply. The first configuration will win. For example for a keyboard where all keys are defined as Key_E and the following LongPress configuration:
LONGPRESS(
kaleidoscope::plugin::LongPressMapping(KeyAddr(1, 1), Key_Y),
kaleidoscope::plugin::LongPressMapping(Key_E, Key_Z),
)
the first one would take precedence over the second one (long-pressing KeyAddr(1, 1) would produce a Y instead of a Z. But changing the order to:
LONGPRESS(
kaleidoscope::plugin::LongPressMapping(Key_E, Key_Z),
kaleidoscope::plugin::LongPressMapping(KeyAddr(1, 1), Key_Y),
)
would produce a Z in all cases, even on KeyAddr(1, 1).
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:
In file included from /home/mherrn/projekte/Kaleidoscope/plugins/Kaleidoscope-LongPress/src/Kaleidoscope-LongPress.h:20:0,
from /home/mherrn/projekte/Kaleidoscope/examples/Keystrokes/LongPress/LongPress.ino:5:
/home/mherrn/projekte/Kaleidoscope/plugins/Kaleidoscope-LongPress/src/kaleidoscope/plugin/LongPress.h: In instantiation of 'void kaleidoscope::plugin::LongPress::configureLongPresses(const kaleidoscope::plugin::LongPressMapping (&)[_explicitmappings_count]) [with unsigned char _explicitmappings_count = 2]':
/home/mherrn/projekte/Kaleidoscope/examples/Keystrokes/LongPress/LongPress.ino:69:3: required from here
/home/mherrn/projekte/Kaleidoscope/plugins/Kaleidoscope-LongPress/src/kaleidoscope/plugin/LongPress.h:309:13: error: invalid conversion from 'const void*' to 'void*' [-fpermissive]
qsort(explicitmappings_,
^~~~~~~~~~~~~~~~~
In file included from /home/mherrn/projekte/Kaleidoscope/.arduino/user/hardware/keyboardio/avr/cores/keyboardio/Arduino.h:23:0,
from /tmp/kaleidoscope-mherrn/build/4140205584-LongPress.ino/sketch/LongPress.ino.cpp:1:
/home/mherrn/projekte/Kaleidoscope/.arduino/data/packages/arduino/tools/avr-gcc/7.3.0-atmel3.6.1-arduino7/avr/include/stdlib.h:185:13: note: initializing argument 1 of 'void qsort(void*, size_t, size_t, __compar_fn_t)'
extern void qsort(void *__base, size_t __nmemb, size_t __size,
^~~~~
In file included from /home/mherrn/projekte/Kaleidoscope/plugins/Kaleidoscope-LongPress/src/Kaleidoscope-LongPress.h:20:0,
from /home/mherrn/projekte/Kaleidoscope/examples/Keystrokes/LongPress/LongPress.ino:5:
/home/mherrn/projekte/Kaleidoscope/plugins/Kaleidoscope-LongPress/src/kaleidoscope/plugin/LongPress.h:309:12: error: invalid conversion from 'int (*)(kaleidoscope::plugin::LongPressMapping, kaleidoscope::plugin::LongPressMapping)' to '__compar_fn_t {aka int (*)(const void*, const void*)}' [-fpermissive]
qsort(explicitmappings_,
~~~~~^~~~~~~~~~~~~~~~~~~
length,
~~~~~~~
sizeof(explicitmappings_[0]),
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
LongPressMapping::compare);
~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/mherrn/projekte/Kaleidoscope/.arduino/user/hardware/keyboardio/avr/cores/keyboardio/Arduino.h:23:0,
from /tmp/kaleidoscope-mherrn/build/4140205584-LongPress.ino/sketch/LongPress.ino.cpp:1:
/home/mherrn/projekte/Kaleidoscope/.arduino/data/packages/arduino/tools/avr-gcc/7.3.0-atmel3.6.1-arduino7/avr/include/stdlib.h:185:13: note: initializing argument 4 of 'void qsort(void*, size_t, size_t, __compar_fn_t)'
extern void qsort(void *__base, size_t __nmemb, size_t __size,
^~~~~
So apparently the explicitmapping array being defined as const prohibits applying the sort operation directly (there may be more errors regarding pointer/reference handling). I don’t grok C++ enough to be able to resolve this issue.
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 KeyAddr and on Key. I don’t think it is that bad to let the earlier defined mappings “win” over the later defined ones. It needs to be documented, for sure. But maybe that is the better approach.
What do you think?
I think that having the documentation clearly state which one "wins" is just great.
(It does look like the README needs to be updated away from 'AutoShift' to 'LongPress')
(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.
https://github.com/keyboardio/Kaleidoscope/actions/runs/9169594412/job/25244886228?pr=1423#step:7:1632
do those tests pass for you locally? Are the timings the tests show right?
https://github.com/keyboardio/Kaleidoscope/actions/runs/9169594412/job/25244886228?pr=1423#step:7:1632
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. This one here presses C, then waits 20ms (the time which is needed to trigger the long-press behaviour) and then tests that the usual Key_C has been produced. But that apparently does not work like I expected. It seems that these 20ms are exactly the difference in the expected an actual timestamp.
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 AutoShiftCategory class to a separate namespace in a separate file to enhance readability of the main plugin class.
I also rename the LongPressMapping struct to LongPressKey. I think that is easier to read and understand by users of the plugin.
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…
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 ;-)
"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 https://github.com/keyboardio/Kaleidoscope/pull/1423#issuecomment-2123414560, 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:
$ make simulator-tests
Building in quiet mode. For a lot more information, add 'VERBOSE=1' to the beginning of your call to make
make -C tests all
make[1]: Entering directory '/home/mherrn/projekte/Kaleidoscope/tests'
make -C /home/mherrn/projekte/Kaleidoscope/testing/googletest/build
[ 25%] Built target gtest
[ 50%] Built target gmock
[ 75%] Built target gmock_main
[100%] Built target gtest_main
compile /home/mherrn/projekte/Kaleidoscope/_build/keyboardio_virtual_model01/obj/AbsoluteMouseReport.o
In file included from /home/mherrn/projekte/Kaleidoscope/src/kaleidoscope/device/virtual/Virtual.h:25,
from /home/mherrn/projekte/Kaleidoscope/src/kaleidoscope/device/device.h:55,
from /home/mherrn/projekte/Kaleidoscope/src/kaleidoscope/KeyAddr.h:19,
from /home/mherrn/projekte/Kaleidoscope/src/kaleidoscope/Runtime.h:21,
from /home/mherrn/projekte/Kaleidoscope/testing/AbsoluteMouseReport.cpp:24:
/home/mherrn/projekte/Kaleidoscope/plugins/Kaleidoscope-Hardware-Model01/src/Kaleidoscope-Hardware-Model01.h:20:10: fatal error: Kaleidoscope-Hardware-Keyboardio-Model01.h: No such file or directory
20 | #include "Kaleidoscope-Hardware-Keyboardio-Model01.h" // IWYU pragma: keep
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[3]: *** [/home/mherrn/projekte/Kaleidoscope/testing/makefiles/libcommon.mk:37: /home/mherrn/projekte/Kaleidoscope/_build/keyboardio_virtual_model01/obj/AbsoluteMouseReport.o] Error 1
make[2]: *** [/home/mherrn/projekte/Kaleidoscope/testing/makefiles/testcase.mk:107: /home/mherrn/projekte/Kaleidoscope/_build/keyboardio_virtual_model01/lib/libcommon.a] Error 2
make -C /home/mherrn/projekte/Kaleidoscope/testing/googletest/build
[ 25%] Built target gtest
[ 50%] Built target gmock
[ 75%] Built target gmock_main
[100%] Built target gtest_main
compile /home/mherrn/projekte/Kaleidoscope/_build/keyboardio_virtual_model01/obj/AbsoluteMouseReport.o
In file included from /home/mherrn/projekte/Kaleidoscope/src/kaleidoscope/device/virtual/Virtual.h:25,
from /home/mherrn/projekte/Kaleidoscope/src/kaleidoscope/device/device.h:55,
from /home/mherrn/projekte/Kaleidoscope/src/kaleidoscope/KeyAddr.h:19,
from /home/mherrn/projekte/Kaleidoscope/src/kaleidoscope/Runtime.h:21,
from /home/mherrn/projekte/Kaleidoscope/testing/AbsoluteMouseReport.cpp:24:
/home/mherrn/projekte/Kaleidoscope/plugins/Kaleidoscope-Hardware-Model01/src/Kaleidoscope-Hardware-Model01.h:20:10: fatal error: Kaleidoscope-Hardware-Keyboardio-Model01.h: No such file or directory
20 | #include "Kaleidoscope-Hardware-Keyboardio-Model01.h" // IWYU pragma: keep
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[4]: *** [/home/mherrn/projekte/Kaleidoscope/testing/makefiles/libcommon.mk:37: /home/mherrn/projekte/Kaleidoscope/_build/keyboardio_virtual_model01/obj/AbsoluteMouseReport.o] Error 1
make[3]: *** [/home/mherrn/projekte/Kaleidoscope/testing/makefiles/testcase.mk:107: /home/mherrn/projekte/Kaleidoscope/_build/keyboardio_virtual_model01/lib/libcommon.a] Error 2
and so on.
When trying it to restrict it to testing the LongPress plugin it fails differently:
$ make simulator-tests TEST_PATH=tests/plugins/LongPress
Building in quiet mode. For a lot more information, add 'VERBOSE=1' to the beginning of your call to make
make -C tests all
make[1]: Entering directory '/home/mherrn/projekte/Kaleidoscope/tests'
find: ‘tests/plugins/LongPress’: No such file or directory
make[1]: Leaving directory '/home/mherrn/projekte/Kaleidoscope/tests'
make[1]: Entering directory '/home/mherrn/projekte/Kaleidoscope/tests'
make -C /home/mherrn/projekte/Kaleidoscope/testing/googletest/build
[ 25%] Built target gtest
[ 50%] Built target gmock
[ 75%] Built target gmock_main
[100%] Built target gtest_main
find: ‘tests/plugins/LongPress’: No such file or directory
make -C /home/mherrn/projekte/Kaleidoscope/testing/googletest/build
[ 25%] Built target gtest
[ 50%] Built target gmock
[ 75%] Built target gmock_main
[100%] Built target gtest_main
find: ‘tests/plugins/LongPress’: No such file or directory
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.
@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.
It’s a normal Debian Bookworm.
I have tried it now on a separate computer with a Debian Bullseye and it is working there. Then I had the idea to try it with a fresh clone of this repository and indeed, I can now run the tests (I can restrict it to the LongPress tests by calling it as make simulator-tests TEST_PATH=plugins/LongPress; the tests directory must not be specified).
However, the result is the same. The tests fail with a timestamp deviation. I was able to resolve two of them (I think I understood what I did wrong). But one test still fails and I don’t know why.
@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.
# press key “A” → this should produce the key A in the keyboard-report
RUN 4 ms
PRESS A
RUN 1 cycle
EXPECT keyboard-report Key_A
# release key "A" again → this should remove the A from the keyboard-report
RUN 4 ms
RELEASE A
RUN 1 cycle
EXPECT keyboard-report empty
RUN 5 ms
However, the test fails with this message:
Expected equality of these values:
observed_report.Timestamp()
Which is: 10
expected_report.Timestamp()
Which is: 5
Report timestamps don't match (i=0)
I played around a bit with the order of the test statements, but I just don’t understand what is the problem here.
I was just starting to dig into the fails, but I see that you're actively pushing updates.
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:
VERSION 1
KEYSWITCH A 1 0
NAME LongPress AutoShift tap
RUN 4 ms
PRESS A
RUN 1 cycle
EXPECT keyboard-report Key_A
RUN 4 ms
RELEASE A
RUN 1 cycle
EXPECT keyboard-report empty
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, tests/plugins/LongPress/autoshift/test/generated-testcase.cpp
The generated content of that is
// ==============================================================================
TEST_F(GeneratedKTest, 1_LongPressAutoShiftTap) {
ClearState(); // Clear any state from previous tests
// This tests that short tapping any of the keys should always produce the
// normal key
sim_.RunForMillis(4);
PressKey(key_addr_A); //
sim_.RunCycles(1);
ExpectKeyboardReport(Keycodes{Key_A}, "No explanatory comment specified");
sim_.RunForMillis(4);
ReleaseKey(key_addr_A); //
sim_.RunCycles(1);
ExpectKeyboardReport(Keycodes{}, "No explanatory comment specified");
LoadState();
CheckReports();
} // TEST_F
The CheckReports call is what's throwing the error.
That's defined in testing/VirtualDeviceTest.cpp
void VirtualDeviceTest::CheckReports() const {
CheckKeyboardReports();
CheckMouseReports();
}
In this case, a keyboard report is at issue:
void VirtualDeviceTest::CheckKeyboardReports() const {
int observed_keyboard_report_count = HIDReports()->Keyboard().size();
int expected_keyboard_report_count = expected_keyboard_reports_.size();
EXPECT_EQ(observed_keyboard_report_count, expected_keyboard_report_count);
int max_count = std::max(observed_keyboard_report_count,
expected_keyboard_report_count);
for (int i = 0; i < observed_keyboard_report_count; ++i) {
auto observed_report = HIDReports()->Keyboard(i);
auto observed_keycodes = observed_report.ActiveKeycodes();
if (i < expected_keyboard_report_count) {
auto expected_report = expected_keyboard_reports_[i];
auto expected_keycodes = expected_report.Keycodes();
EXPECT_THAT(observed_keycodes,
::testing::ElementsAreArray(expected_keycodes))
<< expected_keyboard_reports_[i].Message() << " (i=" << i << ")";
EXPECT_EQ(observed_report.Timestamp(), expected_report.Timestamp())
<< "Report timestamps don't match (i=" << i << ")";
} else {
...
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!
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?
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.
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 make works.
But when calling make simulator-tests I get this compilation error:
/home/runner/work/Kaleidoscope/Kaleidoscope/plugins/Kaleidoscope-LongPress/src/kaleidoscope/plugin/LongPress.h:111:3: error: 'longpress' does not name a type; did you mean 'LongPress'?
111 | longpress::AutoShiftCategories enabledAutoShiftCategories() {
| ^~~~~~~~~
| LongPress
/home/runner/work/Kaleidoscope/Kaleidoscope/plugins/Kaleidoscope-LongPress/src/kaleidoscope/plugin/LongPress.h:126:24: error: 'longpress' has not been declared
126 | void enableAutoshift(longpress::AutoShiftCategories category) {
| ^~~~~~~~~
[…]
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!
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.
Many thanks! I have changed that. It is finished now and ready for review.
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.
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 https://github.com/keyboardio/Kaleidoscope/pull/1423#issuecomment-2138821098, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAALC2GPR4M433FXFJZA2QDZE3FOBAVCNFSM6AAAAABH4VWBOKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZYHAZDCMBZHA . You are receiving this because you were mentioned.Message ID: @.***>
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.
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 KeyAddr but also for mappings on Key.
These are the reasons I did this:
- It doesn’t complicate the implementation at all.
- It doesn’t complicate the configuration of LongPress. In fact it is even easier to grasp if the restriction to a specific layer can be applied to both,
KeyAddrandKey.
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):
- omitting the layer
- specifying the special constant
ALL_LAYERSas the layer number
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:
The special constant ALL_LAYERS is now defined in LongPress.h. It is a public constant and (especially regarding its name) may theoretically be used by other plugins. Would it make more sense to define this constant in layers.h instead? Is the value -1 still correct then?
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.