Fix issues with Windows and Mac checking, reduce noise

dmoonfire commented 4 years ago

Description of the Change

The recent changes to split system and locale checkers have caused some trouble with Windows Spell Checking and spurious warnings on Mac. This is to resolve them as described in https://github.com/atom/spell-check/issues/328. In specific:

Windows with the default settings of "Use System" and "Use Locale" does not work with the default language.
Mac with the default settings shows a warning though system checking is working correctly.

Many of these issues also show up when the default locale is not en-US.

Alternate Designs

The main difficulty is coordinating the various rules that have to be applied:

Mac cannot define locales or paths for system checking to avoid process crashes.
Windows must define locales but cannot provide a path to use Spelling API.
Linux must use locales. System checking does nothing.
All can define a locale and path to use Hunspell.

The resulting fix was basically to smooth over some of the issues with these various conditions.

Benefits

This will benefit most users who want spelling to work in these conditions and not see warnings without unchecking "Use Locales".

I also noticed that there was some confusion on how to set up dictionaries and attempted to clarify the instructions to include more details in the README.md.

Possible Drawbacks

There are a lot of situations where one thing works and another doesn't. Someone may expect to see a warning that won't, but folks should not see more warnings.

Also, Dylan is unable to test on Windows or Mac, so extra care or verification would be greatly appreciated on those platforms.

Applicable Issues

https://github.com/atom/spell-check/issues/328

dmoonfire commented 4 years ago

As usual, please squash on merge. This commit graph is messy.

ashthespy commented 4 years ago

Tested on Win10 with en-GB as default language.

By default Use System and Use Locales are ticked, which lead to no spell check:

spell-check:system-checker enabled +0ms true working correctly
spell-check:locale-checker:en-GB enabled +0ms true hasSystemChecker true inferredLocale true
spell-check:locale-checker:en-GB checking paths +33ms ["C:\"]
spell-check:locale-checker:en-GB using Windows Spell API +21ms
spell-check:system-checker check +1s do you check fro spellings? []

Checking only Use Locales works for my en-GB locale:

spell-check:locale-checker:en-GB enabled +0ms true hasSystemChecker false inferredLocale true
spell-check:locale-checker:en-GB checking paths +80ms ["C:\"]
spell-check:locale-checker:en-GB using Windows Spell API +41ms
atom.config.get('spell-check')
>{grammars: Array(14), useSystem: false, excludedScopes: Array(0), useLocales: true, locales: Array(0), …}

Adding a second system dictionary de-DE also works, but have to add the default system dictionary as well (which is probably by design)

spell-check:locale-checker:en-GB enabled +0ms true hasSystemChecker false inferredLocale false
spell-check:locale-checker:de-DE enabled +0ms true hasSystemChecker false inferredLocale false
spell-check:locale-checker:en-GB checking paths +77ms 
["C:\"]
spell-check:locale-checker:en-GB using Windows Spell API +42ms
spell-check:locale-checker:de-DE checking paths +52ms 
["C:\"]
spell-check:locale-checker:de-DE using Windows Spell API +16ms
atom.config.get('spell-check')
> {grammars: Array(14), locales: Array(2), useSystem: false, excludedScopes: Array(0), useLocales: true, …}
addKnownWords: false
excludedScopes: []
knownWords: []
localePaths: []
locales: (2) ["en-GB", "de-DE"]
noticesMode: "both"
useLocales: true
useSystem: false

A nitpick: I would expect that having only Use System ticked to work in my case, and not only Use Locales - especially given the text description of 'Use Locales' talking about Hunspell..

EDIT: Just saw your https://github.com/atom/spell-check/issues/328#issuecomment-628222551 and to cross post:

On Mac and Windows, you should have it start up and do spelling if you have "Use System" and "Use Locales" checked and nothing in the "Locales" field.

This doesn't happen as noted above - I guess because with Use System checked, a new SpellCheker instance is created that has no dictionary set, that takes precedence over the instance created in locale-checker with the right default dictionary set for Windows?

dmoonfire commented 4 years ago

@ashthespy, let's see why that is happening. I did end up switching branches to https://github.com/dmoonfire/spell-check/tree/locale-issues-tests so the code is there (I wrote a test to prove I broke it before merging the locale-issues branch into it).

I wasn't logging the output from the individual locales. Would you be willing to grab the latest and try again. The entire plugin system is supposed to allow system and locales to coexist (in your case, 1 system + 2 locale are being used in your example). In this case, system will not return anything, but the locales should.

ashthespy commented 4 years ago

@dmoonfire with your latest logging tweaks we get more insight! This is with both Use System and Use Locales

PS: the old tests were on the PR branch..

dmoonfire commented 4 years ago

@ashthespy Well, the locale checking is working correctly judging from the "(2) {..}" line. I just pushed up another commit that adds logging on the next layer out to see why it isn't coordinating the changes between the two.

dmoonfire commented 4 years ago

I think I found the current problem, I'm trying to figure out why this isn't working on a Windows machine.

ashthespy commented 4 years ago

@dmoonfire Not sure how the intersection logic works, but it doesn't seem to work right in this case. I added some more logging:

@@ -180,8 +180,10 @@ class SpellCheckerManager
       # Remove all of the confirmed correct words from the resulting incorrect
       # list. This allows us to have correct-only providers as opposed to only
       # incorrect providers.
-      if correct.ranges.length > 0
-        intersection.subtract(correct)
+      @log 'intersection_before_correctiong', intersection
+      @log 'correct', correct.ranges
+      # if correct.ranges.length > 0
+      #   intersection.subtract(correct)

Which gives:

Lol. And you can also see how much I depend on spellcheck. :-D /correctiong/correction/

dmoonfire commented 4 years ago

Yeah, I'm getting strange behavior that doesn't make sense to me. That's what I'm tracking down. I'm populating the range inside the loop, but when I go to push it, it's pushing up an empty list. :( But, I have a temporary Windows machine so I'm tracing through the code there to look at it.

dmoonfire commented 4 years ago

Okay, the problem. When I split the checking into system and locale, system checking was enabled for both Windows and Macs (as it was previously) but it was also tied into looping through locale code (but wasn't after the split).

System checking produces a check result that sets invertIncorrectAsCorrect as true. What this means is that it says "everything I don't say is incorrect is correct". This is different than "I'm only saying this is wrong but have no opinions on the rest." (invertIncorrectAsCorrect = false). We need this because it doesn't say "here is what right" which is ultimately what we are looking for.

However, with system check not producing good results in this case, it is saying nothing is incorrect, which means everything is correct because of invertIncorrectAsCorrect. This isn't true in this case, because the dictionary isn't actually working.

Now, the Mac is supposed to handle "" passed in as a locale, so I can do a test check (setDictionary) to see if the dictionary is working with the API and use that. The Travis thing isn't picking up the item for some reason, but if that doesn't work, I'll switch the env to say don't use system checking (which is primarily for Mac anyways).

dmoonfire commented 4 years ago

@ashthespy, okay, now it should work. :)

ashthespy commented 4 years ago

@dmoonfire Can confirm the latest fixes work for me with both Use System and Use Locales ticked. Only Use System however doesn't work for the default language. Given that, it might be prudent to update the Use Locales text. Right now the "If checked, then the locales below will be used to check words using Hunspell" is misleading, as it should be checked to use the default dictionary case with Windows as well?

lkashef commented 4 years ago

Thanks @ashthespy for testing this out and thanks @dmoonfire for the great support 🙇‍♂️

9994444ggg commented 4 years ago

Thank You all

Erraach-walid commented 3 years ago

atom / spell-check