search

atom / spell-check

Spell check Atom package
MIT License
204 stars 121 forks source link

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

Closed dmoonfire closed 4 years ago

dmoonfire commented 5 years ago

Requirements

While investigating https://github.com/atom/atom/issues/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 https://github.com/atom/node-spellchecker/pull/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. (https://github.com/atom/atom/issues/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

dmoonfire commented 4 years ago

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 https://github.com/atom/atom/issues/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 commented 4 years ago

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 commented 4 years ago

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:

With just system:

With just locales:

With neither checked:

lee-dohm commented 4 years ago

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

lkashef commented 4 years ago

Thanks @dmoonfire 🙇‍♂️

lkashef commented 4 years ago

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

https://github.com/atom/atom/pull/20400

dmoonfire commented 4 years ago

@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 commented 4 years ago

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

ashthespy commented 4 years ago

@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 commented 4 years ago

same issue here as @ashthespy