Autoshift explicit config

[hupfdule]

This PR extends the existing AutoShift plugin to not only produce shifted
characters, but totally different characters via long press.

It does so by allowing to explicitly configure which character (or rather which
key press) should be produced when holding a key.

Such an explicit mapping takes precedence over shifting the key. That
means if all alpanumeric characters are configured for AutoShift, but
the ‘e’ key has an explicit mapping to produce ‘ë’, a long press on ‘e’
will result in ’ë’, not ‘E’.

A test for the new logic is provided and the example ino-file as well as the README has been extended to show the usage of it.

[hupfdule]

I added a fourth commit that caches the configured keys to not have to fetch them from PROGMEM twice. I’ve got the idea from the Qukeys plugin and basically copied it from there.

Please check whether such a caching mechanism makes sense here and is correctly implemented.

[obra]

This is pretty neat! And I know a bunch of folks will find it useful. One thing I'm wondering is whether this belongs in AutoShift or ought to be a generalized LongPress plugin on its own, since it's not really auto-shifting here.

Thoughts?

[hupfdule]

One thing I'm wondering is whether this belongs in AutoShift or ought to be a generalized LongPress plugin on its own, since it's not really auto-shifting here.

I thought about this as well. This is what springs to mind:

• Even if it is its own plugin the vast majority of the code would be just copy and paste.
• The auto-shifting and explicit configuration do not interfere with each other. Of course one cannot auto-shift and explicitly map the same key, but that would not work even if they were separate plugins.
• If we look at it the other way around, there would be no AutoShift plugin and we only had a generalized LongPress plugin, it is very likely that someone would introduce shortcut methods for supporting the most common use case: auto-shifting on long press.

[hupfdule]

Hmm, I just force pushed the commits to fix formatting problems and now the already executed checks are gone. Should I refrain from force-pushing to this PR?

[hupfdule]

The code style check currently complains about this line in the example AutoShift.ino:

  AUTOSHIFT(
    kaleidoscope::plugin::LongPress(Key_Slash, Key_Backslash), 
  )

and wants it in that form:

  AUTOSHIFT(
    kaleidoscope::plugin::LongPress(Key_Slash, Key_Backslash), )

But I find that much more unreadable. Do you want me to change the formatting according to the code style check or leave it as it is now?

[obra]

Do I love the autoformatting of that? No. Do I believe pretty strongly that we need to have formatting decisions enforced by code? Yeah.

…

On Sun, Apr 23, 2023 at 2:30 PM hupfdule ***@***.***> wrote: The code style check currently complains about this line in the example AutoShift.ino: AUTOSHIFT( kaleidoscope::plugin::LongPress(Key_Slash, Key_Backslash), ) and wants it in that form: AUTOSHIFT( kaleidoscope::plugin::LongPress(Key_Slash, Key_Backslash), ) But I find that much more unreadable. Do you want me to change the formatting according to the code style check or leave it as it is now? — Reply to this email directly, view it on GitHub <#1340 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAALC2DDKUDCYA6MNGRB5NLXCWNPRANCNFSM6AAAAAAW5PONYU> . You are receiving this because you commented.Message ID: ***@***.***>

[hupfdule]

OK, I have changed it accordingly.

[algernon]

I've been going through this PR today, and I have a few observations. First of all, I'm loving the feature, it's great! The code looks good aswell, and I'm inclined to merge it as-is. However, before I do that, there are two things I'd like to discuss, and figure out.

First and foremost, the configuration itself: it's done in a way that looks quite rough to extend later to support setting up mappings via Chrysalis, and storing them in EEPROM. For new functionality, I think it's important to keep Chrysalis in mind, and even if not supporting it immediately, design the implementation in a way that it's not a major rework to add it later. We've had that problem with TapDance and Macros, and the way Chrysalis-enabling worked there (basically a separate plugin) doesn't make me very happy. I'm not sure if we can reasonably avoid that same problem here, but it's worth a shot to give it a little thought.

Second, as @obra mentioned, the name: with this change, AutoShift essentially becomes a generic LongPress plugin, with a special-case for the long-press shift. I'd suggest shuffling things around a bit (but this can be done post-merge): what if there was a generic LongPress class, which had an overrideable method that figures out what to do on a long-press? AutoShift would then become a plugin that is essentially LongPress, with the old auto-shift code set up, while the LongPress plugin would be a different instance of the same class, with the new code set up for handling the long-press case?

This way, the vast majority of the code would be shared between the two, and while there would be some extra code (both source and compiled), it wouldn't be much, as far as I see. For a small cost, we'd have a cleaner separation between the two things, users of either would be able to choose which plugin they want, and the new feature would live under the more appropriate LongPress name.

What do you think?

I'm more than happy to propose code changes towards the aforementioned goal.

[hupfdule]

I've been going through this PR today, and I have a few observations.

Great! Many thanks for reviewing it.

First and foremost, the configuration itself: it's done in a way that looks quite rough to extend later to support setting up mappings via Chrysalis, and storing them in EEPROM. For new functionality, I think it's important to keep Chrysalis in mind, and even if not supporting it immediately, design the implementation in a way that it's not a major rework to add it later.

That is absolutely reasonable and I definitely agree with it. Unfortunately I don’t know what is necessary to prepare support for Chrysalis. It would be great if there was an example or scaffold for plugin authors to follow when preparing configuration possiblities for their plugins. What I did was looking at how other plugins do it and mindlessly copying it. I had the problem that there are many differences between the plugins and I didn’t find a “best practice” on how to do it. [¹]
I mainly copied it from the newest plugin: “Chords”. It not only is the newest one, but also provides the IMO best readable and writable way of defining such configuration options. Of course I am thinking in configuring it in the .ino file as opposed to Chrysalis here. And I think that is equally important. I don’t think we gain much if a feature is configurable via Chrysalis, but unwieldy when configuring it via the .ino file.
For example I don’t like the way the CharShift plugin is configured:

CS_KEYS(
    kaleidoscope::plugin::CharShift::KeyPair(Key_Comma, Key_Semicolon),               // CS(0)
    kaleidoscope::plugin::CharShift::KeyPair(Key_Period, LSHIFT(Key_Semicolon)),      // CS(1)
    kaleidoscope::plugin::CharShift::KeyPair(LSHIFT(Key_Comma), LSHIFT(Key_Period)),  // CS(2)
  );

I like that is results in virtual keycodes that can be used in a keymap, but having only numbers for these is very suboptimal.

Second, as @obra mentioned, the name: with this change, AutoShift essentially becomes a generic LongPress plugin, with a special-case for the long-press shift.

I expressed my basic thoughts about that above, but want to answer on some of your ideas.

First: If this was a new plugin, I would definitely prefer calling it a “LongPress” plugin and having AutoShift as the most common case supported in an easy way. But as the “AutoShift” plugin already exists, this is much harder as renaming an already existing plugin may not be a good idea. And I couldn’t decide which is the best way to go if we leave them as one plugin (like it is done in this PR).
So the remaining thoughts are about whether it would be good to split them into two plugins and if so, how.

what if there was a generic LongPress class, which had an overrideable method that figures out what to do on a long-press? AutoShift would then become a plugin that is essentially LongPress, with the old auto-shift code set up, while the LongPress plugin would be a different instance of the same class, with the new code set up for handling the long-press case?

Basically that sounds like a good idea. But I am not sure it would be worthwhile. Do we really want both, an “AutoShift” and a “LongPress” plugin? AutoShift is mainly just a special case of a LongPress. And they would still conflict if the same key is affected in both plugins. While it is easy to declare that the explicit configuration overrides the AutoShift behaviour if both functionalities are provided in the same plugin, this would be not so easy if they were split into separate plugins.
Again, when imagining we wouldn’t hava such a plugin already and this would be the initial implementation I would see the explicit configuration as the basic functionality. But as AutoShift is certainly the most common use case it would be worthwhile to support this use case by not having to configure each key separately. And therefore I would think providing some methods to tell it to “auto shift alphas”, “auto shift numbers”, etc. would be the way to go. I would not think that creating a separate plugin for that special use case would a good idea.
And I also don’t think that users would want to decide between the “AutoShift” and the “LongPress” functionality. It is very reasonable to wanting both (provided that they are assigned to different keys).

I'm more than happy to propose code changes towards the aforementioned goal.

Your help is very much appreciated! I already expressed my thoughts. It would be good if someone else (being it @obra or someone else) would share his thoughts about it.
If changes are necessary, I will probably need help as I have only basic C++ knowledge and I wrote this PR mainly by copying code and hoping it would do what I want it to do.

[¹] I think that is one of the main problems for plugin authors. There are not enough guides, best practices and HOWTOs on how to do things that are common to many plugins. And the existing plugins differ too much to be a good starting point for such a decision.

[obra]

Hi! So sorry for the long radio silence and thank you for prodding me about it.

I've been pondering what the right thing to do here is.

I think we do want this functionality. I also think we want it with a plugin name like LongPress rather than AutoShift, so I would rather see this functionality as a new plugin named LongPress forked from the old plugin named AutoShift that essentially obsoletes the old plugin.

I agree that there's a potential issue with a user configuring two potentially conflicting plugins causing broken or unexpected behavior, but that is something that can be handled (fairly reasonably) in the documentation for the plugins. We have no rule stating that all plugins need to be compatible with all other plugins.

