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

Encoder support for KeyboardModel and some classes #734

Merged
merged 2 commits into from
Sep 12, 2022

Conversation

yoichiro
Copy link
Collaborator

@yoichiro yoichiro commented Sep 10, 2022

Related #201

This pull request adds a new feature to support encoders for KeyboardModel and some other classes. For instance, if some keys have an encoder ID in the VIA JSON file, the KeyboardModel object provides whether the key has an encoder ID or not.

@yoichiro yoichiro force-pushed the encoder-support-for-keyboard-model branch from f9cda89 to 4d7e3d0 Compare September 10, 2022 07:49
@yoichiro yoichiro force-pushed the encoder-support-for-keyboard-model branch from 4d7e3d0 to 2cd1026 Compare September 10, 2022 10:11
@yoichiro
Copy link
Collaborator Author

@adamrocker Could you review this pull request? I would like to confirm whether this approach looks good to your idea or not.

@@ -101,6 +102,13 @@ class KeymapItem {
this.option = options[0];
this.choice = options[1];
this._toBeDelete = false;
// Check whether this key is an encoder or not
const positions = label.split('\n');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you might integrate this code to line#100 which is the splitter of contents in a label.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@adamrocker I refactored the logic, and I removed the pos instance variable because it is not used. Could you review them again? The diff is 8e714d7.

@adamrocker
Copy link
Collaborator

Your approach looks perfect. I agree with this.

@adamrocker
Copy link
Collaborator

LGTM, thanks!

@yoichiro yoichiro merged commit 5e5262b into main Sep 12, 2022
@yoichiro yoichiro deleted the encoder-support-for-keyboard-model branch September 12, 2022 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants