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.

Sign up for GitHub

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

Jump to bottom

Make default keymap for GMMK Pro reflect stock #13850

Merged
merged 5 commits into from
Aug 9, 2021
Merged

Make default keymap for GMMK Pro reflect stock #13850

merged 5 commits into from
Aug 9, 2021

Conversation

andrebrait
Copy link
Contributor

Description

Since this is the stock keymap for the GMMK Pro, I thought this should likely also be the default keymap on QMK.
This change was also proposed in #13173 and I think it's probably one of the changes that should make it into QMK.

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).

@github-actions github-actions bot added keymap via Adds via keymap and/or updates keyboard for via support labels Aug 2, 2021
@Gigahawk
Copy link

Gigahawk commented Aug 2, 2021

This should be targeting develop until RGB support is merged into master

@andrebrait andrebrait changed the base branch from master to develop August 2, 2021 21:48
@andrebrait
Copy link
Contributor Author

andrebrait commented Aug 2, 2021 •
edited
Loading

@Gigahawk done. I shall resolve the conflicts tomorrow

@andrebrait
Copy link
Contributor Author

@Gigahawk done. All conflicts resolved and I rebased it on current develop.

@Gigahawk
Copy link

Gigahawk commented Aug 3, 2021

Might as well also update the ISO map

@andrebrait
Copy link
Contributor Author

@Gigahawk good point. That totally skipped my mind. I'm gonna do that as well.

@andrebrait
Copy link
Contributor Author

@Gigahawk done

@andrebrait
Copy link
Contributor Author

andrebrait commented Aug 4, 2021 •
edited
Loading

@Gigahawk wanting some input here:

The Stock GMMK firmware uses the left side LEDs to indicate if CAPS LOCK is ON. I figured it'd be nice to have this in the default keymap as well (if RGB is enabled).

I know it can work because I did it for my own keymap (soon to be submitted as a PR), but I wanted to know if you think this is something we'd want in the default keymap or not.

@drashna drashna requested a review from a team August 7, 2021 05:35
@andrebrait
Copy link
Contributor Author

@drashna @fauxpark any comments regarding adding the side LED indicator for CAPS LOCK? It's the same behavior of the stock firmware, when CAPS LOCK is ON it turns on all LEDs on the sides (and strobes them, but I think a solid red or an easily configurable color in the config.h (defaulting to red) is probably better and easier to implement anyway).

I additionally would turn on the LED of the Caps Lock key itself, given the side LEDs alone are frankly not that visible (as you can see, if you Google "GMMK Pro caps leds" you'll find a ton of people who also want that because why wouldn't you turn on the caps lock's key LED when caps is on anyway? It's a weird decision on the - stock firmware). But that's not stock behavior so we can skip that, I think (though I think we can justify deviating here since this is an almost undisputed improvement).

Note: my own keymap only turns on the left side, but stock behavior is to flash both sides. Anyway, I think any caps lock indicator is better than none.

@andrebrait
Copy link
Contributor Author

This isn't implemented here. I'm asking about it so I can do it if everyone agrees it's a good idea

@drashna
Copy link
Member

drashna commented Aug 8, 2021

rgb_matrix_indicator_user would probably be the best way to implement the caps lock, and yeah, having that set on just the caps key should be fine.

@andrebrait
Copy link
Contributor Author

Ok. Will do that and implement the stock behavior as closely as I can.

@andrebrait
Copy link
Contributor Author

We can merge this as is. I'll submit a separate PR for the CAPS LOCK indicators.

@drashna drashna merged commit dbee960 into qmk:develop Aug 9, 2021
@drashna
Copy link
Member

drashna commented Aug 9, 2021

Thanks!

@andrebrait andrebrait deleted the patch-2 branch August 9, 2021 17:09
cadusk pushed a commit to cadusk/qmk_firmware that referenced this pull request Aug 11, 2021
Merge remote-tracking branch 'qmk/develop' into develop
658e0d4 
* qmk/develop: (204 commits)
  Hp69 - Update led logic (qmk#13940)
  Fix shell port bug in computation of QMK_FIRMWARE_DIR (qmk#13950)
  Handwired/Stream_Cheap/2x4: Add via support (qmk#13297)
  add support for m65 and simple 5x13 ortholinear (qmk#12315)
  Improve the qmk lint readme check (qmk#13941)
  Architecture documentation for Configurator and API (qmk#13935)
  Update feature_wpm.md (qmk#13936)
  Add padding to LUFA-MS .BIN files (qmk#13922)
  [Keyboard] Added pistachio_pro (qmk#13466)
  adding my keymap "vayashiko" (qmk#13049)
  Ristretto - Update bootloader (qmk#13933)
  Fix compliation for ferris 0.2 bling (qmk#13937)
  Remove backwards compatibility of debounce names (qmk#13877)
  Remove ONEHAND_ENABLE (qmk#13920)
  Limit RGB max brightness on KPrepublic BM-series keyboards (qmk#13132)
  Fixup rgb matrix config for KBD67 mkII boards (qmk#13931)
  Support all the 0.2 Ferris variants (qmk#12653)
  [User] changes to nstickney's keymaps (qmk#11456)
  [Keymap] new keymap for nui_mini (qmk#13924)
  Make default keymap for GMMK Pro reflect stock (qmk#13850)
  ...
nhongooi pushed a commit to nhongooi/qmk_firmware that referenced this pull request Dec 5, 2021
@andrebrait @nhongooi
Make default keymap for GMMK Pro reflect stock (qmk#13850)
a890c07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drashna drashna drashna approved these changes

@fauxpark fauxpark fauxpark approved these changes

Assignees
No one assigned
Labels
keyboard keymap via Adds via keymap and/or updates keyboard for via support
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@andrebrait @Gigahawk @drashna @fauxpark