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

Convert vuex/keycodes store to pinia #1370

Merged
merged 10 commits into from
Nov 7, 2024
Merged

Convert vuex/keycodes store to pinia #1370

merged 10 commits into from
Nov 7, 2024

Conversation

yanfali
Copy link
Collaborator

@yanfali yanfali commented Nov 5, 2024

Description

This PR replaces all the uses of vuex keycodes store with a pinia equivalent. It also fixes a bug in the steno code I discovered while refactoring, where steno boards were not being identified correctly.

This should be 100% equivalent with no regressions.

Vuex is in deep maintenance mode and pinia is the recommended library for vue. This work is preparation for moving to vue 3 and vue 2 is now EOL.

@yanfali yanfali marked this pull request as ready for review November 5, 2024 22:45
@yanfali
Copy link
Collaborator Author

yanfali commented Nov 5, 2024

@precondition if you get a chance, a quick review or running it locally to spot if I broke anything would be appreciated. Thanks

@yanfali yanfali changed the title more pinia updates Convert vuex/keycodes store to pinia Nov 5, 2024
@@ -1,12 +1,14 @@
import Vue from 'vue';
import random from 'lodash/random';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

remove a lot of dependencies on lodash, as a lot of this stuff is built in now

@yanfali
Copy link
Collaborator Author

yanfali commented Nov 7, 2024

I added some jsdoc for some of the hairy parts, and documented some of the internal types used inside the keycode JSON.

@yanfali yanfali merged commit 76009c0 into master Nov 7, 2024
4 checks passed
@yanfali yanfali deleted the pinia-2 branch November 7, 2024 20:37
@precondition
Copy link
Contributor

Oh shoot, I planned to take a look at this later today.

@yanfali
Copy link
Collaborator Author

yanfali commented Nov 8, 2024

Oh shoot, I planned to take a look at this later today.

No worries. Any feedback you have is appreciated. Please check I didn't break anything on the deployed code. Thank you!

Copy link
Contributor

@precondition precondition left a comment

Choose a reason for hiding this comment

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

I pointed out a few typos but I did not notice any regressions. However, during testing, I did uncover a pre-existing bug in the match count feature.

}

/**
* Used to dynamically generate the tab data for they keycode
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Used to dynamically generate the tab data for they keycode
* Used to dynamically generate the tab data for the keycode

*
* @param {string} osKeyboardLayout
* @param {boolean} isSteno
* @returns {Array.<KeycodeDefinition|KeycodeLabel|WidthPlaceholder}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @returns {Array.<KeycodeDefinition|KeycodeLabel|WidthPlaceholder}
* @returns {Array.<KeycodeDefinition|KeycodeLabel|WidthPlaceholder>}

Comment on lines +160 to +171
setSearchFilter(newVal) {
this.searchFilter = newVal;
if (this.searchFilter !== '') {
this.searchCounters = {
ANSI: countMatches(this.searchFilter, ansi),
'ISO/JIS': countMatches(this.searchFilter, iso_jis),
Quantum: countMatches(this.searchFilter, quantum),
KeyboardSettings: countMatches(this.searchFilter, settings),
AppMediaMouse: countMatches(this.searchFilter, media)
};
}
}
Copy link
Contributor

@precondition precondition Nov 8, 2024

Choose a reason for hiding this comment

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

This is a pre-existing issue but the collections passed to countMatches (ansi, iso_jis, etc.) are read-only variables imported from ./modules/keycodes/###. Consequently, these keycode collections are not localized at all causing incorrect match counts to be reported.

For example, if I search for "ß" with the German OS layout, the ß key will be highlighted but the count in both ANSI and ISO tabs will be "(0)". Same story if I search for "Y" with the German OS layout; DE_Y and KC_Y (=DE_Z) are both highlighted but the ANSI tab will only report a single match because it successfully finds a "Y" in the uppercase version of KC_Y's name but KC_Z aka DE_Y has "Z" as its name in the ansi variable so the countMatches does not count it.

The solution would be to map the toLocalKeycode on each collection before passing it to the countMatches. I'll open a separate issue and open a PR for a possible implementation.

EDIT: See issue #1372

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch.

Comment on lines +185 to +194
* @typedef {Object} WidthPlaceholder - used for spacing
* @property {number} width - width in Key Units * 1000. e.g. 1U = 1000, 2U = 2000.
*/

/**
* @typedef {Object} KeycodeDefinition - metadata about a keycode
* @property {string} name - UI display label for the keycode
* @property {string} code - QMK keycode defintion
* @property {string} [keys] - javascript keypress id. Used by keyboard handler
* @property {number} [width] - width in Key Units * 1000. e.g. 1U = 1000, 2U = 2000.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @typedef {Object} WidthPlaceholder - used for spacing
* @property {number} width - width in Key Units * 1000. e.g. 1U = 1000, 2U = 2000.
*/
/**
* @typedef {Object} KeycodeDefinition - metadata about a keycode
* @property {string} name - UI display label for the keycode
* @property {string} code - QMK keycode defintion
* @property {string} [keys] - javascript keypress id. Used by keyboard handler
* @property {number} [width] - width in Key Units * 1000. e.g. 1U = 1000, 2U = 2000.
* @typedef {Object} WidthPlaceholder - used for spacing
* @property {number} width - width in Key Units * 1000. e.g. 1U = 1000, 2U = 2000
*/
/**
* @typedef {Object} KeycodeDefinition - metadata about a keycode
* @property {string} name - UI display label for the keycode
* @property {string} code - QMK keycode definition
* @property {string} [keys] - javascript keypress id. Used by keyboard handler
* @property {number} [width] - width in Key Units * 1000. e.g. 1U = 1000, 2U = 2000

Typo + inconsistent use of trailing period

/**
* @typedef {Object} KeycodeStoreState
* @property {Array.<KeycodeDefinition|KeycodeLabel|WidthPlaceholder>} keycodes - active keycodes
* @property {string} searchFilter - currently search filter
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @property {string} searchFilter - currently search filter
* @property {string} searchFilter - current query in the keycode picker search bar

this.commit('keycodes/updateKeycodeNames');
const keycodesStore = useKeycodesStore();

// Important to call keycodes/updatekeycodeNames *before* keymap/updateKeycodeNames.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Important to call keycodes/updatekeycodeNames *before* keymap/updateKeycodeNames.
// Important to call keycodesStore.updateKeycodeNames *before* keymap/updateKeycodeNames.

Updating the comment to reflect changes to the call signature

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