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

[settings-view] Fix Package keymap compatibility check #1161

Merged

Conversation

confused-Techie
Copy link
Member

It turns out that for quite some time the way the package's keymap section checks for platform compatibility has been broken.

I've detailed the reason why in #1160, but basically, if a keymap listed multiple platforms, such as linux and win32 the compatibility check would just see that it works on a platform other than the current one, and would think it's incompatible and not show it to the user.

So I've gone ahead and fixed this check much in the same way we already check in settings-view/lib/keybindings-panel.js except I've expanded out on this functionality a bit, by creating a method for this check.

Also, I've given the ability to pass in a platform rather than going with the current one, which makes it much easier to test this, and means if we wanted to in the future we could purposefully grab incompatible keymaps.

And since it's not testable, of course I've written new tests to be able to test this behavior.

Resolves #1160

This PR also makes this compatibility check testable, and even expandable, by being able to provide a new platform if preferred.
Copy link
Contributor

@savetheclocktower savetheclocktower left a comment

Choose a reason for hiding this comment

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

I haven't tried to manually verify this — the xml-formatter package that triggered this fix always did show keymaps on my platform — but the code seems solid and the unit tests are appreciated.

Assuming that this has been verified to fix the original issue, 👍 .

@confused-Techie
Copy link
Member Author

Thanks for the review!
It hasn't been manually tested, as I was having difficulty testing getting errors from Windows about the app being incompatible.

But I'll go ahead and download the generated bins to give it a quick manual test

@confused-Techie
Copy link
Member Author

Alright, I can confirm that this does in fact solve the issue for xml-formatter and others like it.
Merging now!

@confused-Techie confused-Techie merged commit 7ab0460 into master Dec 16, 2024
101 of 103 checks passed
@confused-Techie confused-Techie deleted the settings-view/keymap-platform-compatibility-check branch December 16, 2024 01:18
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.

Pulsar does not show keybindings table for xml-formatter package
2 participants