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

Fix issues with Windows and Mac checking, reduce noise #332

Merged
merged 15 commits into from May 18, 2020

Conversation

dmoonfire
Copy link
Contributor

@dmoonfire dmoonfire commented May 14, 2020

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 #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

- Added `debug` for tracking down issues. Use `localStorage.debug =
'spell-check:*'` to enable.
@dmoonfire dmoonfire linked an issue May 14, 2020 that may be closed by this pull request
@dmoonfire dmoonfire changed the title WIP - Fix issues with Windows and Mac checking, reduce noise Fix issues with Windows and Mac checking, reduce noise May 14, 2020
@dmoonfire
Copy link
Contributor Author

@dmoonfire dmoonfire commented May 14, 2020

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

lib/system-checker.coffee Outdated Show resolved Hide resolved
@ashthespy
Copy link

@ashthespy ashthespy commented May 14, 2020

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 #328 (comment) 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
Copy link
Contributor Author

@dmoonfire dmoonfire commented May 14, 2020

@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
Copy link

@ashthespy ashthespy commented May 14, 2020

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

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

@dmoonfire
Copy link
Contributor Author

@dmoonfire dmoonfire commented May 14, 2020

@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
Copy link
Contributor Author

@dmoonfire dmoonfire commented May 14, 2020

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

@ashthespy
Copy link

@ashthespy ashthespy commented May 14, 2020

@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:

image

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

@dmoonfire
Copy link
Contributor Author

@dmoonfire dmoonfire commented May 14, 2020

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
Copy link
Contributor Author

@dmoonfire dmoonfire commented May 14, 2020

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
Copy link
Contributor Author

@dmoonfire dmoonfire commented May 14, 2020

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

@ashthespy
Copy link

@ashthespy ashthespy commented May 15, 2020

@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?

package.json Show resolved Hide resolved
@lkashef lkashef merged commit 31296a4 into atom:master May 18, 2020
2 checks passed
@lkashef
Copy link
Contributor

@lkashef lkashef commented May 18, 2020

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

@dmoonfire dmoonfire deleted the locale-issues-tests branch May 18, 2020
@lkashef lkashef removed the triaged label May 27, 2020
@9994444ggg
Copy link

@9994444ggg 9994444ggg commented Jul 6, 2020

Thank You all

@Erraach-walid
Copy link

@Erraach-walid Erraach-walid commented Nov 25, 2020


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.

6 participants