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

Fallback to the dictionary path provided by node-spellchecker. #233

Merged
merged 1 commit into from Jan 3, 2018

Conversation

Projects
None yet
4 participants
@dmoonfire
Contributor

dmoonfire commented Dec 30, 2017

Requirements

Windows 7 doesn't correct use the packaged en-US dictionary when no settings are included in the dictionary.

Description of the Change

On Windows 7 (#162), spell-check does not correctly fall back to en-US dictionary inside the node-spellcheck because we ship a ASAR-packed archive. The fallback directory search was based on the original version of node-spellcheck's check for local director but did not include the "unpacked" logic that is there today. In version 3.4.4 of node-spellcheck, a method was provided to get the library's search path which we are now using instead of duplicating the (incorrect) logic that doesn't handle ASAR packing.

Alternate Designs

Originally, we copied the search path from node-spellcheck. An alternative is to update our search path logic to use the same try/catch logic to handle archives. This seemed more fragile and harder to maintain.

Benefits

It works on Windows 7.

node-spellcheck will always agree with spell-check in guessing the vendor path.

Possible Drawbacks

The path that was previously used may be different than the one node-spellcheck provides.

Applicable Issues

I have spent a year trying to get Windows 7 to package on a local machine and have failed. I can't fully test with a ASAR-packaged version of Atom.

@@ -74,8 +74,7 @@ class SystemChecker
# Try the packaged library inside the node_modules. `getDictionaryPath` is

This comment has been minimized.

@50Wliu

50Wliu Jan 3, 2018

Member

This comment doesn't look correct anymore now that it's been changed to use spellchecker.getDictionaryPath() 😄.

@50Wliu

50Wliu Jan 3, 2018

Member

This comment doesn't look correct anymore now that it's been changed to use spellchecker.getDictionaryPath() 😄.

@nathansobo nathansobo self-assigned this Jan 3, 2018

@nathansobo nathansobo merged commit 0f39b0a into atom:master Jan 3, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Jan 3, 2018

Contributor

Thanks so much @dmoonfire! You are our spellcheck savior.

Contributor

nathansobo commented Jan 3, 2018

Thanks so much @dmoonfire! You are our spellcheck savior.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Jan 3, 2018

Contributor

@ungb I went ahead and merged this on @dmoonfire's word since he has such a good track record, but it would be good if you could test the 1.23.3 build that includes it just to be sure. It needed to be bundled in the ASAR archive anyway, which means it was easier to just merge it.

Contributor

nathansobo commented Jan 3, 2018

@ungb I went ahead and merged this on @dmoonfire's word since he has such a good track record, but it would be good if you could test the 1.23.3 build that includes it just to be sure. It needed to be bundled in the ASAR archive anyway, which means it was easier to just merge it.

@dmoonfire dmoonfire deleted the dmoonfire:use-spellchecker-path branch Jan 4, 2018

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Jan 4, 2018

sounds good! thanks!

ungb commented Jan 4, 2018

sounds good! thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment