Merge pull request #332 from dmoonfire/locale-issues-tests

Fix issues with Windows and Mac checking, reduce noise

Author: lkashef

DEVELOP.md

@@ -0,0 +1,15 @@
+Here are additional details for debugging for working with the library.
+
+## Plugins
+
+_Spell Check_ allows for plugins to provide additional spell checking functionality. See the `PLUGINS.md` file in the repository on how to write a plugin.
+
+## Debugging
+
+Debugging messages for this library can be enabled by going into the developer console and running the following:
+
+```
+localStorage.debug = 'spell-check:*'
+```
+
+A reload of the window may be required.

README.md

@@ -20,15 +20,25 @@ To enable _Spell Check_ for your current file type: put your cursor in the file,
 
 ## Changing the dictionary
 
-To change the language of the dictionary, set the "Locales" configuration option to the IETF tag (en-US, fr-FR, etc). More than one language can be used, simply separate them by commas.
+Except for Mac, Atom needs to know what language to use to perform spell-checking. To list these, set the "Locales" configuration option to the IETF tag (en-US, fr-FR, etc). More than one language can be used, simply separate them by commas.
+
+If no locale is given, then Atom will attempt to infer the language based on environment variables and settings.
+
+If any value is given for the "Locales", then Atom will not automatically add the browser language. So, if your browser is United States English (`en-US`), leaving this blank will still do US English checking. However, if it the "Locales" is set to French (`fr-FR`), then the checker will only check French. If the "Locales" is set to `en-US, fr-FR`, then both languages will be checked.
 
 ### Missing Languages
 
 This plugin uses the existing system dictionaries. If a locale is selected that is not installed, a warning will pop up when a document that would be spell-checked is loaded. To disable this, either remove the incorrect language from the "Locales" configuration or clear the check on "Use Locales" to disable it entirely.
 
 To get the search paths used to look for a dictionary, make sure the "Notices Mode" is set to "console" or "both", then reload Atom. The developer's console will have the directory list.
 
-#### Windows 8 and Higher
+## Mac
+
+On the Mac, checking "Use System" will use the operating system's spellchecking library. This uses all of the user's loaded dictionaries and doesn't require any customization within Atom.
+
+Checking "Use Locales" and providing locales would use Hunspell as additional dictionaries. Having "Use Locales" checked but no locales given will do nothing.
+
+## Windows 8 and Higher
 
 For Windows 8 and 10, this package uses the Windows spell checker, so you must install the language using the regional settings before the language can be chosen inside Atom.
 
@@ -40,11 +50,13 @@ If your Windows user does not have Administration privileges, you'll need to do
 
 ![Download the "Basic Typing" language option](docs/windows-10-language-settings-3.png)
 
-Once the additional language is added, Atom will need to be restarted.
+Once the additional language is added, Atom will need to be restarted and configured to use it. Add the IEFT tag into the "Locales" setting for the language to be set.
+
+If a Hunspell dictionary is found on a path (see below), it will be used in favor of the Windows API.
 
-*Previously, setting `SPELLCHECKER_PREFER_HUNSPELL` environment variable would change how checking works. Now this is controlled by the system and locale checker to use the operating system version or Hunspell respectively.*
+## Linux
 
-If locale is not set, Atom will attempt to use the current locale from the environment variable; if that is missing, `en-US` will be used. The dictionary for `en-US` is shipping with Atom but all other locale-based checkers will need to be downloaded from another source.
+For all Linux-based operating systems, "Use System" does nothing. It can remained checked but has no impact. "Use Locales" is required for spell-checking.
 
 ### Debian, Ubuntu, and Mint
 
