Adds a default value for IS_COMMAND for COMMAND feature #4301

drashna · 2018-10-31T01:09:07Z

Wasn't sure if I should pull out all of the existing defines for this, but opted against it.

tmk_core/common/command.c

vomindoraan · 2018-10-31T15:16:06Z

Yeah, I'd remove existing defines that aren't different from the default just because it'd make all the config.h files cleaner.

But in either case, you should definitely remove #undef IS_COMMAND in these three files since it isn't needed anymore.

drashna · 2018-10-31T16:09:37Z

Well, one of those files doesn't actually need it removed, if you check. :)
And that's mine.

Though as for removing it everywhere else, I'll let @jackhumbert or @skullydazed make the call on that one, and then proceed accordingly. I'm not a fan of touching every keyboard, unless it's a significant change (like the PSM change).

vomindoraan · 2018-10-31T16:21:29Z

Totally understandable. Even though I can't see anything going wrong if it's done properly, it's still editing every or almost every keyboard's config.h file. This refactor would be most similar to the KEYMAP→LAYOUT change in that regard.

Regarding the #undef, your code would be cleaner without it too 😉

vomindoraan · 2018-11-04T05:26:33Z

Suggestions here: drashna#15

vomindoraan · 2018-11-08T19:07:04Z

keyboards/orthodox/keymaps/drashna/config.h

@@ -34,6 +34,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.


 /* key combination for magic key command */
+#undef IS_COMMAND


The keyboard doesn't have #ifndef around its #define IS_COMMAND?

Nope, and no other board has.

And I think this should be fine, to be honest.

vomindoraan

Yeah, in that case the templates should probably be updated to prevent this for future boards.

vomindoraan · 2018-11-08T19:10:26Z

quantum/template/ps2avrgb/config.h

-#define IS_COMMAND() (keyboard_report->mods == (MOD_BIT(KC_LSHIFT) | MOD_BIT(KC_RSHIFT)))
+/* key combination for magic key command */
+/* defined by default; to change, uncomment and set to the combination you want */
+// #define IS_COMMAND() (keyboard_report->mods == (MOD_BIT(KC_LSHIFT) | MOD_BIT(KC_RSHIFT)))


Suggested change

// #define IS_COMMAND() (keyboard_report->mods == (MOD_BIT(KC_LSHIFT) | MOD_BIT(KC_RSHIFT)))

// #ifndef IS_COMMAND

// #define IS_COMMAND() (keyboard_report->mods == (MOD_BIT(KC_LSHIFT) | MOD_BIT(KC_RSHIFT)))

// #endif

quantum/template/avr/config.h

vomindoraan · 2018-11-09T01:07:13Z

keyboards/whitefox/keymaps/konstantin/config.h

@@ -18,3 +18,5 @@
 #define PERMISSIVE_HOLD
 #define TAPPING_TERM    200
 #define TAPPING_TOGGLE  2
+
+#define IS_COMMAND() (keyboard_report->mods == (MOD_BIT(KC_LSFT) | MOD_BIT(KC_RCTL)))


Suggested change

#define IS_COMMAND() (keyboard_report->mods == (MOD_BIT(KC_LSFT) | MOD_BIT(KC_RCTL)))

It's already defined above on lines 5 and 6, so remove these two lines. No idea how this passed CI.

@drashna 👆

Also remove the #undef IS_COMMAND on line 5, it works without that.
(You also have an undef in your Orthodox keymap, so you can remove that too.)

vomindoraan · 2018-11-09T01:08:34Z

keyboards/s60_x/keymaps/ansi_qwertz/config.h

@@ -6,9 +6,8 @@
 // keymap needs oneshot functionality
 #undef NO_ACTION_ONESHOT

-#undef IS_COMMAND
 #define IS_COMMAND() ( \


Suggested change

#define IS_COMMAND() ( \

This is the same as the existing definition for the keyboard, so this can be removed.

vomindoraan · 2019-01-03T12:34:06Z

keyboards/1upkeyboards/1up60hse/config.h

-/* key combination for magic key command */
-#define IS_COMMAND() ( \
-    keyboard_report->mods == (MOD_BIT(KC_LSHIFT) | MOD_BIT(KC_RSHIFT)) \
-)



I suggest removing the extra newlines as well so we don't end up with lots of files with double empty lines.

Here's a shell script that will find and replace these extra newlines in the files changed by this PR:

for file in $(git diff --numstat master...HEAD | awk '{print $3}'); do tmp="$file.tmp" tr '\n' '\f' < "$file" | sed -e 's/\f\f\f/\f\f/g' | tr '\f' '\n' > "$tmp" && mv "$tmp" "$file" & done

vomindoraan

LGTM, would be cool to see this merged soon.

vomindoraan

I always misclick "Request changes" instead of "Approve" 🙄

vomindoraan · 2019-01-08T20:51:50Z

I found some more candidates for removal:

keyboards/atomic/keymaps/pvc/config.h:91
keyboards/atreus/keymaps/alphadox/config.h:58
keyboards/atreus/keymaps/dvorak_42_key/config.h:84
keyboards/atreus/keymaps/erlandsona/config.h:74
keyboards/comet46/keymaps/satt/config.h:25
keyboards/dz60/keymaps/LEdiodes/config.h:40
keyboards/gh60/keymaps/dbroqua/config.h:100
keyboards/gh60/keymaps/robotmaxtron/config.h:103
keyboards/handwired/downbubble/config.h:134
keyboards/handwired/numbrero/config.h:41
keyboards/handwired/ortho60/config.h:52
keyboards/handwired/tradestation/config.h:41
keyboards/hhkb/keymaps/mjt/config.h:55
keyboards/jd45/keymaps/mjt/config.h:63
keyboards/pinky/3/config.h:135
keyboards/pinky/4/config.h:135
keyboards/planck/keymaps/dodger/config.h:67
keyboards/planck/keymaps/zach/config.h:37
keyboards/preonic/keymaps/kinesis/config.h:60
keyboards/preonic/keymaps/zach/config.h:62
keyboards/rama/m60_a/config.h:55
keyboards/s60_x/keymaps/ansi_qwertz/config.h:9
keyboards/s60_x/keymaps/bluebear/config.h:42
keyboards/satan/keymaps/admiralStrokers/config.h:59
keyboards/satan/keymaps/fakb/config.h:40
keyboards/snagpad/config.h:41
keyboards/whitefox/keymaps/konstantin/config.h:29

vomindoraan · 2019-01-08T21:14:53Z

I've removed these occurrences here: drashna#22

vomindoraan · 2019-01-12T19:18:43Z

Removed double empty lines in modified config.h files in drashna#23.

There's one more redundant IS_COMMAND definition on master now, so I suggest rebasing.

Jan 24th edition

vomindoraan · 2019-01-26T05:48:52Z

One more: https://github.com/qmk/qmk_firmware/blob/master/keyboards/clueboard/66/rev4/config.h#L41-L44

mechmerlin

looks good!

* 'master' of https://github.com/qmk/qmk_firmware: Fix Command feature: use get_mods() instead of keyboard_report->mods (qmk#4955) [Keymap] Small improvements to Maxr1998's Contra keymap (qmk#4952) [Keymap] Minor updates to my dz60 keymap (qmk#4943) [Keyboard] UniGo66 keyboard added (qmk#4913) [Keymap] Move Iris via support to Via keymap (qmk#4893) Adds a default value for IS_COMMAND for COMMAND feature (qmk#4301) [Keyboard] drop unused i2c files (qmk#4948) [Keymap] Add Maxim keymap for Fourier (qmk#4534) use built-in arm stuff [Keymap] Add userspace files for vosechu (qmk#4912)

