qmk / qmk_firmware

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

OSL not working properly when autoshift is enabled #4629

Closed danielo515 closed 5 years ago

danielo515 commented 5 years ago

Describe the bug

When I have the autoshift enabled (hold long for shift a key) the OSL modifiers are not working as expecting. What I expect is to hit the OSL, go to the desired layer, hit one keystroke and come back. However, if the key I hit is not excluded from autoshift then it does not come back to the original layer, I have to hit one of the keys excluded from autoshift (in my case letters) to get out of the layer.

Also, OSL keys started to behave weird, in the sense that I can not hit again to deactivate the layer, i have to hit it several times once it finally decides to come back to the original layer, usually I have to tap it twice in a row and then on the third tap it comes back. I have the ONESHOT_TAP_TOGGLE set to two.

Regards

System Information

drashna commented 5 years ago

Yeah, this is probably normal.

Specifically, the autoshift function returns false, at one point. So this would block the One Shot layer from clearing properly.

On this line: https://github.com/qmk/qmk_firmware/blob/master/quantum/process_keycode/process_auto_shift.c#L197

Try adding this before the return false; and see if that fixes the issue:

#if !defined(NO_ACTION_ONESHOT) && !defined(NO_ACTION_TAPPING)
set_oneshot_layer(ONESHOT_PRESSED);
#endif 

that, or apply this diff:

diff --git a/quantum/process_keycode/process_auto_shift.c b/quantum/process_keycode/process_auto_shift.c
index 0d0930ee6..cd58e549c 100644
--- a/quantum/process_keycode/process_auto_shift.c
+++ b/quantum/process_keycode/process_auto_shift.c
@@ -194,6 +194,9 @@ bool process_auto_shift(uint16_t keycode, keyrecord_t *record) {
 #endif

         autoshift_on(keycode);
+        #if !defined(NO_ACTION_ONESHOT) && !defined(NO_ACTION_TAPPING)
+          set_oneshot_layer(ONESHOT_PRESSED);
+        #endif
         return false;

       default:
danielo515 commented 5 years ago

Hello @drashna , thank you for your prompt response. When I add your code I get this error while compiiling:

code/process_auto_shift.c: In function 'process_auto_shift':
quantum/process_keycode/process_auto_shift.c:198:9: error: too few arguments to function 'set_oneshot_layer'
         set_oneshot_layer(ONESHOT_PRESSED);
         ^
drashna commented 5 years ago

That's what I get for not testing this

drashna commented 5 years ago

The command should actually be clear_oneshot_layer_state(ONESHOT_PRESSED);

danielo515 commented 5 years ago

Ook, that makes more sense. I'll try it out. I tried looking at the documentation, but I didn't found that particular function, just the cancel one and I was not sure about it

drashna commented 5 years ago

Yeah, the issue is that the function calls return false, so it doesn't process the keypress properly.

I had this issue with my custom macros, too. The simple solution is to return true, but that DOES NOT work here. So, you have to tell one_shots, that it's been pressed.

danielo515 commented 5 years ago

Hello @drashna , With the suggested change it now compiles, but the behavior remains the same. I don't know if this makes a difference, but I'm using autoshift for numbers and symbols.

This is the code I wrote at the lines you specified


        autoshift_on(keycode);
      #if !defined(NO_ACTION_ONESHOT) && !defined(NO_ACTION_TAPPING)
        clear_oneshot_layer_state(ONESHOT_PRESSED);
      #endif
        return false;

      default:
        autoshift_flush();
        return true;
drashna commented 5 years ago

I'll have to see about testing this....

apalugniok commented 5 years ago

I have also just experienced this issue. Doing this worked for me. Not sure if this is how it's meant to be solved.

autoshift_on(keycode);
      #if !defined(NO_ACTION_ONESHOT) && !defined(NO_ACTION_TAPPING)
        clear_oneshot_layer_state(ONESHOT_OTHER_KEY_PRESSED);
      #endif
        return false;

      default:
        autoshift_flush();
        return true;
drashna commented 5 years ago

Double checking the code, that is the correct solution.