@@ -86,6 +98,27 @@ sudo ln -s en_GB-large.dic en_GB.dic
 sudo ln -s en_GB-large.aff en_GB.aff
 ```
 
-## Plugins
+## Hunspell Dictionaries
+
+For all platforms, a Hunspell-compatible dictionary is also supported. To use this, a `.dic` and `.aff` need to be located in one of the default search directories or in a directory entered into "Locale paths" (multiples may be entered with commas separating them). If the appropriate files are found for the locale and "Use Locales" is checked, then the dictionary will be used.
+
+For example, if the following is set, then `/usr/share/hunspell/en_US.dic` will be used:
+
+- Use Locales: checked
+- Locales: `en-US`
+- Locale Paths: `/usr/share/hunspell`
+
+If "Locales" is not provided, then the user's current language will be inferred from environmental settings.
+
+In addition to what is provided, the following paths are checked:
+
+- `/usr/share/hunspell` (Linux only)
+- `/usr/share/myspell` (Linux only)
+- `/usr/share/myspell/dicts` (Linux only)
+- `/` (Mac only)
+- `/System/Library/Spelling` (Mac only)
+- `C:\` (Windows only)
+
+Dictionaries can be downloaded from various sites (such as [wooorm's repository](https://github.com/wooorm/dictionaries) or [LibreOffice's](https://github.com/LibreOffice/dictionaries)), but the file has to be renamed `locale.dic` and `locale.aff`.
 
-_Spell Check_ allows for plugins to provide additional spell checking functionality. See the `PLUGINS.md` file in the repository on how to write a plugin.
+*Example locations to download are not an endorsement.*

lib/locale-checker.coffee

@@ -1,6 +1,7 @@
 spellchecker = require 'spellchecker'
 pathspec = require 'atom-pathspec'
 env = require './checker-env'
+debug = require 'debug'
 
 # The locale checker is a checker that takes a locale string (`en-US`) and
 # optionally a path and then checks it.
@@ -10,12 +11,17 @@ class LocaleChecker
   enabled: true
   reason: null
   paths: null
+  checkDictionaryPath: true
+  checkDefaultPaths: true
 
-  constructor: (locale, paths) ->
+  constructor: (locale, paths, hasSystemChecker, inferredLocale) ->
     @locale = locale
     @paths = paths
     @enabled = true
-    #console.log @getId(), "enabled", @isEnabled()
+    @hasSystemChecker = hasSystemChecker
+    @inferredLocale = inferredLocale
+    @log = debug('spell-check:locale-checker').extend(locale)
+    @log 'enabled', @isEnabled(), 'hasSystemChecker', @hasSystemChecker, 'inferredLocale', @inferredLocale
 
   deactivate: ->
     return
@@ -33,7 +39,9 @@ class LocaleChecker
     @deferredInit()
     id = @getId()
     if @enabled
-      @spellchecker.checkSpellingAsync(text).then (incorrect) ->
+      @spellchecker.checkSpellingAsync(text).then (incorrect) =>
+        if @log.enabled
+          @log 'check', incorrect
         {id, invertIncorrectAsCorrect: true, incorrect}
     else
       {id, status: @getStatus()}
@@ -43,59 +51,76 @@ class LocaleChecker
     @spellchecker.getCorrectionsForMisspelling(word)
 
   deferredInit: ->
-    # The system checker is not enabled for Linux platforms (no built-in checker).
-    if not @enabled
-      @reason = "Darwin does not use locale-based checking without SPELLCHECKER_PREFER_HUNSPELL set."
-      return
-
     # If we already have a spellchecker, then we don't have to do anything.
     if @spellchecker
       return
 
     # Initialize the spell checker which can take some time. We also force
     # the use of the Hunspell library even on Mac OS X. The "system checker"
     # is the one that uses the built-in dictionaries from the operating system.
-    @spellchecker = new spellchecker.Spellchecker
-    @spellchecker.setSpellcheckerType spellchecker.ALWAYS_USE_HUNSPELL
+    checker = new spellchecker.Spellchecker
+    checker.setSpellcheckerType spellchecker.ALWAYS_USE_HUNSPELL
 
     # Build up a list of paths we are checking so we can report them fully
     # to the user if we fail.
     searchPaths = []
-
-    # Windows uses its own API and the paths are unimportant, only attempting
-    # to load it works.
-    if env.isWindows()
-      #if env.useWindowsSystemDictionary()
-      #  return
-      searchPaths.push "C:\\"
-
-    # Check the paths supplied by the user.
     for path in @paths
       searchPaths.push pathspec.getPath(path)
 
-    # For Linux, we have to search the directory paths to find the dictionary.
-    if env.isLinux()
-      searchPaths.push "/usr/share/hunspell"
-      searchPaths.push "/usr/share/myspell"
-      searchPaths.push "/usr/share/myspell/dicts"
+    # Add operating system specific paths to the search list.
+    if @checkDefaultPaths
+      if env.isLinux()
+        searchPaths.push "/usr/share/hunspell"
+        searchPaths.push "/usr/share/myspell"
+        searchPaths.push "/usr/share/myspell/dicts"
 
-    # OS X uses the following paths.
-    if env.isDarwin()
-      searchPaths.push "/"
-      searchPaths.push "/System/Library/Spelling"
+      if env.isDarwin()
+        searchPaths.push "/"
+        searchPaths.push "/System/Library/Spelling"
 
-    # Try the packaged library inside the node_modules. `getDictionaryPath` is
-    # not available, so we have to fake it. This will only work for en-US.
-    searchPaths.push spellchecker.getDictionaryPath()
+      if env.isWindows()
+        searchPaths.push "C:\\"
 
     # Attempt to load all the paths for the dictionary until we find one.
+    @log 'checking paths', searchPaths
     for path in searchPaths
-      if @spellchecker.setDictionary @locale, path
+      if checker.setDictionary @locale, path
+        @log 'found checker', path
+        @spellchecker = checker
+        return
+
+    # On Windows, if we can't find the dictionary using the paths, then we also
+    # try the spelling API. This uses system checker with the given locale, but
+    # doesn't provide a path. We do this at the end to let Hunspell be used if
+    # the user provides that.
+    if env.isWindows()
+      systemChecker = new spellchecker.Spellchecker
+      systemChecker.setSpellcheckerType spellchecker.ALWAYS_USE_SYSTEM
+      if systemChecker.setDictionary @locale, ""
+        @log 'using Windows Spell API'
+        @spellchecker = systemChecker
+        return
+
+    # If all else fails, try the packaged en-US dictionary in the `spellcheck`
+    # library.
+    if @checkDictionaryPath
+      if checker.setDictionary @locale, spellchecker.getDictionaryPath()
+        @log 'using packaged locale', path
+        @spellchecker = checker
         return
 
+    # If we are using the system checker and we infered the locale, then we
+    # don't want to show an error. This is because the system checker may have
+    # handled it already.
+    if @hasSystemChecker and @inferredLocale
+      @log 'giving up quietly because of system checker and inferred locale'
+      @enabled = false
+      @reason = "Cannot load the locale dictionary for `" + @locale + "`. No warning because system checker is in use and locale is inferred."
+      return
+
     # If we fell through all the if blocks, then we couldn't load the dictionary.
     @enabled = false
-    @reason = "Cannot load the system dictionary for `" + @locale + "`."
+    @reason = "Cannot load the locale dictionary for `" + @locale + "`."
     message = "The package `spell-check` cannot load the " \
       + "checker for `" \
       + @locale + "`." \

lib/main.coffee

@@ -1,4 +1,5 @@
 {CompositeDisposable} = require 'atom'
+debug = require 'debug'
 
 SpellCheckView = null
 spellCheckViews = {}
@@ -7,6 +8,9 @@ LARGE_FILE_SIZE = 2 * 1024 * 1024
 
 module.exports =
   activate: ->
+    log = debug('spell-check')
+    log('initializing')
+
     @subs = new CompositeDisposable
 
     # Since the spell-checking is done on another process, we gather up all the

lib/spell-check-manager.coffee

@@ -1,3 +1,6 @@
+debug = require 'debug'
+env = require './checker-env'
+
 class SpellCheckerManager
   checkers: []
   checkerPaths: []
@@ -35,20 +38,22 @@ class SpellCheckerManager
       @removeSpellChecker @knownWordsChecker
       @knownWordsChecker = null
 
-    # Handle system checker.
+    # Handle system checker. We also will remove the locale checkers if we
+    # change the system checker because we show different messages if we cannot
+    # find a locale based on the use of the system checker.
     removeSystemChecker = false
+    removeLocaleCheckers = false
 
     if @useSystem isnt data.useSystem
       @useSystem = data.useSystem
       removeSystemChecker = true
+      removeLocaleCheckers = true
 
     if removeSystemChecker and @systemChecker
       @removeSpellChecker @systemChecker
       @systemChecker = undefined
 
     # Handle locale checkers.
-    removeLocaleCheckers = false
-
     if not _.isEqual(@locales, data.locales)
       # If the locales is blank, then we always create a default one. However,
       # any new data.locales will remain blank.
@@ -119,6 +124,9 @@ class SpellCheckerManager
       promises.push Promise.resolve checker.check(args, text)
 
     Promise.all(promises).then (allResults) =>
+      if @log.enabled
+        @log "check results", allResults, text
+
       for results in allResults
         if results.invertIncorrectAsCorrect and results.incorrect
           # We need to add the opposite of the incorrect as correct elements in
@@ -145,7 +153,12 @@ class SpellCheckerManager
 
       # If we don't have any incorrect spellings, then there is nothing to worry
       # about, so just return and stop processing.
+      if @log.enabled
+        @log "merged correct ranges", correct
+        @log "merged incorrect ranges", incorrects
+
       if incorrects.length is 0
+        @log "no spelling errors"
         return {misspellings: []}
 
       # Build up an intersection of all the incorrect ranges. We only treat a word
@@ -165,6 +178,7 @@ class SpellCheckerManager
 
       # If we have no intersection, then nothing to report as a problem.
       if intersection.length is 0
+        @log "no spelling after intersections"
         return {misspellings: []}
 
       # Remove all of the confirmed correct words from the resulting incorrect
@@ -173,6 +187,9 @@ class SpellCheckerManager
       if correct.ranges.length > 0
         intersection.subtract(correct)
 
+      if @log.enabled
+        @log "check intersections", intersection
+
       # Convert the text ranges (index into the string) into Atom buffer
       # coordinates ( row and column).
       row = 0
@@ -316,7 +333,11 @@ class SpellCheckerManager
     ])
 
   init: ->
+    # Set up logging.
+    @log = debug "spell-check:spell-check-manager"
+
     # Set up the system checker.
+    hasSystemChecker = @useSystem and env.isSystemSupported()
     if @useSystem and @systemChecker is null
       SystemChecker = require './system-checker'
       @systemChecker = new SystemChecker
@@ -335,10 +356,14 @@ class SpellCheckerManager
       @localeCheckers = []
 
       # If we have a blank location, use the default based on the process. If
-      # set, then it will be the best language.
+      # set, then it will be the best language. We keep track if we are using
+      # the default locale to control error messages.
+      inferredLocale = false
+
       if not @locales.length
         defaultLocale = process.env.LANG
         if defaultLocale
+          inferredLocale = true
           @locales = [defaultLocale.split('.')[0]]
 
       # If we can't figure out the language from the process, check the
@@ -351,18 +376,20 @@ class SpellCheckerManager
         if defaultLocale and defaultLocale.length is 5
           separatorChar = defaultLocale.charAt(2)
           if separatorChar is '_' or separatorChar is '-'
+            inferredLocale = true
             @locales = [defaultLocale]
 
       # If we still can't figure it out, use US English. It isn't a great
       # choice, but it is a reasonable default not to mention is can be used
       # with the fallback path of the `spellchecker` package.
       if not @locales.length
+        inferredLocale = true
         @locales = ['en_US']
 
       # Go through the new list and create new locale checkers.
       LocaleChecker = require "./locale-checker"
       for locale in @locales
-        checker = new LocaleChecker locale, @localePaths
+        checker = new LocaleChecker locale, @localePaths, hasSystemChecker, inferredLocale
         @addSpellChecker checker
         @localeCheckers.push checker