Replace NumPad plugin with ColormapOverlay

[EvyBongers]

Following up on #1416, this PR builds on top of #1418 and replaces the NumPad plugin with ColormapOverlay.

This PR includes all changes of #1418, but should be rebased once #1418 is merged to keep changes to their relevant PRs.

[EvyBongers]

Should I include some kind of deprecation warning for the NumPad plugin as well, or do you prefer to have a separate PR for that?

[obra]

I'm wondering if we can make this a little nicer to manage. with a couple of macros.

[EvyBongers]

I could probably make it similar to Colormap, but when I created the plugin my reasoning was that there will there will generally only be very few overlays on a layer. I'm sure that I could create a some macros to make this nicer, but I wonder if it's worth the effort if this is an edge case like I feel it is.

[EvyBongers]

Been thinking about this a bit. The one possibility I see is to add a macro for defining an area (col_start, row_start, col_end, row_end) (or something similar). That feels easy enough to implement and may well serve several more users. I do wonder how I would store that in EEPROM once I start working on that PR.

[EvyBongers]

I just had another idea. The way it's currently set up is heavily inspired by Qukeys, but because of the different nature of the plugins, I could change it to use structs that store layer, color and a list of keys. That makes it more easily extendable for this use case without needing to change much in the api

[EvyBongers]

Code style CI complains about the indent on this comment... should I add // clang-format on/off to stop it from complaining?

[obra]

Either that or let it adjust the indent to where it wants.

[EvyBongers]

Adjusting the indent breaks with how the comments on the closing brackets are placed elsewhere in the file. I could also remove the comments on all the keys, but that seems worse...

[obra]

I think that once it's in eeprom, it probably makes sense to just decompose it to the actual bitmap of keys rather than ranges? Although I'd love to have a nice way to define keys or areas of keys that could have effects targeted at them. So you could make a column a VU meter or a named area that a color notification could be sent to. but that's out of scope here, i think.

…

On Tue, May 7, 2024 at 2:39 PM Evy Bongers ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In examples/Devices/Keyboardio/Model100/Model100.ino <#1420 (comment)> : > @@ -622,14 +649,30 @@ void setup() { // First, call Kaleidoscope's internal setup function Kaleidoscope.setup(); + COLORMAP_OVERLAYS( Been thinking about this a bit. The one possibility I see is to add a macro for defining an area (col_start, row_start, col_end, row_end) (or something similar). That feels easy enough to implement and may well serve several more users. I do wonder how I would store that in EEPROM once I start working on that PR. — Reply to this email directly, view it on GitHub <#1420 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAALC2FRN2PDWHDW6D6JGMLZBFCYBAVCNFSM6AAAAABHHEEHZ6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANBUGI2DKOBZGA> . You are receiving this because you commented.Message ID: ***@***.***>

[obra]

Ok. Having looked over the changes:

• My gut would be to not disable clang-format for the two new blocks of code. Just let it do its thing. It's less for us to have to think about if we can just have the formatter format.

• Especially in the sketch, we need comments about what the new code is doing. Many users use their sketches as a learning tool. So explaining what each chunk of code does (and ideally why we do it) is important.

Once those two things are sorted, I'd be good to merge this.

[EvyBongers]

I may have gone a bit overboard with the comments, but since key addresses aren't necessarily straight forward, I figured should include a map of all the addresses with a reference to the source.

[obra]

I may have gone a bit overboard with the comments, but since key addresses aren't necessarily straight forward, I figured should include a map of all the addresses with a reference to the source.

Heh. Although you going a bit overboard does lead me to ponder whether it would make more sense to have a way to specify the overlay for a given layer using the keymap macros. It'd be self-documenting and easier to create and modify. Thoughts?

[EvyBongers]

Heh. Although you going a bit overboard does lead me to ponder whether it would make more sense to have a way to specify the overlay for a given layer using the keymap macros. It'd be self-documenting and easier to create and modify. Thoughts?

It can certainly be done, but I feel that that defeats the purpose of the plugin. This is something that Colormap does already and does really well. I feel that Colormap-Overlay would typically apply colors to just a few keys, at they are applied on top of the active LED effect.

[obra]

It can certainly be done, but I feel that that defeats the purpose of the plugin. This is something that Colormap does already and does really well. I feel that Colormap-Overlay would typically apply colors to just a few keys, at they are applied on top of the active LED effect.

What I'm thinking about is mostly about making the API easier for users to understand. Having the two plugins configured with equivalent datastructures means there's less syntax for folks to learn, we don't need to provide the big comment showing off the key map layout, etc. If the colormaps and overlay colormaps worked the same, it'd also make it more straightforward to add support for colormap overlays to chrysalis.

This isn't a blocker for this PR landing (as you might guess by the fact that I merged the PR). I'm going to open a new related ticket.