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

Allow changes to the moonlander default music map #18715

Merged
merged 1 commit into from
Oct 16, 2022

Conversation

joukewitteveen
Copy link
Contributor

Description

A music map where each row has the same length is a lot more intuitive. With a chromatic scale, having rows of twelve tones is especially nice.

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

24, 25, 26, 27, 28, 29, 29, 30, 30, 31, 32, 33, 34, 35,
12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23,
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11,
5, 5, 5, 6, 6, 6
Copy link
Member

Choose a reason for hiding this comment

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

These really should be different values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? Try it, it makes a lot of sense. Sure, the hit-area for 53, 54, 41, 42, 29, 30, 5, and 6 is rather large, but these are also the hardest to hit from the resting position on the home row.

Copy link
Member

Choose a reason for hiding this comment

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

Hardest to hit is absolutely subjective. Eg, on mine, hitting the different thumb keys is very easy for me, as my thumb sits in the center key in the thumb cluster, when my fingers are on the home row. And I'm very used to using my thumb a lot. So setting all of these keys to be the same thing is a literal waste.

And while I definitely understand that not everyone uses the board like me, ... well, that's still my point: that somebody likely will want to have these as different notes/positions.

What might be better/simpler in the long run is adding __attribute__((weak)) to the variable, so that it can be changed in a users keymap.

Copy link
Contributor Author

@joukewitteveen joukewitteveen Oct 14, 2022

Choose a reason for hiding this comment

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

Fair enough. I just thought a highly structured layout (basically five rows of 12 tones each) was a better default.
If you undef MUSIC_MAP, wouldn't that allow you to define a music_map of your own in your keymap? edit: I misread the code and will change this PR into one that makes the map weak.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed the PR so that it no longer picks a different default, but rather allows redefinition by the addition of __attribute__((weak)). Thanks for the suggestion, @drashna!

This allows user keymaps to set a different music map.
@joukewitteveen joukewitteveen changed the title Change moonlander keyboard default music map Allow changes to the moonlander default music map Oct 15, 2022
@drashna drashna requested a review from a team October 15, 2022 23:24
@zvecr zvecr merged commit d089af8 into qmk:develop Oct 16, 2022
ramonimbao pushed a commit to ramonimbao/qmk_firmware that referenced this pull request Nov 28, 2022
This allows user keymaps to set a different music map.
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.

4 participants