Re: ispell.el, flyspell.el: better ispell/aspell switching

[Agustin Martin <agustin.martin@hispalinux.es>]

On Tue, Apr 15, 2008 at 02:40:23PM -0400, Stefan Monnier wrote:
> >>>>> "Agustin" == Agustin Martin <agustin.martin@hispalinux.es> writes:
> 
> > Hi,
> 
> > I come back with a rewritten approach for a problem in current ispell.el and
> > flyspell.el when switching spellchecker in an emacs session.
> 
> > The problem is as follows, when in an emacs run aspell is used for the first
> > time, ispell-dictionary-alist is filled with the aspell values, and if
> > ispell-program-name is customized or changed to ispell during that emacs run
> > it inherits the aspell values, thus failing if there was an aspell entry
> > with the same name. Since ispell is still (rarely) needed for pseudo-encodings
> > like [\'a -> á] I think this should not happen and all ispell values should
> > be restored in such case. Also current behavior is too ispell/aspell
> > centric, in case support for another spellchecker is ever added.
> 
> > In the proposed attached patches (ispell-maybe-find-aspell-dictionaries) is
> > replaced by (more neutral) new (ispell-set-spellchecker-params) function.
> > That function will check if spellchecker is changed and fill
> > ``ispell-dictionary-alist'' with the appropriate values. As written it has
> > support for info provided by distros. Patches are mostly as currently used
> > in Debian, with some comments rewritten, and a naive check for [:alpha:]
> > used (instead of just discarding that for xemacs as in Debian). Keeping
> > those xemacs checks saves me a couple of patches, please leave them there if
> > possible.
> 
> This is a good change, but there are a few minor issues with it:
> 
> - ispell-spellchecker-init-pre-hook is not documented.  What are
>   distro-override-dicts-alist and distro-fallback-dicts-alist?

The hook is intended for easier interaction of distro maintainers wrt the
dictionary alist. E.g., in Debian ispell and aspell dictionary packages
provide info about the dictionary that might or not be in
original ispell-dictionary-alist. That hook is intended to allow easy
modification of both distro-override-dicts-alist and
distro-fallback-dicts-alist. These have the same format as
``ispell-dictionary-alist'' and are (very poorly) documented when
defined in the 'let', and allow two levels of overriding,

``distro-override-dicts-alist'' will override even dicts in
``found-dicts-alist'' (currently the alist of parsed aspell dicts and
associated properties if spellchecker is aspell). Put here just in case is
ever needed, but I am currently not using it in Debian.

``distro-fallback-dicts-alist'' will just override (or complement if
originally not present) initial ``ispell-dictionary-alist'' values (those
that are in ``base-dicts'' in the function), so an entry can be fixed without
patching ispell.el or waiting for a new emacs version be released.

For instance, this is the relevant part in debian-ispell.el

---------------------------
(defun debian-ispell-initialize-dicts-alist ()
  (setq distro-fallback-dicts-alist
        (if ispell-really-aspell
            debian-aspell-only-dictionary-alist
          debian-ispell-only-dictionary-alist)))

(add-hook 'ispell-spellchecker-init-pre-hook 'debian-ispell-initialize-dicts-alist)
--------------------------

``debian-aspell-only-dictionary-alist'' and ``debian-ispell-only-dictionary-alist''
being built after the info provided by the package maintainers.

Should I document the hook just in the function, or there is a better place?

> - (setq ispell-last-program-name ispell-program-name) should be done
>   right after checking if they're equal, not at the end of the function.

I usually do this kind of things after the work is done, so is only changed
if nothing wrong happened.

> - (defvar ispell-aspell-dictionary-alist...) should be before the first
>   use of that variable.

I just put it in the aspell stuff section, but I agree that is better as you
propose, before (ispell-set-spellchecker-params). I should probably move the
ispell-set-spellchecker-params stuff below the aspell stuff, so each has an
own block.

> BTW (this is unrelated to this patch, but I just noticed it while
> looking at the code): why are ispell-dictionary-alist* autoloaded?
> I just tried to remove the autoloads on them and I couldn't notice any
> negative effect.

As you pointed out, presumably because the way menus were previously built
required it. 

While we are at this, since ``ispell-dictionary-alist'' is in my patch built
from its components in (ispell-set-spellchecker-params),  its original
definition may be simplified to just a 

(defvar ispell-dictionary-alist nil
  " ..... The long description of ispell-dictionary-list ...
follows
...."

probably adding a coment like

;; Its actual value will be set in the (ispell-set-spellchecker-params)
;; function

-- 
Agustin