Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The Numpad plugin should only highlight actual numpad keys #1416

Open
EvyBongers opened this issue Mar 23, 2024 · 9 comments
Open

The Numpad plugin should only highlight actual numpad keys #1416

EvyBongers opened this issue Mar 23, 2024 · 9 comments
Labels
design-change Proposals that incur potentially breaking design changes

Comments

@EvyBongers
Copy link
Contributor

What do you want to change?

The way the Numpad plugin currently works, it checks if the numpad layer is active and that keys on the layer don't have any modifiers (key flags) applied to them. For the default layouts in the provided firmware builds this works, but for Chrysalis users who repurpose layer it's far from convenient. In order to improve user experience in Chrysalis, I see a few options:

  • Store the numpad layer setting in EEPROM (allow setting a different layer as numpad layer through Chrysalis)
    • Optionally allow specifying -1 as value to disable the numpad plugin
  • Add an option (EEPROM and/or Focus) to disable the numpad plugin (instead of or in addition to checking if the numpad layer is active)
  • Update the numpad plugin to only set the led color for keys on the keypad according to the HID mappings (Key_Keypad*)
    • Caveat is that the function of the keys is influenced by the state of numlock, which might be unexpected for users. Possibly the plugin can be extended to automatically enable/disable numlock when the numpad layer is (de)activated.

How will it make Kaleidoscope better?

It's not so much Kaleidoscope that would get better, but this would set the stage for improving user experience for Chrysalis users.

What trouble might users have in adapting to the new functionality?

In case of options 1 and 2, I don't see any trouble. Chrysalis users who use the numpad layer and plugin as is wouldn't notice any difference, as long as the default values are kept. Kaleidoscope users might have to update their sketch to match how the plugin parameters are set.
In case of option 3, users would have to update their keymap to use only keypad keys, where the factory sketches for both M01 and M100 use the number row keys and 'normal' enter rather than their keypad counterparts. As stated above, using the keymap variants of the numbers and dot would be affected by the numlock state, though it might be possible to enable/disable numlock through the plugin.

What trouble might developers have in adapting to the new functionality?

I don't think there would be any differences outside of the plugin itself, except for now the existing plugin parameters are set.

@EvyBongers EvyBongers added the design-change Proposals that incur potentially breaking design changes label Mar 23, 2024
@obra
Copy link
Member

obra commented Mar 25, 2024

I'd actually really love to entirely get rid of the Numpad plugin in favor of a pre-configured color theme for that layer. It's never been good and dates back to before we had the option of custom per-layer LED color themes..

@EvyBongers
Copy link
Contributor Author

Oh, that should be easy enough to do. Would that be for all example sketches or just for Keyboardio hardware?

@obra
Copy link
Member

obra commented Mar 27, 2024 via email

@EvyBongers
Copy link
Contributor Author

EvyBongers commented Mar 27, 2024

How do we make it not hurt for users flashing a new firmware onto their existing model 100?

I think this should cover it:

  • Remove the plugin from the default/example sketch
  • Define a colormap entry in the default/example sketch matching the default layout of the numpad layer
    • Users who don't use layer 1 shouldn't notice for obvious reasons
    • Newly sold M100s should get updated as soon as users start using Chrysalis
    • Users who have a custom colormap for that layer would notice the change if:
      • they assigned different colors on those keys (they would start to see the assigned colors)
      • they assigned colors only to keys that aren't in the numpad (they would see those keys have the leds turned off)
  • Add a deprecation messege to the plugin (both in the README and a compile time message)
  • Mention the change in the Kaleidoscope release notes

For the users in the third category of the second bullet, you should have an entry in NEWS.md (which you probably want anyway).

And we should mark the old plugin as "don't use this for new sketches. do this simpler thing instead"

The plugin detzt is working on combined with ActiveLayerKeys might make a sound alternative if you're gonna deprecate it

I think we leave the model 01 alone.

That's for you to decide. Replacing the numpad plugin with a colormap entry might free up some bytes, but in the end that decision is yours. I wonder how model 01 users would feel about this change.

@obra
Copy link
Member

obra commented Mar 29, 2024 via email

@EvyBongers
Copy link
Contributor Author

Custom colormaps would behave differently anyway, because that require users to use that specific LED mode in order to see the keys light up. Users that use any other LED mode wouldn't see it in the first place.

If you do want to set a colormap for that layer, that seems like something best solved in Chrysalis imho. As this seems to apply only to users who work exclusively with Chrysalis, they would (most likely) flash new firmware with Chrysalis. Therefore restoring EEPROM at the end of that process could make sure to set those colors.
Of course that does have some drawbacks and caveats:

  • It's a corner case that would need code paths to be removed again at a later point
  • It requires setting a palette color somewhere, which might surprise users
  • Setting a palette color for it requires finding some palette entry that isn't used (which might not exist)
    • To solve this, the palatte could be extended to 24 colours

Setting the colormap in Kaleidoscope is impossible except for newly delivered keyboards, because there is no way to check what palette indexes are used or if the colormap for that layer has been touched at all. That is, unless you want to go down the path of overwriting EEPROM after it has been restored...

I see only two ways to create an experience that matches the way it currently works:

  • Update the numpad plugin to add a toggle to enable/disable it and allow it to be changed in Chrysalis. Then add release notes stating that it will be removed in a future release and write documentation on how to create a colormap to replace it for when it does get removed
  • Add LED-EffectPerLayer and ActiveLayerKeys to the build to replace it
    • Optionally add controls to Chrysalis to configure those plugins
    • ℹ️ ActiveLayerKeys does not (yet) use the same palette as colormap, though it probably should do that eventually
  • Add the ColormapOverlay plugin to the build to create the same experience
    • Optionally add controls to Chrysalis to configure the plugin
    • ColormapOverlay uses the same palette as Colormap, so that would still require work to make sure that the relevant color is available on the palette

@obra
Copy link
Member

obra commented Apr 16, 2024 via email

@EvyBongers
Copy link
Contributor Author

EvyBongers commented Apr 17, 2024

I feel like ColormapOverlay seems like a cleaner solution. Do you have an opinion on which of the two solutions you think would work better?

This option is what best matches the current setup, with the exception that it uses the palette and that therefore the colors would be configurable through Chrysalis already. This would be my choice as well.

To implement this, I think it would be best split up like this:

  1. Update the palette to use 24 colors
  2. update the example sketches to replace NumPad with ColormapOverlay
  3. Update ColormapOverlay to add support for EEPROM

@obra
Copy link
Member

obra commented Apr 17, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design-change Proposals that incur potentially breaking design changes
Projects
None yet
Development

No branches or pull requests

2 participants