Qmk_firmware: Auto Shift: emit holding key before release

[guo-yong-zhi]

Feature Request Type

• [ ] Core functionality
• [ ] Add-on hardware support (eg. audio, RGB, OLED screen, etc.)
• [ ] Alteration (enhancement/optimization) of existing feature(s)
• [x] New behavior

Description

Currently, Auto Shift didn't send the capital char to screen until the key released. If we do it once taping-time-out, users will get a feedback that they do the long pressing successfully. That's also the holding processing strategy in Tap Dance.
That can be done with a timing key-flushing task. Here's my solution:

1. Add a autoshift_holding_flush function in qmk_firmware/quantum/process_keycode/process_auto_shift.c

void autoshift_holding_flush(void) {
  if (autoshift_lastkey != KC_NO) {
    uint16_t elapsed = timer_elapsed(autoshift_time);

    if (elapsed > autoshift_timeout) {
        register_code(KC_LSFT);

        register_code(autoshift_lastkey);
        unregister_code(autoshift_lastkey);

        unregister_code(KC_LSFT);
        autoshift_time = 0;
        autoshift_lastkey = KC_NO;
    }
  }
}

1. Call it repeatedly by add it to matrix_scan_user (at keymap.c).

void autoshift_holding_flush(void);
void matrix_scan_user(void) {
    autoshift_holding_flush();
    //someting else...
}

It would be better if someone could turn it into a simple configurable switch.

[FilipParyz]

What do you want exactly to be done? As you've resolved the problem already from what I can see, what else do you have on your mind? Would you like a single #define to turn that functionality on or off?

[guo-yong-zhi]

What do you want exactly to be done? As you've resolved the problem already from what I can see, what else do you have on your mind? Would you like a single #define to turn that functionality on or off?

Yes, something like AUTO_SHIFT_TIMEOUT and NO_AUTO_SHIFT_SPECIAL.

[cari66ean]

I have just finally decided to give Auto-Shift a try and found it weird that I have to release the key for the shifted character to appear. It makes me hold the desired key artificially longer than I probably have to because I want to make sure I'm holding it long enough.

From what I can see there's no real reason for Auto Shift to wait for release of the key, so in my opinion it should be the default behavior that the shifted key appears as soon as AUTO_SHIFT_TIMOUT is reached.

[drashna]

It may be more complicated, but I think the flush should just register the keycode. Let the process_record_user function handle the upstroke. Or you have the same issue.

[stale[bot]]

This issue has been automatically marked as stale 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.
For maintainers: Please label with bug, in progress, on hold, discussion or to do to prevent the issue from being re-flagged.

[macintacos]

I'd still really like for this to be addressed, so please don't close this stale bot 🙂

[cari66ean]

I've been using the solution suggested by OP for a few weeks now and it's been working perfectly fine so far. In my opinion this is the way AutoShift should be working. Anything I'm missing?

[IsaacElenbaas]

It may be more complicated, but I think the flush should just register the keycode. Let the process_record_user function handle the upstroke. Or you have the same issue.

@drashna What do you mean 'you have the same issue'? If you mean the physical upstroke, that would cause keyrepeat to occur for shifted keycodes, but either way that's not how AutoShift currently works. process_record_user is not triggered for any AutoShift keys. Both would break my keymap - I have very low keyrepeat delay because my typing keys don't have it, and timing the release would be difficult. I also have macros set for A-Z because they can only be triggered on my layer where AutoShift is disabled.

[p00ya]

+1 to not sending unregister_code from the matrix_scan, and for reworking process_auto_shift.c generally so it leverages the logic in action_tapping.c. In particular, having tap-then-hold behaviour (like dual-function keys without TAPPING_FORCE_HOLD) would be possible. It also avoids bugs (like forgetting to add TAP_CODE_DELAY). I can send a PR along these lines, though it wouldn't be a trivial change like the OP.

[stale[bot]]

This issue has been automatically marked as stale 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.
For maintainers: Please label with bug, in progress, on hold, discussion or to do to prevent the issue from being re-flagged.

[p00ya]

!stale - waiting for maintainers to look at the PR (though I've given up keeping in sync with qmk_firmware/develop until someone is actually looking).

[favetelinguis]

Looks like change have been approved. Any eta when this Will get merged?

[p00ya]

FWIW, I've rebased the PR on develop again, so it should apply cleanly. Still waiting for the QMK collaborators to merge it, perhaps @noroadsleft can give an ETA.

@IsaacElenbaas - any chance you can test in the meantime? The rebase had a bunch of conflicts (not 100% confident I merged them all right) and it's been a while since we manually tested anyway.

[IsaacElenbaas]

I should have some free time today to go through edge cases. I used the custom shifts branch, which really doesn't have much changed (and no difference in logic/program flow) (lol yes it does, my memory is just bad), on my travel board for the last week and saw no issues.

[IsaacElenbaas]

autoshift_on was (I assume accidentally) readded in rebase. It's not in .h so should be removed.
autoshift_timer_report was as well, and I don't remember removing it, but I checked and it was definitely me. Should be there.
Aside from that there are no differences between the cleanup branch I didn't delete yet for this purpose and the current pull. Will still go through edge cases later.

[IsaacElenbaas]

Everything I can think to test works. Only things I think could even be addressed before it's merged is the define names.

[p00ya]

autoshift_on was (I assume accidentally) readded in rebase. It's not in .h so should be removed.

Okay, I've removed autoshift_on again. This is a funny one because even though it was never in the header, it had external linkage. Doesn't look like anybody is calling it in qmk_firmware/master, though.

autoshift_timer_report was as well, and I don't remember removing it, but I checked and it was definitely me. Should be there.

Yep, we should keep autoshift_timer_report, though IMO the whole setup/timer tuning thing becomes unnecessary with this issue getting resolved, since users get immediate visual feedback.

Everything I can think to test works. Only things I think could even be addressed before it's merged is the define names.

Which preprocessor names would you change?

[IsaacElenbaas]

Personally I think they're all fine, but you had suggested merging AUTO_SHIFT_REPEAT into TAPPING_FORCE_HOLD and AUTO_SHIFT_NO_AUTO_REPEAT is rather awkward to the point that I have to reread it a few times even though its meaning is obvious.

[fauxpark]

Closing, apparently #9826 has resolved this.