Relocate RGB keycode processing

[zvecr]

Description

This PR relocates a block of keycode handling to its own file for readability and also attempts to save some firmware bytes. Mimicking travis by running ./util/docker_build.sh helix/pico/back:default:

before
* The firmware size is approaching the maximum - 28482/28672 (99%, 190 bytes free)

after
* The firmware size is approaching the maximum - 28158/28672 (98%, 514 bytes free)

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[drashna]

I like this, a lot.

Though, I don't like the remapping of the rgb matrix functions to rgb light functions, even if it's behind a preprocessor.

In fact, this makes #5404 almost trival to implement, rather than the messy mess that it is right now.

[yanfali]

Looks good

[yanfali]

let's change these to return boolean and return false. This will make the caller slightly cleaner.

[zvecr]

So i gave the above suggestion a go, and it consumes about an extra 50 bytes with the return false and return handleKeycodeRGB(. While its about 0.17% of the available flash, we recent had the issue with trying to claw back 4 bytes. Do we want to focus on readability here? (its still a net saving)

[drashna]

While I'm all for regaining as much program space as possible... I think that readability (and maintainability) should be the priority.

[zvecr]

If the handleKeycodeRGB had more logic, i would agree, but if it just returns false i would say personally not any more readable. That being said, i don't mind either way.

In the case of the animations, you end up with

#ifdef RGBLIGHT_EFFECT_KNIGHT
                return handleKeycodeRGBMode(RGBLIGHT_MODE_KNIGHT, RGBLIGHT_MODE_KNIGHT_end);
#else
                return false;
#endif

or

#ifdef RGBLIGHT_EFFECT_KNIGHT
                return handleKeycodeRGBMode(RGBLIGHT_MODE_KNIGHT, RGBLIGHT_MODE_KNIGHT_end);
#endif
                return false;

with the dead double return that gets optimised out, which in both cases is less readable to me.

[yanfali]

I deliberately didn't ask you to modify RGBMode because I didn't think it helped readability

[yanfali]

So i gave the above suggestion a go, and it consumes about an extra 50 bytes with the return false and return handleKeycodeRGB(. While its about 0.17% of the available flash, we recent had the issue with trying to claw back 4 bytes. Do we want to focus on readability here? (its still a net saving)

I actually think that making this signature return something maybe useful in the long run, much like we had with led settings. Right now it only returns false, but there may be cases in the future where it's useful to return true.

[zvecr]

Sorry, I read "these" as both the new function signatures.

As this is non user facing, would we not just defer to the point we implement the additional functionality?

[yanfali]

I think that's a fair point. I still think it's slightly more readable than having an explicit false, but it's also anticipating use cases we don't have, so just leave it as is for now.

[zvecr]

Finally ironed out all the rebase issues, should be good to go again!