Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New keymap for ryanbaekr/rb87 #23713

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

ryanbaekr
Copy link
Contributor

Description

Added a new keymap for ryanbaekr/rb87 that emulates my most used emacs shortcuts

IF THIS IS NOT CONSIDERED A VENDOR KEYMAP, PLEASE EXPLAIN WHY

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@fauxpark
Copy link
Member

Added a new keymap for ryanbaekr/rb87 that emulates my most used emacs shortcuts

Sounds like a personal keymap to me.

@ryanbaekr
Copy link
Contributor Author

Added a new keymap for ryanbaekr/rb87 that emulates my most used emacs shortcuts

Sounds like a personal keymap to me.

So the criteria is based on sound and not whether or not it's from the vendor? That feels wrong but it's not my call

@fauxpark
Copy link
Member

It's neither. A "vendor" keymap is a/the keymap that ships with the board, which may not necessarily be the same as the default keymap.

@ryanbaekr
Copy link
Contributor Author

It's neither. A "vendor" keymap is a/the keymap that ships with the board, which may not necessarily be the same as the default keymap.

That's what this is

@tzarc
Copy link
Member

tzarc commented May 16, 2024

Then it needs to be renamed to via_ryanbaekr, as per the PR checklist. Also, the unused combo feature should be removed, as well as empty layouts.

@ryanbaekr
Copy link
Contributor Author

Then it needs to be renamed to via_ryanbaekr, as per the PR checklist. Also, the unused combo feature should be removed, as well as empty layouts.

Will do on the name

Combos must be enabled for repeat key. This is well documented here

Add it to your keymap

If you are new to QMK macros, see my macro buttons post for an intro.

Step 1: Repeat Key requires that Combos are enabled. If you aren’t using Combos already, set in rules.mk:

COMBO_ENABLE = yes

It is not required that you use combos for anything in your keymap. But at a minimum, you need to define an empty key_combos array by adding the following in keymap.c:

combo_t key_combos[] = {};
uint16_t COMBO_LEN = 0;

Step 2: In your keymap.c, add a custom keycode for the Repeat Key and use the new keycode in your layout. I’ll name it REPEAT, but you can call it anything you like.

enum custom_keycodes {
REPEAT = SAFE_RANGE,
// Other custom keys...
};

It used to be the case that via keymaps had to have 4 layers. I see that is no longer the case, will do

@tzarc
Copy link
Member

tzarc commented May 17, 2024

Compiles fine without combos enabled. Also, there's no mention on the QMK documentation of requiring combos.
Perhaps the documentation you're referring to should be considered historical?

@tzarc
Copy link
Member

tzarc commented May 17, 2024

Your docs:

An essential piece to making this work is that we set the desired alternate keycode in the keyrecord_t’s .keycode field, and this field is only present when Combos are enabled.

QMK Code:

typedef struct {
    keyevent_t event;
#ifndef NO_ACTION_TAPPING
    tap_t tap;
#endif
#if defined(COMBO_ENABLE) || defined(REPEAT_KEY_ENABLE)
    uint16_t keycode;
#endif
} keyrecord_t;

Seems like combos aren't required.

@ryanbaekr
Copy link
Contributor Author

ryanbaekr commented May 17, 2024

Compiles fine without combos enabled. Also, there's no mention on the QMK documentation of requiring combos. Perhaps the documentation you're referring to should be considered historical?

It compiles fine but the feature does not work on the board when flashed

To clarify, QK_REP works, get_last_keycode() does not

@tzarc
Copy link
Member

tzarc commented May 18, 2024

To clarify, QK_REP works, get_last_keycode() does not

Can't reproduce this I'm afraid. Implemented your repeat keycodes identically and it works fine without any combos.

        case TZ_REP_2 ... TZ_REP_5:
            if (record->event.pressed) {
                for (int i = TZ_REP_2 - 1; i <= keycode; i++) {
                    tap_code16(get_last_keycode());
                }
            }
            return false;

...where TZ_REP_2 would be the equivalent of your M2, etc.

@ryanbaekr
Copy link
Contributor Author

I believe the last commit addresses your concerns

@ryanbaekr
Copy link
Contributor Author

I believe the last commit addresses your concerns

@tzarc

@ryanbaekr
Copy link
Contributor Author

I believe the last commit addresses your concerns

@tzarc

@tzarc @fauxpark

@tzarc
Copy link
Member

tzarc commented Jun 11, 2024

QMK is an open-source project run by volunteers. This is a low-priority item. It can wait its turn.

To gain some perspective, please have a read of these links:
GitHub Etiquette
Don't be that open-source user, don't be me
Open Source Maintainers Owe You Nothing
Entitlement in Open Source

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants