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

Remove redundant, language-specific aliases for KC_ALGR #4720

Merged
merged 3 commits into from
Jan 3, 2019
Merged

Remove redundant, language-specific aliases for KC_ALGR #4720

merged 3 commits into from
Jan 3, 2019

Conversation

vomindoraan
Copy link
Contributor

@vomindoraan vomindoraan commented Dec 24, 2018 •
edited
Loading

This PR continues from where #4389 left off. Namely, it removes redundant, language-specific aliases for KC_ALGR, since that keycode is standard now (see the aforementioned PR). This is done to improve consistency throughout keymaps and the codebase as a whole.

Description

There is no national layout1 in which AltGr is anything other than right Alt. Therefore, language-specific aliases for KC_ALGR are unnecessary. This PR removes DE_ALGR, FR_ALGR, FR_CH_ALGR etc. and replaces uses thereof with KC_ALGR. Also, two of the language keymaps defined the same ALTGR(kc) alias for ALGR(kc), which I've replaced as well.

1 Other than maybe the German NEO layout, but that one has its own modifiers (NEO_L1_L, NEO_L1_R, NEO_L2_L, NEO_L2_R), which I haven't touched.

Types of changes

  • Core
  • Bugfix
  • New Feature
  • Enhancement/Optimization
  • Keyboard (addition or update)
  • Keymap/Layout/Userspace (addition or update)
  • Documentation

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document. (https://docs.qmk.fm/#/contributing)
  • 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).

@drashna
Copy link
Member

drashna commented Dec 24, 2018

Honestly, I think we should keep the language specific keycodes, but they should be using the correct KC_ versions.

That way, they fit better into the keymap_extras stuff.

@vomindoraan
Copy link
Contributor Author

vomindoraan commented Dec 24, 2018 •
edited
Loading

It's usually not done for any of the other modifiers, though, nor is it done for keycodes like KC_SPACE etc. The KC_ALGR aliases are a holdover from before KC_ALGR was added as a standard keycode and every extra keymap had to define it separately. Therefore, I think it would be okay to remove them.

Speaking of which, I just noticed these; they should probably be removed too for the same reason. Should be okay as they aren't used anywhere.

Edit: But then there are these... I don't think that's consistent with the rest of the extra keymaps.

@vomindoraan
Copy link
Contributor Author

vomindoraan commented Dec 31, 2018 •
edited
Loading

Updated to latest master and removed two more redundant aliases (BE_LALT and BE_LGUI).

The plethora of redundant aliases in the UK keymap can stay for now.

mechmerlin
mechmerlin approved these changes Jan 3, 2019
View reviewed changes
Copy link
Contributor

@mechmerlin mechmerlin left a comment

Choose a reason for hiding this comment

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

Looks good.

@mechmerlin mechmerlin merged commit e76bf17 into qmk:master Jan 3, 2019
rseymour pushed a commit to rseymour/qmk_firmware that referenced this pull request Mar 13, 2019
@vomindoraan @rseymour
Remove redundant, language-specific aliases for KC_ALGR (qmk#4720)
45c8b85 
* Use standard KC_ALGR, remove language-specific redefinitions

* Use ALGR instead of ALTGR in BÉPO and Canadian multilingual keymaps

* Remove BE_LALT, BE_LGUI aliases
djthread pushed a commit to djthread/qmk_firmware that referenced this pull request Mar 17, 2019
@vomindoraan @djthread
Remove redundant, language-specific aliases for KC_ALGR (qmk#4720)
c77dc20 
* Use standard KC_ALGR, remove language-specific redefinitions

* Use ALGR instead of ALTGR in BÉPO and Canadian multilingual keymaps

* Remove BE_LALT, BE_LGUI aliases
dlhextall pushed a commit to dlhextall/qmk_firmware that referenced this pull request May 24, 2019
@vomindoraan
Remove redundant, language-specific aliases for KC_ALGR (qmk#4720)
77ef56b 
* Use standard KC_ALGR, remove language-specific redefinitions

* Use ALGR instead of ALTGR in BÉPO and Canadian multilingual keymaps

* Remove BE_LALT, BE_LGUI aliases
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

@mechmerlin mechmerlin mechmerlin approved these changes

@ezuk ezuk Awaiting requested review from ezuk

@fredizzimo fredizzimo Awaiting requested review from fredizzimo

@jackhumbert jackhumbert Awaiting requested review from jackhumbert

@skullydazed skullydazed Awaiting requested review from skullydazed

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vomindoraan @drashna @mechmerlin