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

DZ60 keymap and layout #5474

Merged
merged 20 commits into from
May 4, 2019
Merged

DZ60 keymap and layout #5474

merged 20 commits into from
May 4, 2019

Conversation

OlliGranlund
Copy link
Contributor

@OlliGranlund OlliGranlund commented Mar 23, 2019

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

.gitignore Outdated Show resolved Hide resolved
keyboards/crkbd/keymaps/olligranlund_nordic/keymap.c Outdated Show resolved Hide resolved
keyboards/crkbd/keymaps/olligranlund_nordic/keymap.c Outdated Show resolved Hide resolved
keyboards/dz60/dz60.h Outdated Show resolved Hide resolved
keyboards/satan/keymaps/olligranlund_iso/keymap.c Outdated Show resolved Hide resolved
keyboards/satan/keymaps/olligranlund_iso/rules.mk Outdated Show resolved Hide resolved
@drashna
Copy link
Member

drashna commented Mar 24, 2019

You've opened this pull request from your fork's master branch. While this isn't a major issue now, later changes may result in merge conflicts if you try to file another PR because the commit history will have diverged between your master and QMK's. It is highly recommended for QMK development - regardless of what is being done or where - to keep your master updated, but NEVER commit to it. Instead, do all your changes in a branch (branches are basically free in Git) and issue PRs from your branches when you're developing.

There are instructions on how to keep your master updated here:

Best Practices: Your fork's master: Update Often, Commit Never

If you need any help with this just ask.

You do not need to close this PR. Further changes should still be pushed to your master branch, until this PR is merged.

@OlliGranlund
Copy link
Contributor Author

Alright, so apparently my iris keymap had gotten in as a duplicate to the wrong folder, no idea how this happened... I removed that since it was irrelewant for crkbd. Reviewed the changes you requested and made changes accordingly. I had to create a new DZ60 layout due to there not being a ISO layout with split backspace and split right shift. I now added also the json-data of my layout. Also cleaned layout to match 1:1 the visual representation of layouts.

I couldn't remember how I did the PR last time so that's why some of this mess. Should I now wait for you to accept this PR or should I create a new branch and push it to qmk-repo and request that to be merged?

@mechmerlin
Copy link
Contributor

Unless this new layout name is a community accepted standard, I am opposed to having this new layout named with your user name. Can you think of a more generic name for this or make any of the existing layout macros work with your keymap?

@OlliGranlund
Copy link
Contributor Author

I could come up with a better name for it, however I've just followed how the previous layouts have been made eg. "LAYOUT_60_tsangan_hhkb" has exactly as much information as my layout. Do you still feel like I should rename my layout to something like "LAYOUT_60_iso_split_backspace_rshift"?

@noroadsleft
Copy link
Member

LAYOUT_60_iso_split_backspace_rshift

Most other 60% keyboards that I know of would use LAYOUT_60_iso_split_bs_rshift (bs being Backspace here) for that. You've got split Space as well, but I don't know that there's a custom for that as far as naming.

@OlliGranlund
Copy link
Contributor Author

Alrighty! Renamed the layout to something more generic, so I guess this should be fine now? How should we proceed with this merge, it's fine like this or should I do these changes via a separate branch and make PR for that?

@drashna
Copy link
Member

drashna commented Mar 25, 2019

Could you remove the .vscode/ipch/50bbb6394ee57e02/DZ60.ipch file?

@OlliGranlund
Copy link
Contributor Author

Damn, did that get in there now too... Well I removed the file as requested.

@@ -48,6 +48,9 @@
#define CALC KC_CALC // Open default calculator app
#define MYCM KC_MYCM // Open default file manager

// increase readability
#define XXXXX KC_NO
Copy link
Member

@zvecr zvecr Mar 26, 2019

Choose a reason for hiding this comment

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

Is this required when XXXXXXX can be used for free as its provided along with ______ for KC_TRANS?

Suggested change
#define XXXXX KC_NO

Copy link
Member

Choose a reason for hiding this comment

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

Well, it's not predefined, like XXXXXXX, so it depends on what you want.

Ideally, I think we should use the built in one.

Copy link
Member

Choose a reason for hiding this comment

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

I would also prefer use of the seven-character alias defined in QMK core.

Copy link
Member

Choose a reason for hiding this comment

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

@OlliGranlund I recommend you rewrite this keymap to use the LAYOUT_60_iso macro. (Your existing readme suggests that's the layout you're running anyway.) Right now this keymap is based on outdated conventions, and wouldn't compile from a current version of QMK if merged as-is.

@OlliGranlund
Copy link
Contributor Author

As my GH60 if fried, I can't test the compiled hex if it works as expected...

@drashna
Copy link
Member

drashna commented Mar 27, 2019

Oh no, I'm very sorry to hear that! :(

@IsaacElenbaas IsaacElenbaas mentioned this pull request Mar 31, 2019
13 tasks
Co-Authored-By: OlliGranlund <[email protected]>
@OlliGranlund
Copy link
Contributor Author

It's been a while. Yeah, my GH60 board is not working anymore and nothing seems to help for it, hence I would rather not do any more changes to my Satan keymap as it might break things. I've done the changes as requested, how should we go on with this merge? Do you want me to make it into a separate branch and PR or is it fine just in here after all the changes?

@drashna
Copy link
Member

drashna commented Apr 4, 2019

This should be fine. @mechmerlin @noroadsleft any issues with the layout changes?

@OlliGranlund
Copy link
Contributor Author

This still seems to be pending :)

@mechmerlin
Copy link
Contributor

This should be fine. @mechmerlin @noroadsleft any issues with the layout changes?

looks good to me!

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.

all good, but looks like someone's keymap is over the limit!

@noroadsleft
Copy link
Member

This should be fine. @mechmerlin @noroadsleft any issues with the layout changes?

The DZ60 layout changes are good, but the Satan keymap actually won't compile as-is when this is merged into master.

@@ -48,6 +48,9 @@
#define CALC KC_CALC // Open default calculator app
#define MYCM KC_MYCM // Open default file manager

// increase readability
#define XXXXX KC_NO
Copy link
Member

Choose a reason for hiding this comment

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

@OlliGranlund I recommend you rewrite this keymap to use the LAYOUT_60_iso macro. (Your existing readme suggests that's the layout you're running anyway.) Right now this keymap is based on outdated conventions, and wouldn't compile from a current version of QMK if merged as-is.

@noroadsleft
Copy link
Member

It's been a while. Yeah, my GH60 board is not working anymore and nothing seems to help for it, hence I would rather not do any more changes to my Satan keymap as it might break things. I've done the changes as requested, how should we go on with this merge? Do you want me to make it into a separate branch and PR or is it fine just in here after all the changes?

I forgot that your Satan is out of commission. Please disregard my comments regarding rewriting that keymap.

@OlliGranlund
Copy link
Contributor Author

Hmm, so we are still waiting for yanfali:s approval?

@drashna
Copy link
Member

drashna commented May 4, 2019

Thanks!

@drashna drashna merged commit b9f060c into qmk:master May 4, 2019
foosinn pushed a commit to foosinn/qmk_firmware that referenced this pull request May 6, 2019
* init

* function layout planning

* nordic keymap v1.0

* Added latest satan layouts, updated readmes

* Cleaning code

* Renamed ISO -> iso

* Updated keymap to work better with Swedish layout

* merge conflict

* Added dz60 layout

* pr issues fixes

* removed weirdly positioned files

* code cleanup, added dz60 layout json data

* Added dz60 layout readme

* Renamed layout

* removed vscode file

* Update keyboards/dz60/dz60.h

Co-Authored-By: OlliGranlund <[email protected]>
KauyonKais pushed a commit to KauyonKais/qmk_firmware that referenced this pull request May 8, 2019
* init

* function layout planning

* nordic keymap v1.0

* Added latest satan layouts, updated readmes

* Cleaning code

* Renamed ISO -> iso

* Updated keymap to work better with Swedish layout

* merge conflict

* Added dz60 layout

* pr issues fixes

* removed weirdly positioned files

* code cleanup, added dz60 layout json data

* Added dz60 layout readme

* Renamed layout

* removed vscode file

* Update keyboards/dz60/dz60.h

Co-Authored-By: OlliGranlund <[email protected]>
KauyonKais pushed a commit to KauyonKais/qmk_firmware that referenced this pull request May 8, 2019
* init

* function layout planning

* nordic keymap v1.0

* Added latest satan layouts, updated readmes

* Cleaning code

* Renamed ISO -> iso

* Updated keymap to work better with Swedish layout

* merge conflict

* Added dz60 layout

* pr issues fixes

* removed weirdly positioned files

* code cleanup, added dz60 layout json data

* Added dz60 layout readme

* Renamed layout

* removed vscode file

* Update keyboards/dz60/dz60.h

Co-Authored-By: OlliGranlund <[email protected]>
Timbus pushed a commit to Timbus/qmk_firmware that referenced this pull request Jun 23, 2019
* init

* function layout planning

* nordic keymap v1.0

* Added latest satan layouts, updated readmes

* Cleaning code

* Renamed ISO -> iso

* Updated keymap to work better with Swedish layout

* merge conflict

* Added dz60 layout

* pr issues fixes

* removed weirdly positioned files

* code cleanup, added dz60 layout json data

* Added dz60 layout readme

* Renamed layout

* removed vscode file

* Update keyboards/dz60/dz60.h

Co-Authored-By: OlliGranlund <[email protected]>
ridingqwerty pushed a commit to ridingqwerty/qmk_firmware that referenced this pull request Jan 10, 2020
* init

* function layout planning

* nordic keymap v1.0

* Added latest satan layouts, updated readmes

* Cleaning code

* Renamed ISO -> iso

* Updated keymap to work better with Swedish layout

* merge conflict

* Added dz60 layout

* pr issues fixes

* removed weirdly positioned files

* code cleanup, added dz60 layout json data

* Added dz60 layout readme

* Renamed layout

* removed vscode file

* Update keyboards/dz60/dz60.h

Co-Authored-By: OlliGranlund <[email protected]>
JeffreyPalmer pushed a commit to JeffreyPalmer/qmk_firmware that referenced this pull request Feb 27, 2020
* init

* function layout planning

* nordic keymap v1.0

* Added latest satan layouts, updated readmes

* Cleaning code

* Renamed ISO -> iso

* Updated keymap to work better with Swedish layout

* merge conflict

* Added dz60 layout

* pr issues fixes

* removed weirdly positioned files

* code cleanup, added dz60 layout json data

* Added dz60 layout readme

* Renamed layout

* removed vscode file

* Update keyboards/dz60/dz60.h

Co-Authored-By: OlliGranlund <[email protected]>
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.

5 participants