* Add default value for IS_COMMAND for COMMAND feature * Cleanup and consistency * Update Templates to reflect change * Fix IS_COMMAND in template * Fix IS_COMMAND define * Use consistent IS_COMMAND block in templates * Remove unnecessary `#undef IS_COMMAND` directives * Fix compile issue on orthodox * Reomve IS_COMMAND option for newer boards * Remove all existing definitions of IS_COMMAND if they use default LSHIFT and RSHIFT setting * Remove a couple of additional IS_COMMAND defines * Remove remaining redundant IS_COMMAND definitions * Remove #undef IS_COMMAND from orthodox:drashna and whitefox:konstantin * Remove multiple empty lines in modified config.h files * Update additional boards * Reomve IS_COMMAND from newer boards * Update Alice keyboard * Remove IS_COMMAND from additional boards Jan 24th edition

drashna added core optimization labels Oct 31, 2018

drashna requested review from fredizzimo, jackhumbert, mechmerlin and skullydazed October 31, 2018 01:09

vomindoraan reviewed Oct 31, 2018

View reviewed changes

tmk_core/common/command.c Outdated Show resolved Hide resolved

drashna force-pushed the default_is_command branch from 2b21b18 to 3756601 Compare November 3, 2018 01:11

jackhumbert approved these changes Nov 6, 2018

View reviewed changes

drashna force-pushed the default_is_command branch from f6567d1 to 0786d1b Compare November 8, 2018 16:27

vomindoraan reviewed Nov 8, 2018

View reviewed changes

vomindoraan suggested changes Nov 9, 2018

View reviewed changes

drashna force-pushed the default_is_command branch 2 times, most recently from 560d687 to 9de1a6a Compare December 6, 2018 02:35

jackhumbert approved these changes Dec 14, 2018

View reviewed changes

drashna force-pushed the default_is_command branch from 9de1a6a to 9cefbe8 Compare December 14, 2018 19:12

drashna force-pushed the default_is_command branch from f0a182d to 7e7b388 Compare December 28, 2018 18:27

vomindoraan reviewed Jan 3, 2019

View reviewed changes

drashna force-pushed the default_is_command branch from 0d6d63a to a127a18 Compare January 7, 2019 21:20

vomindoraan suggested changes Jan 8, 2019

View reviewed changes

vomindoraan approved these changes Jan 8, 2019

View reviewed changes

fauxpark mentioned this pull request Jan 15, 2019

Command feature not working since single HID endpoint was introduced #4838

Closed

drashna force-pushed the default_is_command branch from 1ed2ed7 to 8fbe108 Compare January 16, 2019 17:08

Remove IS_COMMAND from additional boards

f450390

Jan 24th edition

drashna added the awaiting review label Jan 26, 2019

mechmerlin approved these changes Jan 26, 2019

View reviewed changes

mechmerlin merged commit b05c0e4 into qmk:master Jan 26, 2019

vomindoraan mentioned this pull request Jan 27, 2019

Fix Command feature: use get_mods() instead of keyboard_report->mods #4955

Merged

13 tasks

drashna deleted the default_is_command branch January 27, 2019 08:10

vomindoraan mentioned this pull request Feb 28, 2019

Remove unnecessary IS_COMMAND definitions from a couple of boards #5269

Merged

noroadsleft mentioned this pull request Mar 25, 2019

Add Collide39 keyboard #5486

Merged

13 tasks

noroadsleft mentioned this pull request Apr 5, 2019

Update hineybush keyboard projects: h87a, hineyg80 #5554

Merged

13 tasks

vomindoraan mentioned this pull request Apr 6, 2019

Remove unnecessary IS_COMMAND definition in treadstone48 #5566

Merged

shelaf added a commit to shelaf/qmk_firmware that referenced this pull request Apr 19, 2019

Fix compilation error of feature qmk#4301

53657c7

drashna pushed a commit that referenced this pull request Apr 19, 2019

[Keymap] Fix compilation error of feature #4301 (#5649)

8aee82e

This was referenced Apr 23, 2019

Add a new keyboard ADKB96 #5685

