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

Update Swiss French/German keymaps and add sendstring LUTs #8689

Merged
merged 4 commits into from
Apr 18, 2020

Conversation

fauxpark
Copy link
Member

@fauxpark fauxpark commented Apr 5, 2020

Description

🇨🇭 (🇫🇷/🇩🇪)

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.
  • 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 fauxpark requested a review from a team April 5, 2020 14:47
@vomindoraan
Copy link
Contributor

vomindoraan commented Apr 5, 2020

It would be nice to see {keymap,sendstring}_fr_ch.h renamed to {keymap,sendstring}_french_ch.h for consistency. It's only used in a couple of community layouts as far as I can tell, so renaming it shouldn't be too bad.

@vomindoraan
Copy link
Contributor

vomindoraan commented Apr 5, 2020

Consider using CH_* instead of FR_* and DE_*, since the Swiss layouts are more closely related to each other than they are to the regular French and German layouts. Also, CH_* would be in line with the current practice of using country codes as prefixes, whereas FR_* and DE_* would not.

@fauxpark
Copy link
Member Author

fauxpark commented Apr 5, 2020

I would probably want to rename the whole lot to match fr_ch, but that would be quite the breaking change. For the moment, I could rename these to {keymap,sendstring}_{french,german}_swiss.h and have the original headers simply include the correct ones, perhaps with a #pragma message indicating they are deprecated.

@vomindoraan
Copy link
Contributor

vomindoraan commented Apr 5, 2020

That sounds good to me.

In my opinion, the best solution in the long term would be to name the files by locale (as discussed in #8560) and the keycodes by country code (for brevity). So Swiss French would use keymap_fr_ch.h, as you suggested, with CH_* keycodes; Serbian Latin would use keymap_sr_latn.h with RS_* keycodes; Brazilian Portuguese would use keymap_pt_br.h with BR_* keycodes; etc.
We could implement this in the upcoming breaking change, and I'd be glad to assist you.

For the time being, though, {keymap,sendstring}_{french,german}_swiss.h is the way to go.

@vomindoraan
Copy link
Contributor

LGTM, just rename the files when you get the chance.

@drashna drashna requested a review from a team April 12, 2020 17:50
@fauxpark fauxpark merged commit 89eb3a9 into qmk:master Apr 18, 2020
@fauxpark fauxpark deleted the keymap-extras-swiss branch April 18, 2020 20:56
kylekuj pushed a commit to kylekuj/qmk_firmware that referenced this pull request Apr 21, 2020
violet-fish pushed a commit to violet-fish/qmk_firmware that referenced this pull request May 3, 2020
bitherder pushed a commit to bitherder/qmk_firmware that referenced this pull request May 15, 2020
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request May 24, 2020
sowbug pushed a commit to sowbug/qmk_firmware that referenced this pull request May 24, 2020
fdidron pushed a commit to zsa/qmk_firmware that referenced this pull request Jun 12, 2020
turky pushed a commit to turky/qmk_firmware that referenced this pull request Jun 13, 2020
jakobaa pushed a commit to jakobaa/qmk_firmware that referenced this pull request Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants