Skip to content

Commit

Permalink
Fix weak modifier clear in action macro
Browse files Browse the repository at this point in the history
  • Loading branch information
@tmk
tmk committed Sep 12, 2015
1 parent c2a6c5c commit e852582
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions common/action_macro.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ void action_macro_play(const macro_t *macro_p)
dprintf("KEY_DOWN(%02X)\n", macro);
if (IS_MOD(macro)) {
add_weak_mods(MOD_BIT(macro));
send_keyboard_report();
} else {
register_code(macro);
}
Expand All @@ -51,6 +52,7 @@ void action_macro_play(const macro_t *macro_p)
dprintf("KEY_UP(%02X)\n", macro);
if (IS_MOD(macro)) {
del_weak_mods(MOD_BIT(macro));
send_keyboard_report();
} else {
unregister_code(macro);
}
Expand Down

4 comments on commit e852582

@p3lim
Copy link
Contributor

@p3lim p3lim commented on e852582 Oct 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not solve using Shift with the following macro:

uint8_t shifted = get_mods() & (MOD_BIT(KC_LSHIFT) | MOD_BIT(KC_RSHIFT));
return shifted ?
    (record->event.pressed ? MACRO(T(F20), T(SLSH), D(LSHIFT), T(O), U(LSHIFT), END) : MACRO_NONE) :
    (record->event.pressed ? MACRO(T(F20), T(SLSH), T(O), END) : MACRO_NONE);

F20 is my Compose key, I use this to create upper- and lower case letters specific for my language (æøå).

With my patch it works fine:

From 17fd1c34d5b1f27c3b6a4d4c061364530d522284 Mon Sep 17 00:00:00 2001
From: Adrian L Lange <[email protected]>
Date: Thu, 22 Oct 2015 15:17:36 +0200
Subject: [PATCH 1/1] Fix existing modifiers messing with macros

---
 common/action_macro.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/common/action_macro.c b/common/action_macro.c
index ffaf125..2df166f 100644
--- a/common/action_macro.c
+++ b/common/action_macro.c
@@ -35,6 +35,12 @@ void action_macro_play(const macro_t *macro_p)
     uint8_t interval = 0;

     if (!macro_p) return;
+
+    // remove the current modifiers before running the macro
+    uint8_t old_mods = get_mods();
+    clear_mods();
+    send_keyboard_report();
+
     while (true) {
         switch (MACRO_READ()) {
             case KEY_DOWN:
@@ -76,6 +82,9 @@ void action_macro_play(const macro_t *macro_p)
                 break;
             case END:
             default:
+                // restore the old modifiers after running the macro
+                add_mods(old_mods);
+                send_keyboard_report();
                 return;
         }
         // interval
-- 
2.6.2.windows.1

@tmk
Copy link
Owner Author

@tmk tmk commented on e852582 Oct 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is good idea to remove modifier state automatically like that. People may want to combine their macro with modifier keys. Let's say you define a macro as 'Alt+Tab' for 'Cycle windows' it is natural to expect Shift + the macro works as 'Cycle windows reverse'.

Instead macro commands should be extended so that you can tweak modifier state in your macro definition.
Like, SM(), for store modifier state, RM() for restore modifier state and CM() for clear modifier? Current Macro implementation is clearly poor and should be redesigned from scratch probably.
https://github.com/tmk/tmk_keyboard/blob/master/doc/keymap.md#231-macro-commands

With this extension you will be able to write macro like this:

uint8_t shifted = get_mods() & (MOD_BIT(KC_LSHIFT) | MOD_BIT(KC_RSHIFT));
return shifted ?
    (record->event.pressed ? MACRO(CM(), T(F20), T(SLSH), D(LSHIFT), T(O), U(LSHIFT), RM(), END) : MACRO_NONE) :
    (record->event.pressed ? MACRO(CM(), T(F20), T(SLSH), T(O), RM(), END) : MACRO_NONE);

I'll file this as an issue, thanks.

@tmk
Copy link
Owner Author

@tmk tmk commented on e852582 Oct 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed that in #116.

@p3lim
Copy link
Contributor

@p3lim p3lim commented on e852582 Oct 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead macro commands should be extended so that you can tweak modifier state in your macro definition.
Like, SM(), for store modifier state, RM() for restore modifier state and CM() for clear modifier?

That would do nicely. The main reason why I needed this in my macro is because the uppercase version of the symbol would also add the shift state to the / symbol, which would en up being ?.

I would suggest that SM/RM/CM should be able to take a value (or multiple) for storing/restoring/clearing specific and/or multiple modifiers, the default behavior being all modifiers.

Please sign in to comment.