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

Split checkers to explictly handle macOS/Windows system checkers verses locale-based checking #314

Merged
merged 1 commit into from Jan 24, 2020

Conversation

dmoonfire
Copy link
Contributor

@dmoonfire dmoonfire commented Aug 29, 2019

Requirements

While investigating atom/atom#15912, the problem seemed to be based on calling the macOS built-in checking library with different locales. With multiple languages, that would call it with first one and then the second. Eventually, there would be an error thrown that caused the system to crash.

Then, while working on that item, we identified a problem where certain Windows 10 installations (in my case, an enterprise-distributed one) don't always provide spell-checking for non-English dictionaries and therefore couldn't use the built-in language for non-English (or whatever) languages.

In both of these cases, we need to be able to use system checker in a generic manner (without specifying language) or force the use of Hunspell even without the environment variable.

Description of the Change

So, to fix that, I got atom/node-spellchecker#115 merged that exposes an explicit flag to choose system or Hunspell-based checking while setting up the library.

Now, I need to change the UI to split built-in system checking verses locale which involves some UI changes and verification since now there are more options to control how spell-checking is done.

The three environments also have different rules:

  1. Linux does not provide system checking, so it is ignored.
  2. macOS and Windows can't take languages for the system checking.
  3. All three need to handle specified locale checking.

Alternate Designs

Originally this was going to just be a macOS fix however the validation with Windows 10 that caused the problem identified a more generic problem.

I also considered splitting out the spell-check checkers out into spell-check-windows, spell-check-macos, spell-check-hunspell but I didn't get a definitive opinion either way. This option is still possible, it would just require automatically packaging 1-3 of these with the normal Atom installation.

Benefits

  1. It won't crash on macOS. (atom/atom#15912)
  2. It should handle the Windows installations that can't check other languages.

Possible Drawbacks

  1. Increased complexity to the settings.
  2. Possibly slower performance if Hunspell and system are enabled.

Applicable Issues

- The locale version uses the locale paths but can be used without an
environment variable.
  - This is the same logic for Linux and when the environment variable
was used (which no longer is used).
  - Defaults to on.
- The system version only uses the operating system based checker and
always ignores Hunspell, even with the environment variable.
  - This does nothing on Linux but can be set.
  - Defaults to on.
- Refactored tests to be easier to clean.
- Added system-based checking tests but are only active on OS X because
of inconsistent availability on Windows.
@dmoonfire
Copy link
Contributor Author

@dmoonfire dmoonfire commented Jan 1, 2020

This splits checking into system and locale based ones as shown in the screenshot below.

image

Due to my access to platforms, I'm going to need help with verifying this code on various forms of Windows and OS X, with single and different languages at the same time. Specific care needs to be looked at for atom/atom#15912 which causes a crash when using multiple languages (this is handled by having only a single "system" checker regardless of the number of views open).

@calebmeyer
Copy link

@calebmeyer calebmeyer commented Jan 2, 2020

macOS

Works as expected with all combinations of check boxes except only locales. Note that each screenshot is after a Window: Reload
image
image

No checkboxes, no spelling errors (I believe this is the expected result)
image
image

Different behavior with only locales checked

image

Note that `Aussi` (French for Also) is spelled correctly here:

image

@calebmeyer
Copy link

@calebmeyer calebmeyer commented Jan 6, 2020

I can't get good screenshots for windows right now, but I ran the same checks. As you mentioned on slack, I'd need to install a custom dictionary for French. Windows version is Windows 10 Enterprise 1903

With both checked:

  • orange error about not having the French dictionary
  • no spelling errors underlined

With just system:

  • same as both

With just locales:

  • spelling errors all in English (so all French words are underlined as misspelled)

With neither checked:

  • no errors underlined, as expected.

@dmoonfire dmoonfire changed the title [WIP] Split checkers to explictly handle macOS/Windows system checkers verses locale-based checking Split checkers to explictly handle macOS/Windows system checkers verses locale-based checking Jan 15, 2020
@lee-dohm
Copy link
Contributor

@lee-dohm lee-dohm commented Jan 21, 2020

Thanks for this @dmoonfire! @lkashef is going to take a look 👍

@lkashef
Copy link
Contributor

@lkashef lkashef commented Jan 24, 2020

Thanks @dmoonfire 🙇‍♂️

@lkashef lkashef merged commit 17e8862 into atom:master Jan 24, 2020
2 checks passed
@lkashef
Copy link
Contributor

@lkashef lkashef commented Feb 11, 2020

Hey @dmoonfire, did you try running the tests on Atom using your PR, because the build keeps failing on the CI.

atom/atom#20400

@dmoonfire
Copy link
Contributor Author

@dmoonfire dmoonfire commented Feb 11, 2020

@lkashef. I had to use the pull checks on atom/spell-check to get it working since I don't have access to Windows or Mac machines. They were all running green, but I haven't seen this error before: https://github.visualstudio.com/Atom/_build/results?buildId=59300&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=b525c197-f769-5e52-d38a-e6301f5260f2&l=1396

Cannot replace with lazy function because the supplied node does not belong to an assignment expression or a variable declaration!

I don't normally build Atom from source, just the plugins. This error looks like it is pointing to the require.resolve which is based on the plugin system which I wrote earlier. Is there a chance there is a new rule or process that is transforming the file that would choke on the dynamic requires?

https://blog.inkdrop.info/understanding-atom-editor-and-hacking-it-to-build-a-react-app-4692f049795 is where I got my information about it being the resolve call.

@dmoonfire dmoonfire deleted the split-checkers branch Feb 11, 2020
@dmoonfire
Copy link
Contributor Author

@dmoonfire dmoonfire commented Feb 11, 2020

So far, I'm not entirely sure what the problem is to fix it. I'll try to look into it.

@atom atom deleted a comment from 9994444ggg Feb 12, 2020
@atom atom deleted a comment from 9994444ggg Feb 12, 2020
@atom atom deleted a comment from 9994444ggg Feb 12, 2020
Alhadis added a commit to Alhadis/.atom that referenced this issue May 6, 2020
No idea what atom/spell-check#314 did, but the package is now griping at
startup about missing dictionaries, even though it's working fine (words
like "colour" are recognised, and "color" is marked as a misspelling).
@ashthespy
Copy link

@ashthespy ashthespy commented May 6, 2020

@calebmeyer Did you manage to get spell-check working on a non en-US language on Windows? I can't seem to make it work, even though the dictionaries are there..

@nupfel
Copy link

@nupfel nupfel commented May 7, 2020

same issue here as @ashthespy

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.

None yet

6 participants