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

Added german_gaming keymap for hidtech/bastyl #11446

Merged
merged 3 commits into from
Feb 10, 2021
Merged

Added german_gaming keymap for hidtech/bastyl #11446

merged 3 commits into from
Feb 10, 2021

Conversation

CreamyCookie
Copy link

Description

Added a german gaming keymap for @HID-Technologies' Bastyl.

MOUSEKEY_ENABLE was set to no for this keymap, since I'm not using it, and IIRC the size of the firmware ended up being too large.

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

@github-actions github-actions bot added the keymap label Jan 5, 2021
@drashna drashna requested a review from a team January 30, 2021 02:58
keyboards/hidtech/bastyl/keymaps/german_gaming/rules.mk Outdated Show resolved Hide resolved
@@ -0,0 +1,24 @@
![German Gaming Layout Image](https://i.imgur.com/0y938rG.png)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
![German Gaming Layout Image](https://i.imgur.com/0y938rG.png)
![German Gaming Layout Image](https://i.imgur.com/0y938rGh.png)

Copy link
Author

@CreamyCookie CreamyCookie Jan 30, 2021

Choose a reason for hiding this comment

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

All the other changes make sense to me, but this seems unnecessary and counterproductive.

The file size is only 283 KiB and screen resolutions are constantly increasing. Halving the size makes this image and the text fuzzy and harder to read.

Am I missing something, and if not, is this change necessary?

Copy link
Author

Choose a reason for hiding this comment

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

@fauxpark Thanks again for the review! 👍

Is this a blocking change request?

Copy link
Member

Choose a reason for hiding this comment

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

The file size is only 283 KiB and screen resolutions are constantly increasing. Halving the size makes this image and the text fuzzy and harder to read.

Am I missing something, and if not, is this change necessary?

I don't know if fauxpark considers this a blocking issue, but GitHub's not going to display this image at anything larger than about 1,000 pixels wide anywhere that a user would see it, no matter how large the source image is.

Copy link
Author

@CreamyCookie CreamyCookie Feb 8, 2021

Choose a reason for hiding this comment

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

@noroadsleft That does not seem to be true. Check out the way it is rendered here on a HiDPI screen:

https://github.com/qmk/qmk_firmware/blob/ecbef0f66440674cca7dac8ed6e3e940be12b6bb/keyboards/hidtech/bastyl/keymaps/german_gaming/readme.md

or

https://github.com/CreamyCookie/qmk_firmware/blob/german_gaming_keymap_for_bastyl/keyboards/hidtech/bastyl/keymaps/german_gaming/readme.md

Screenshot created using Firefox's screenshot tool (on a 1440p screen):

hidpi

Now here's the fuzzy half-size image again ( https://i.imgur.com/0y938rGh.png ):

0y938rGh

Codec: Also important to note is that the half-size image is not just a half-size PNG, it's actually a lossy JPEG, which is the reason for the washed out look.

Even if it was true that GitHub currently downsized all images or limited the size via the layout, there's no reason to believe that this will still be the case in a couple years, especially when screen resolutions like 4K are getting really common. But, of course, 2339 x 1654 is not even that large.

I want to be clear. It's not a big issue. I'm happy for this to be merged either way, but I also believe that the change is actually counter productive for any potential user and thus also for the QMK community. (Of course, nobody might ever use the keymap, but who knows.)

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I guess this is hardware dependent; I wasn't aware GitHub did anything differently with page layout/rendering in that regard.

I personally don't find the smaller image to be so much more fuzzy as to be unreadable, but I'm undoubtedly not the best person to ask as I have 20/20 vision.

Also good point about the fact that GitHub may change this rendering in future as higher-resolution display become more common (I'm on a 24" 1080p screen).

keyboards/hidtech/bastyl/keymaps/german_gaming/keymap.c Outdated Show resolved Hide resolved
CreamyCookie and others added 2 commits January 30, 2021 23:58
Co-authored-by: Ryan <fauxpark@gmail.com>
@drashna drashna requested a review from a team February 4, 2021 17:40
@noroadsleft noroadsleft merged commit 4b2ab84 into qmk:master Feb 10, 2021
@noroadsleft
Copy link
Member

Thanks!

@CreamyCookie
Copy link
Author

@noroadsleft Thank you for merging and being open minded about the image thing! 👍

@CreamyCookie CreamyCookie deleted the german_gaming_keymap_for_bastyl branch February 11, 2021 15:12
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