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 Configurator fix and refactor #3205

Merged
merged 5 commits into from
Jun 21, 2018
Merged

Conversation

noroadsleft
Copy link
Member

Configurator support for DZ60 was semi-broken in #3198 due to an invalid JSON file. I've fixed that error and added another matrix for a standard ISO layout.

Changes

The matrix that was LAYOUT_ISO is renamed to LAYOUT_60_iso_5x1u

The matrix/layout submitted in #3198 was standard ISO, except for there being five 1u keys to the right of the spacebar on the bottom row, rather than the standard four 1.25u keys. In an attempt to reduce the possibility of user confusion, this matrix/layout was renamed — it would have otherwise been the only layout labeled as ISO, but the non-standard layout would yield an extra key.

The keymap submitted with #3198 has also been refactored to use the renamed matrix.

New matrix: LAYOUT_60_iso

This is a standard 62-key ISO 60% layout.

New rule: LAYOUTS = 60_iso

With the new matrix/layout, the DZ60 can now support community 60% ISO layouts.

New keymap iso_uk

I've added a sample keymap based on ISO-UK, that uses the new matrix, to assist users who may desire this layout on their DZ60s. A readme file is also included.

Keymap refactor

All the existing keymaps have been refactored to use the QMK_KEYBOARD_H include. In addition, the iso_vim_arrow keymap has been refactored to use the new LAYOUT_60_iso_5x1u matrix, after confirming that it uses the same physical layout submitted in #3198.

Possible "Gotcha"s

Due to the way the community LAYOUT_60_iso and LAYOUT_60_iso_5x1u matrix/layouts are configured, in LAYOUT_60_iso, the Enter key appears as the last key on the home row:

KC_P,    KC_LBRC, KC_RBRC,
KC_SCLN, KC_QUOT, KC_NUHS, KC_ENT,

whereas in LAYOUT_60_iso_5x1u, the Enter key is at the end of Row 2:

KC_P,    KC_LBRC, KC_RBRC, KC_ENT,
KC_SCLN, KC_QUOT, KC_NUHS,

It would be nice, long-term, to have some sort of agreement about which way should be "standard" for QMK.


Tagging:


/* ISO VIM arrow (ISO German layout shown)
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the renaming, because it is more generic and reusable. File and this description still mention the word "vim arrow" which is a more specific example. This is intended, because it's an example or should it be changed, too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps I should change ISO VIM arrow on line 7 to ISO 5x1u.

I'm a bit unsure of which way I prefer. The code block you've pointed out shows the physical layout of the keymap, whose name is unchanged. I question whether vim arrow or 5x1u is better, because to my mind, neither has name recognition. It's not like saying "Tsangan" or "Winkeyless", terms that are widespread in the community.

Something that I think is less than ideal is that the only way to view the physical layouts of the matrices are to view them via the Configurator — you can't view them directly in the repo unless someone documents them, and currently there's no standard for doing so.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would add more detail to the description and things would be clearer. E.g. changing the name to 5x1u, but the description and layout is describing a variant which is supposed to have arrow keys in a single line and can be used to navigate similar to the vim editor.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like that idea. I think I'll go with that.

@jankolkmeier
Copy link
Contributor

About the row of the ISO enter key: I put it above the home row b/c that's how it is in KLE.
PS: Sorry for the broken JSON.

@drashna
Copy link
Member

drashna commented Jun 20, 2018

I know you're going to hate me for suggesting this, but maybe it would be a good idea to start including the layout info in the readme (such as images, or ASCII art), so that people have something to reference (both from source and in the configurator, since it shows the readme.md)

@noroadsleft
Copy link
Member Author

@jankolkmeier Not a problem.

It's actually one of the things that's not straightforward about building the info.json files, because people "imagine" the keymaps differently in their heads. As an American, I'm an ANSI user, but I strongly believe that both ANSI and ISO should be fully supported where possible. When I consider the differences between the two, I always imagine there being an extra key between KC_QUOT and KC_ENT, with the KC_BSLS key being deleted from the second row and not replaced.

Some keyboard PCBs also wire them differently. Sometimes ISO Enter shares its matrix location with ANSI Enter, and sometimes it shares with the ANSI Backslash.

Neither way is right or wrong on either topic, but it can get confusing.

@noroadsleft
Copy link
Member Author

@drashna I considered the ASCII art last night, but I'm not sure I'd like to go that route, even though it's a good solution.

Right now the DZ60 has nine supported layouts, and this PR would add one more. I'm wondering if there's a point where a readme file can be bloated, or have "too much" information therein.

@drashna
Copy link
Member

drashna commented Jun 20, 2018

@noroadsleft Doesn't @skullydazed have that nice template thing with all the different layouts in one image? That ... could work.

Otherwise, yeah, it could be a PITA.

@noroadsleft
Copy link
Member Author

@drashna If he does, I haven't seen it. The only thing I know about that's relevant is this image for the Clueboard, which is linked in the docs, but it's for displaying keymap rather than physical layout.

I considered adding KLE permalinks to the readme files, which makes them easily viewable if seen on GitHub, but aren't exposed in the Configurator in cases where a layout may be broken in the Configurator for some reason.

@drashna
Copy link
Member

drashna commented Jun 20, 2018

He's posted it in discord, I think.

But let me know when this is done

@@ -4,7 +4,11 @@

const uint16_t PROGMEM keymaps[][MATRIX_ROWS][MATRIX_COLS] = {

/* ISO VIM arrow (ISO German layout shown)
/* ISO 5x1u layout (ISO German keyboard layout shown)
Copy link
Member Author

@noroadsleft noroadsleft Jun 20, 2018

Choose a reason for hiding this comment

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

@andys8 How's this? (Lines 7 through 11)

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the changes a lot!

@drashna
Copy link
Member

drashna commented Jun 21, 2018

Awesome, thanks!

@drashna drashna merged commit f5109c9 into qmk:master Jun 21, 2018
@noroadsleft noroadsleft deleted the c10r-dz60 branch June 21, 2018 18:31
alexey-danilov pushed a commit to alexey-danilov/qmk_firmware that referenced this pull request Jul 27, 2018
* Bugfix refactor

* Added 60_iso layout to rules.mk

* Added sample ISO-UK keymap

* Keymap refactor

* Fixes per @andys8
yamad pushed a commit to yamad/qmk_firmware that referenced this pull request Apr 10, 2019
* Bugfix refactor

* Added 60_iso layout to rules.mk

* Added sample ISO-UK keymap

* Keymap refactor

* Fixes per @andys8
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