In terms of making this plugin work with Chrysalis, what we'd need is some way to represent the data for what a key in the keymap does on long press in EEPROM, coupled with a way to mark a key on a given layer as having long-press behavior that needs to be resolved by the longpress plugin.

Right now, eeprom keymaps are 16 bits per key per layer (

Kaleidoscope/plugins/Kaleidoscope-EEPROM-Keymap/src/kaleidoscope/plugin/EEPROM-Keymap.cpp

Line 89 in fd7f5f9

void EEPROMKeymap::updateKey(uint16_t base_pos, Key key) {

) - 8 bits for the primary keycode and 8 bits for flags describing modifications to that keycode:

Kaleidoscope/src/kaleidoscope/key_defs.h

Line 39 in fd7f5f9

// Constant flags values

Plugins can define ranges of keycodes they use for their own specialized behavior, but those ranges are somewhat limited. https://github.com/keyboardio/Kaleidoscope/blob/master/plugins/Kaleidoscope-Ranges/src/Kaleidoscope-Ranges.h#L83

So we could set up, say, 32 "longpress" keys that get handed off to the longpress plugin for evaluation, sort of like how we do Dynamic Macros: https://github.com/keyboardio/Kaleidoscope/blob/master/plugins/Kaleidoscope-DynamicMacros/src/kaleidoscope/plugin/DynamicMacros.cpp

But that's not a great user experience and feels very...limiting.

It feels sort of like it might be time to think about redesigning how we store keymaps in EEPROM to make them more extensible. But that's not a small undertaking.

I'd dearly love to be able to support full configuration of LongPress, QuKeys, Chords, and all the rest in Chrysalis.

All of that is not a blocker for merging a LongPress plugin. But it is certainly something that I'd love to see and think users would really appreciate.

[obra]

I should clarify that it's my intent that after the new longpress plugin exists, AutoShift would no longer be recommended for new users. I don't see a reason to remove it, but I'd be happy to see a note in its readme pointing folks at LongPress as a more capable replacement that is expected to see future improvements.

[hupfdule]

Hello and thanks for your answer!

I think we do want this functionality. I also think we want it with a plugin name like LongPress rather than AutoShift, so I would rather see this functionality as a new plugin named LongPress forked from the old plugin named AutoShift that essentially obsoletes the old plugin.

In fact these were my thoughts too in the last days. Having both plugins exist simultanously is probably the easiest approach without breaking any existing configs and still allowing further development as long as the documentation clarifies that AutoShift is deprecated/obsolete.

I will now rewrite the PR to introduce a new LongPress plugin instead of modifying AutoShift.

In terms of making this plugin work with Chrysalis, what we'd need is some way to represent the data for what a key in the keymap does on long press in EEPROM, coupled with a way to mark a key on a given layer as having long-press behavior that needs to be resolved by the longpress plugin.

[…]

It feels sort of like it might be time to think about redesigning how we store keymaps in EEPROM to make them more extensible. But that's not a small undertaking.

I'd dearly love to be able to support full configuration of LongPress, QuKeys, Chords, and all the rest in Chrysalis.

All of that is not a blocker for merging a LongPress plugin. But it is certainly something that I'd love to see and think users would really appreciate.

OK, does that mean that we should focus on getting the base LongPress functionality implemented first and wait for the outcome of #1419 before trying to get LongPress be configurable via Chrysalis?

[hupfdule]

One question regarding the configuration of the plugin (Not only, but also in respect of having it configurable via Chrysalis).

At the moment the user needs to specify the (logical) Key to apply to and the (logical) Key that will be generated on long press. I am not fully happy with that approach and would like to change it to specifying a (physical) KeyAddr to apply to and the (logical) Key that will be generated on long press.

Is this relevant for making it configurable via Chrysalis?

The current existing plugins have very different approaches of configuring mappings (see this post in the Keyboardio forum I wrote before actually trying to write this PR).
I think I copied it from the Qukeys plugin. What is the preferred approach here? Is there some ”standard”?

[obra]

I think my ideal is to define a long-press key as "this logical key when tapped, this logical key when held" and to then insert that logical entry into a keymap.

https://github.com/keyboardio/Kaleidoscope/blob/master/examples/Keystrokes/TapDance/TapDance.ino is kind of like that. I don't love needing to track the index of the tapdance key in the keymap rather than being able to write something like Plugin_Tapdance( tap: Key_Q, double_tap: Key_X, triple_tap: Key_Blah, Hold: Key_Foo) in the keymap, but that's more a syntax thing than anything else.

[hupfdule]

I have created a new PR for the LongPress plugin. It was easier for me to separate it into a different branch and we can easier compare it to this one if necessary.

So please see #1423 to review the new LongPress plugin. It not yet in a finished state. I already have a few questions I mention in the PR.