Merged

omron75 support #5702

Closed

noroadsleft mentioned this pull request May 3, 2019

Kumo Keyboard added with 3 layouts #5770

Closed

13 tasks

foosinn pushed a commit to foosinn/qmk_firmware that referenced this pull request May 6, 2019

[Keymap] Fix compilation error of feature qmk#4301 (qmk#5649)

52b4205

shimesaba-type0 pushed a commit to shimesaba-type0/qmk_firmware that referenced this pull request Jun 22, 2019

[Keymap] Fix compilation error of feature qmk#4301 (qmk#5649)

b0ea45c

Timbus pushed a commit to Timbus/qmk_firmware that referenced this pull request Jun 23, 2019

[Keymap] Fix compilation error of feature qmk#4301 (qmk#5649)

d3c14be

vomindoraan mentioned this pull request Jul 16, 2019

Remove unnecessary IS_COMMAND definitions from a couple of boards #6346

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds a default value for IS_COMMAND for COMMAND feature #4301

Adds a default value for IS_COMMAND for COMMAND feature #4301

drashna commented Oct 31, 2018

vomindoraan commented Oct 31, 2018 •

edited

Loading

drashna commented Oct 31, 2018 •

edited

Loading

vomindoraan commented Oct 31, 2018 •

edited

Loading

vomindoraan commented Nov 4, 2018

vomindoraan Nov 8, 2018

drashna Nov 9, 2018

vomindoraan left a comment

vomindoraan Nov 8, 2018

vomindoraan Nov 9, 2018 •

edited

Loading

vomindoraan Jan 8, 2019 •

edited

Loading

vomindoraan Nov 9, 2018

vomindoraan Jan 3, 2019

vomindoraan left a comment

vomindoraan left a comment

vomindoraan commented Jan 8, 2019 •

edited

Loading

vomindoraan commented Jan 8, 2019

vomindoraan commented Jan 12, 2019

vomindoraan commented Jan 26, 2019 •

edited

Loading

mechmerlin left a comment

		@@ -34,6 +34,7 @@ along with this program. If not, see <http://www.gnu.org/licenses/>.


		/* key combination for magic key command */
		#undef IS_COMMAND

Adds a default value for IS_COMMAND for COMMAND feature #4301

Adds a default value for IS_COMMAND for COMMAND feature #4301

Conversation

drashna commented Oct 31, 2018

vomindoraan commented Oct 31, 2018 • edited Loading

drashna commented Oct 31, 2018 • edited Loading

vomindoraan commented Oct 31, 2018 • edited Loading

vomindoraan commented Nov 4, 2018

vomindoraan Nov 8, 2018

Choose a reason for hiding this comment

drashna Nov 9, 2018

Choose a reason for hiding this comment

vomindoraan left a comment

Choose a reason for hiding this comment

vomindoraan Nov 8, 2018

Choose a reason for hiding this comment

vomindoraan Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

vomindoraan Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

vomindoraan Nov 9, 2018

Choose a reason for hiding this comment

vomindoraan Jan 3, 2019

Choose a reason for hiding this comment

vomindoraan left a comment

Choose a reason for hiding this comment

vomindoraan left a comment

Choose a reason for hiding this comment

vomindoraan commented Jan 8, 2019 • edited Loading

vomindoraan commented Jan 8, 2019

vomindoraan commented Jan 12, 2019

vomindoraan commented Jan 26, 2019 • edited Loading

mechmerlin left a comment

Choose a reason for hiding this comment

vomindoraan commented Oct 31, 2018 •

edited

Loading

drashna commented Oct 31, 2018 •

edited

Loading

vomindoraan commented Oct 31, 2018 •

edited

Loading

vomindoraan Nov 9, 2018 •

edited

Loading

vomindoraan Jan 8, 2019 •

edited

Loading

vomindoraan commented Jan 8, 2019 •

edited

Loading

vomindoraan commented Jan 26, 2019 •

edited

Loading