qmk / qmk_firmware

Open-source keyboard firmware for Atmel AVR and Arm USB families
https://qmk.fm
GNU General Public License v2.0
18.02k stars 38.73k forks source link

MOD_MASK_* constants for single modifiers, and modifier macros in general #5700

Closed vomindoraan closed 4 years ago

vomindoraan commented 5 years ago

In #4337 we added the MOD_MASK_CTRL, MOD_MASK_SHIFT, MOD_MASK_CSA etc. constants to core code. These are shorthand for (MOD_BIT(KC_LCTRL) | MOD_BIT(KC_RCTRL)) etc. since expressions of this form are very common.

Would constants for single modifiers, e.g. #define MOD_MASK_LCTRL MOD_BIT(KC_LCTRL), be equally as useful? I think they could be. I see a lot of checks in the codebase that use MOD_BIT() with single modifiers. Adding these definitions would “complete the set”, so to speak. It could make code that uses mod masks more uniform and easier to understand, since it would essentially remove the need for MOD_BIT(). It could also help reduce the confusion between:

by removing expressions with MOD_BIT() from the equation. Finally, MOD_MASK_LCTRL is also shorter and easier to type than MOD_BIT(KC_LCTRL).

The new constants could be implemented like this: https://github.com/qmk/qmk_firmware/compare/master...vomindoraan:mod_mask_single.

Another thing to consider are the names. Should the new constants use:

/discuss

vomindoraan commented 5 years ago

@drashna @noroadsleft — I'd appreciate your guys' input on this matter, since you seem to be using the MOD_MASK_* constants the most. @fauxpark too, since you were involved with the original PR. Also @skullydazed, since you seem to have the best idea of where QMK is heading in terms of code style.

drashna commented 5 years ago

I think it would be useful, yeah. I think it would be an edge case, though. But that's fine.

Also, I think that MEH and HYPER should be added, as well. :)

As for the naming, I think aliasing the various options would be best. That way either can be used, interchangeably. Which is the current behavior for keycodes, and the like.

noroadsleft commented 5 years ago

I can't speak to masks for MEH or HYPER, but I agree with everything else drashna said.

stale[bot] commented 4 years ago

This issue has been automatically marked as resolved because it has not had activity in the last 90 days. It will be closed in the next 30 days unless it is tagged properly or other activity occurs.

stale[bot] commented 4 years ago

This issue has been automatically closed because it has not had activity in the last 30 days. If this issue is still valid, re-open the issue and let us know.