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

[kle2info] Trim the code in kle2xy #8955

Merged
merged 2 commits into from
May 15, 2020
Merged

Conversation

kbrock
Copy link
Contributor

@kbrock kbrock commented Apr 28, 2020

This is not aimed to change functionality. Just trimming the code a little to hopefully be more understandable

Description

2 commits:

  • use min, max
  • remove current_x, current_y

Use Min, Max

the label_style can be between 0 and 9.

If a value is too high, it is set to 9.
The old code would put the correct value in current_key, but would put the incorrect value in skel (i.e. default values for the current key).

Simplifying the bounds checking to a single expression allowes both skel and current_key to be set with the correct value.

Same goes for font_size.

remove current_{x,y}

row and column are the location of a key in u units. u being the size of a key.
x and y are the location of the center of the key in mm.

y = current_row * key_size + key_height * key_size /2
you can have this value either by adding key_size every row, or by calculating this on the fly.

It seemed simpler to me to not have (and maintain) the 2 extra variables

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

I ran before and after with the test kle.txt and the larger alice keyboard layout.
The output was the same.

Since info does not output the x, y in mm, I also ran alice keyboard with print statements of the row, col, x, y values both before and after to ensure the calculated values were the same as the maintained values.

Sorry, something went wrong.

Copy link
Member

@skullydazed skullydazed left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements! Can you also submit a PR upstream? https://github.com/skullydazed/kle2xy

lib/python/kle2xy.py Outdated Show resolved Hide resolved
@kbrock
Copy link
Contributor Author

kbrock commented Apr 28, 2020

@skullydazed I updated this PR and put in another across the way.
Let me know if you want me to change anything else.

@drashna drashna requested review from Erovia, noroadsleft and a team April 29, 2020 09:29
kbrock added 2 commits April 30, 2020 01:58
This is a slight change.
Before, the key_skel would keep the invalid value for future keys.
I think this is what was actually intended.
x is the current_x * key_size + (key_size/2)
y is the current_y * key_size + (key_size/2)

no reason to track both
@kbrock kbrock force-pushed the kle2_info_4_trim branch from 2c233b5 to 39bc28b Compare April 30, 2020 05:58
@kbrock
Copy link
Contributor Author

kbrock commented Apr 30, 2020

removed x,y from skel + rebased
thnx

@zvecr zvecr added cli qmk cli command python labels May 1, 2020
@skullydazed skullydazed requested a review from a team May 9, 2020 18:51
@skullydazed skullydazed merged commit c3aaed8 into qmk:master May 15, 2020
bitherder pushed a commit to bitherder/qmk_firmware that referenced this pull request May 15, 2020
* [kle2jinfo] use min/max instead of if

This is a slight change.
Before, the key_skel would keep the invalid value for future keys.
I think this is what was actually intended.

* [kle2info] calculate x

x is the current_x * key_size + (key_size/2)
y is the current_y * key_size + (key_size/2)

no reason to track both
@kbrock kbrock deleted the kle2_info_4_trim branch May 20, 2020 18:25
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request May 24, 2020
* [kle2jinfo] use min/max instead of if

This is a slight change.
Before, the key_skel would keep the invalid value for future keys.
I think this is what was actually intended.

* [kle2info] calculate x

x is the current_x * key_size + (key_size/2)
y is the current_y * key_size + (key_size/2)

no reason to track both
sowbug pushed a commit to sowbug/qmk_firmware that referenced this pull request May 24, 2020
* [kle2jinfo] use min/max instead of if

This is a slight change.
Before, the key_skel would keep the invalid value for future keys.
I think this is what was actually intended.

* [kle2info] calculate x

x is the current_x * key_size + (key_size/2)
y is the current_y * key_size + (key_size/2)

no reason to track both
fdidron pushed a commit to zsa/qmk_firmware that referenced this pull request Jun 12, 2020
* [kle2jinfo] use min/max instead of if

This is a slight change.
Before, the key_skel would keep the invalid value for future keys.
I think this is what was actually intended.

* [kle2info] calculate x

x is the current_x * key_size + (key_size/2)
y is the current_y * key_size + (key_size/2)

no reason to track both
turky pushed a commit to turky/qmk_firmware that referenced this pull request Jun 13, 2020
* [kle2jinfo] use min/max instead of if

This is a slight change.
Before, the key_skel would keep the invalid value for future keys.
I think this is what was actually intended.

* [kle2info] calculate x

x is the current_x * key_size + (key_size/2)
y is the current_y * key_size + (key_size/2)

no reason to track both
jakobaa pushed a commit to jakobaa/qmk_firmware that referenced this pull request Jul 7, 2020
* [kle2jinfo] use min/max instead of if

This is a slight change.
Before, the key_skel would keep the invalid value for future keys.
I think this is what was actually intended.

* [kle2info] calculate x

x is the current_x * key_size + (key_size/2)
y is the current_y * key_size + (key_size/2)

no reason to track both
sjmacneil pushed a commit to sjmacneil/qmk_firmware that referenced this pull request Feb 19, 2021
* [kle2jinfo] use min/max instead of if

This is a slight change.
Before, the key_skel would keep the invalid value for future keys.
I think this is what was actually intended.

* [kle2info] calculate x

x is the current_x * key_size + (key_size/2)
y is the current_y * key_size + (key_size/2)

no reason to track both
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
* [kle2jinfo] use min/max instead of if

This is a slight change.
Before, the key_skel would keep the invalid value for future keys.
I think this is what was actually intended.

* [kle2info] calculate x

x is the current_x * key_size + (key_size/2)
y is the current_y * key_size + (key_size/2)

no reason to track both
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli qmk cli command python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants