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

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

[Agustin Martin]

[-- Attachment #1: Type: text/plain, Size: 2043 bytes --]

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.

ispell.el:
==========
(ispell-set-spellchecker-params): 
  New function to make sure right params and dictionary alists are used
  after spellchecker changes.

``ispell-aspell-dictionary-alist'', ``ispell-last-program-name'':
  New variables: used by (ispell-set-spellchecker-params)

(ispell-find-aspell-dictionaries)
  Modified to use ``ispell-aspell-dictionary-alist''

(ispell-maybe-find-aspell-dictionaries)
  Removed. Calls replaced by (ispell-set-spellchecker-params) calls.

(ispell-have-aspell-dictionaries)
  Removed.

flyspell.el:
============
(ispell-maybe-find-aspell-dictionaries)
  Calls replaced by (ispell-set-spellchecker-params) calls.

-- 
Agustin

[-- Attachment #2: ispell.el_ispell-set-spellchecker-params.diff --]
[-- Type: text/x-diff, Size: 5750 bytes --]

--- ispell.el.orig	2008-03-28 12:55:25.000000000 +0100
+++ ispell.el	2008-04-04 13:33:11.000000000 +0200
@@ -879,20 +879,63 @@
 				   )
   "Non-nil means that the OS supports asynchronous processes.")
 
-;; Make ispell.el work better with aspell.
+;; Set params according to the selected spellchecker
 
-(defvar ispell-have-aspell-dictionaries nil
-  "Non-nil if we have queried Aspell for dictionaries at least once.")
+(defvar ispell-last-program-name nil
+  "Last value of ispell-program name. Internal use.")
 
-(defun ispell-maybe-find-aspell-dictionaries ()
-  "Find Aspell's dictionaries, unless already done."
-  (when (and (not ispell-have-aspell-dictionaries)
-	     (condition-case ()
-		 (progn (ispell-check-version) t)
+(defun ispell-set-spellchecker-params ()
+  "Initialize some spellchecker params when it is changed."
+  (unless (eq ispell-last-program-name ispell-program-name)
+    (ispell-kill-ispell t)
+    (if (and (condition-case ()
+		 (progn
+		   (setq ispell-library-directory (ispell-check-version))
+		   t)
 	       (error nil))
 	     ispell-really-aspell
-	     ispell-aspell-supports-utf8)
-    (ispell-find-aspell-dictionaries)))
+	     ispell-aspell-supports-utf8
+	     ;; xemacs does not like [:alpha:] regexps
+	     (string-match "^[[:alpha:]]+$" "abcde"))
+	(unless ispell-aspell-dictionary-alist
+	  (ispell-find-aspell-dictionaries)))
+
+    ;; Substitute ispell-dictionary-alist with the list of dictionaries
+    ;; corresponding to the given spellchecker. If a recent aspell, use
+    ;; the list of really installed dictionaries and add to it elements
+    ;; of the original list that are not present there. Allow distro info.
+    (let ((base-dicts-alist
+	   (append ispell-dictionary-alist-1 ispell-dictionary-alist-2
+		   ispell-dictionary-alist-3 ispell-dictionary-alist-4
+		   ispell-dictionary-alist-5 ispell-dictionary-alist-6))
+	  (found-dicts-alist
+	   (if (and ispell-really-aspell
+		    ispell-aspell-supports-utf8)
+	       ispell-aspell-dictionary-alist
+	     nil))
+	  distro-override-dicts-alist ; Override even found-dicts-alist
+	  distro-fallback-dicts-alist ; Override only base-dicts-alist
+	  all-dicts-alist)
+
+      ;; If distro has more info, consider it if needed. Mostly for possible
+      ;; ``distro-override-dicts-alist'' and ``distro-fallback-dicts-alist''.
+      (run-hooks 'ispell-spellchecker-init-pre-hook)
+
+      ;; Add dicts to ``ispell-dictionary-alist'' unless already present.
+      (setq all-dicts-alist distro-override-dicts-alist)
+      (dolist (dict (append found-dicts-alist
+			    distro-fallback-dicts-alist
+			    base-dicts-alist))
+	(unless (assoc (car dict) all-dicts-alist)
+	  (add-to-list 'all-dicts-alist dict)))
+      (setq ispell-dictionary-alist all-dicts-alist))
+    (setq ispell-last-program-name ispell-program-name)))
+
+;; Make ispell.el work better with aspell.
+
+(defvar ispell-aspell-dictionary-alist nil
+  "An alist of parsed aspell dicts and associated parameters.
+Internal use.")
 
 (defun ispell-find-aspell-dictionaries ()
   "Find Aspell's dictionaries, and record in `ispell-dictionary-alist'."
@@ -915,14 +958,13 @@
     (dolist (dict ispell-dictionary-alist)
       (unless (assoc (car dict) found)
 	(setq found (nconc found (list dict)))))
-    (setq ispell-dictionary-alist found)
+    (setq ispell-aspell-dictionary-alist found)
     ;; Add a default entry
     (let* ((english-dict (assoc "en" ispell-dictionary-alist))
 	   (default-dict
 	     (cons nil (or (cdr english-dict)
 			   (cdr (car ispell-dictionary-alist-1))))))
-      (push default-dict ispell-dictionary-alist))
-    (setq ispell-have-aspell-dictionaries t)))
+      (push default-dict ispell-aspell-dictionary-alist))))
 
 (defvar ispell-aspell-data-dir nil
   "Data directory of Aspell.")
@@ -1007,8 +1049,12 @@
 (defun ispell-valid-dictionary-list ()
   "Returns a list of valid dictionaries.
 The variable `ispell-library-directory' defines the library location."
-  ;; If Ispell is really Aspell, query it for the dictionary list.
-  (ispell-maybe-find-aspell-dictionaries)
+  ;; Initialize variables and dictionaries alists for desired spellchecker.
+  ;; Make sure ispell.el is loaded to avoid some autoload loops in xemacs
+  ;; (and may be others)
+  (if (featurep 'ispell)
+      (ispell-set-spellchecker-params))
+
   (let ((dicts (append ispell-local-dictionary-alist ispell-dictionary-alist))
 	(dict-list (cons "default" nil))
 	name load-dict)
@@ -1566,7 +1612,7 @@
     (ispell-region (region-beginning) (region-end)))
    (continue (ispell-continue))
    (t
-    (ispell-maybe-find-aspell-dictionaries)
+    (ispell-set-spellchecker-params)    ; Initialize variables and dicts alists
     (ispell-accept-buffer-local-defs)	; use the correct dictionary
     (let ((cursor-location (point))	; retain cursor location
 	  (word (ispell-get-word following))
@@ -2602,7 +2648,7 @@
 	       (mapcar 'list (ispell-valid-dictionary-list)))
 	  nil t)
 	 current-prefix-arg))
-  (ispell-maybe-find-aspell-dictionaries)
+  (ispell-set-spellchecker-params) ; Initilize variables and dicts alists
   (unless arg (ispell-buffer-local-dict 'no-reload))
   (if (equal dict "default") (setq dict nil))
   ;; This relies on completing-read's bug of returning "" for no match
@@ -2653,7 +2699,7 @@
 Return nil if spell session is quit,
  otherwise returns shift offset amount for last line processed."
   (interactive "r")			; Don't flag errors on read-only bufs.
-  (ispell-maybe-find-aspell-dictionaries)
+  (ispell-set-spellchecker-params)      ; Initialize variables and dicts alists
   (if (not recheckp)
       (ispell-accept-buffer-local-defs)) ; set up dictionary, local words, etc.
   (let ((skip-region-start (make-marker))

[-- Attachment #3: flyspell.el_ispell-set-spellchecker-params.diff --]
[-- Type: text/x-diff, Size: 1536 bytes --]

--- flyspell.el.orig	2008-03-28 12:55:25.000000000 +0100
+++ flyspell.el	2008-03-28 12:58:02.000000000 +0100
@@ -578,7 +578,7 @@
 ;;*---------------------------------------------------------------------*/
 (defun flyspell-mode-on ()
   "Turn Flyspell mode on.  Do not use this; use `flyspell-mode' instead."
-  (ispell-maybe-find-aspell-dictionaries)
+  (ispell-set-spellchecker-params) ; Initialize variables and dicts alists
   (setq ispell-highlight-face 'flyspell-incorrect)
   ;; local dictionaries setup
   (or ispell-local-dictionary ispell-dictionary
@@ -1016,6 +1016,7 @@
 (defun flyspell-word (&optional following)
   "Spell check a word."
   (interactive (list ispell-following-word))
+  (ispell-set-spellchecker-params)    ; Initialize variables and dicts alists
   (save-excursion
     ;; use the correct dictionary
     (flyspell-accept-buffer-local-defs)
@@ -1531,7 +1532,7 @@
     ;; this is done, we can start checking...
     (if flyspell-issue-message-flag (message "Checking region..."))
     (set-buffer curbuf)
-    (ispell-check-version)
+    (ispell-set-spellchecker-params)  ; Initialize variables and dicts alists
     ;; Local dictionary becomes the global dictionary in use.
     (setq ispell-current-dictionary
 	  (or ispell-local-dictionary ispell-dictionary))
@@ -1590,6 +1591,7 @@
 (defun flyspell-region (beg end)
   "Flyspell text between BEG and END."
   (interactive "r")
+  (ispell-set-spellchecker-params)  ; Initialize variables and dicts alists
   (if (= beg end)
       ()
     (save-excursion

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

[Agustin Martin]

On Fri, Apr 04, 2008 at 02:02:17PM +0200, Agustin Martin wrote:
> 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 ...

Any comment?

-- 
Agustin

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

[Lennart Borgman (gmail)]

Agustin Martin wrote:
> On Fri, Apr 04, 2008 at 02:02:17PM +0200, Agustin Martin wrote:
>> 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 ...
> 
> Any comment?

I have not looked at your patch, but your reasoning above seems ok to me.

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

[Stefan Monnier]

>>>>> "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?
- (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.
- (defvar ispell-aspell-dictionary-alist...) should be before the first
  use of that variable.

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.


        Stefan

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

[Jason Rumney]

Stefan Monnier wrote:
> 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.
>   

Could it be because people tend to append or prepend to them in .emacs, 
rather than setting them outright?

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

[Stefan Monnier]

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

> Could it be because people tend to append or prepend to them in .emacs,
> rather than setting them outright?

I don't think so: there's ispell-local-dictionary-alist for that.

But I now suspect it is done for hysterical raisins: the "Tools => Spell
checking" menu used to list the available dictionaries (even before
ispell.el was loaded).

Now that we have removed those menu entries, I guess it doesn't need to
be preloaded.


        Stefan

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

[Agustin Martin]

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

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

[Stefan Monnier]

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

In what kind of circumstance would this be needed?

> ``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.

Can't this be done by:
1 - remove autoloads on ispell-dictionary-alist-N
2 - merge ispell-dictionary-alist-N into ispell-base-dictionary-alist
3 - let Debian use after-eval-hook to adjust ispell-base-dictionary-alist

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

Why does Debian need to do that?  I.e. What kind of changes does
this effect?  Why doesn't ispell.el get it right on its own?

> (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.

I must say that I generally prefer if there aren't any hooks and when we
don't use dynamic scoping, so I'm not thrilled about this code.
I understand that there might be a need for some hook, but I'd rather it
doesn't use dynamic scoping.  Could you get away with a normal hook run
at the end of ispell-set-spellchecker-params, which would work by
modifying ispell-dictionary-alist?

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

Add a defvar for the hook, where you can document it.

>> - (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.

In case of an error, I'd rather avoid doing it
over-and-over-and-over-and-over ... also it's good to have it all in
a single place.  But I won't insist.

>> - (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.

Either way is fine by me.

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

Yes, I reached the exact same conclusion.


        Stefan

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

[Agustin Martin]

On Wed, Apr 16, 2008 at 11:21:33AM -0400, Stefan Monnier wrote:
> 
> > ``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.
> 
> In what kind of circumstance would this be needed?

I put this there just for completeness, but I do not think of a particular
case where this is actually needed. The only thing I can think of is to
work around a bad aspell autodetection for a dict, but in this case is the
autodetection what needs a fix. Not a problem to remove it.
 
> > ``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.
> 
> Can't this be done by:
> 1 - remove autoloads on ispell-dictionary-alist-N
> 2 - merge ispell-dictionary-alist-N into ispell-base-dictionary-alist
> 3 - let Debian use after-eval-hook to adjust ispell-base-dictionary-alist

(I guess you mean after-load-hook instead of after-eval-hook) Note that 
this will fail if spellchecker is switched after ispell.el is loaded. If so,
ispell-base-dictionary-alist will be left with the old values, that might
not match what the new available dicts (or new needs) for spellchecker.
 
> > 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)))
> 
> Why does Debian need to do that?  I.e. What kind of changes does
> this effect?  Why doesn't ispell.el get it right on its own?

Note that we use the same FSF ispell.el and flyspell.el (with a number of
compatibility patches and some known limitations) for emacs21, emacs22 and
xemacs, so dictionaries are integrated in a similar way for all emacs
flavours. Those are in a package different from the {x}emacs* that get
installed along with the ispell or aspell dictionaries.

The current way of doing things is that dictionary maintainers provide info
that is used to build a dictionary-alist for the really installed dicts
after the provided info. That makes debian-aspell-only-dictionary-alist for
the aspell dicts and debian-ispell-only-dictionary-alist for the ispell
dicts, and that is the info passed to the hook as
distro-fallback-dicts-alist.

One advantage of this is that there is no need to patch ispell.el each time
a new dictionary package is added to Debian or some info is fixed, and the
right dictionary-alists are automagically rebuilt every time a dictionary
package is installed in the user box. Note that for emacs and aspell this
would not be a big problem because of the autodetection, but is a problem
for emacs and ispell as well as for xemacs.

The other advantage is that we do not try to have ispell.el entries for
every language having a dict, so original alist is smaller.

> > (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.
> 
> I must say that I generally prefer if there aren't any hooks and when we
> don't use dynamic scoping, so I'm not thrilled about this code.
> I understand that there might be a need for some hook, but I'd rather it
> doesn't use dynamic scoping.  Could you get away with a normal hook run
> at the end of ispell-set-spellchecker-params, which would work by
> modifying ispell-dictionary-alist?

Disclaimer: Although I do some elisp hacking, I am far from being an elisp
expert, so do not be surprised if I need some reading before understanding
some details, or even finally do not fully understand them. Thanks in
advance for your patience.

I think the important point is at which time is this extra distro info to be
processed if present (or of course if minimal support for this possible
extra info belongs to pristine ispell.el or is something expected from
distros). I think this is better done as currently, just in the middle, that
is, after parsed aspell dicts, but before base-dicts. You do not know where
things come from if things are done at the end, and if you need to check it
you may end reusing half of the function.

If the problem is about using a hook there, something like

(when (fboundp 'ispell-distro-may-modify-dictionary-alist)
  (ispell-distro-may-modify-dictionary-alist))

will also do the work, although may look a bit ugly. If the problem is
both hooks and dynamic scoping, something like

(let ...
  (distro-fallback-dicts-alist (when (fboundp 'ispell-may-get-distro-fallback-dicts)
                                  (ispell-may-get-distro-fallback-dicts)))

in (ispell-set-spellchecker-params), where
(ispell-may-get-distro-fallback-dicts), if present, returns the alist,
may work.

Of course, is also possible to consider that this kind of distro things should
not go in pristine ispell.el.

> > Should I document the hook just in the function, or there is a better place?
> 
> Add a defvar for the hook, where you can document it.

Thanks

> >> - (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.
> 
> In case of an error, I'd rather avoid doing it
> over-and-over-and-over-and-over ... also it's good to have it all in
> a single place.  But I won't insist.

Is not bad to insist. As a matter of fact you have convinced me in this
case. I usually prefer the other way, so errors are evident and noisy, but
here most error-possible things are condition-case protected, so there would
be no noise, just extra computing work for no good reason.

Thanks a lot for your feedback,

Regards,

-- 
Agustin

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

[Stefan Monnier]

>> > ``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.
>> 
>> In what kind of circumstance would this be needed?

> I put this there just for completeness, but I do not think of a particular
> case where this is actually needed. The only thing I can think of is to
> work around a bad aspell autodetection for a dict, but in this case is the
> autodetection what needs a fix. Not a problem to remove it.
 
>> > ``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.
>> 
>> Can't this be done by:
>> 1 - remove autoloads on ispell-dictionary-alist-N
>> 2 - merge ispell-dictionary-alist-N into ispell-base-dictionary-alist
>> 3 - let Debian use after-eval-hook to adjust ispell-base-dictionary-alist

> (I guess you mean after-load-hook instead of after-eval-hook) Note that 
> this will fail if spellchecker is switched after ispell.el is loaded. If so,
> ispell-base-dictionary-alist will be left with the old values, that might
> not match what the new available dicts (or new needs) for spellchecker.

Oh, because you want to modify a ispel--independent list but you want to
modify it in a ispell-dependent way.  Yes, I see it doesn't cut it.
 
>> > 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)))
>> 
>> Why does Debian need to do that?  I.e. What kind of changes does
>> this effect?  Why doesn't ispell.el get it right on its own?

> Note that we use the same FSF ispell.el and flyspell.el (with a number of
> compatibility patches and some known limitations) for emacs21, emacs22 and
> xemacs, so dictionaries are integrated in a similar way for all emacs
> flavours. Those are in a package different from the {x}emacs* that get
> installed along with the ispell or aspell dictionaries.

> The current way of doing things is that dictionary maintainers provide info
> that is used to build a dictionary-alist for the really installed dicts
> after the provided info. That makes debian-aspell-only-dictionary-alist for
> the aspell dicts and debian-ispell-only-dictionary-alist for the ispell
> dicts, and that is the info passed to the hook as
> distro-fallback-dicts-alist.

> One advantage of this is that there is no need to patch ispell.el each time
> a new dictionary package is added to Debian or some info is fixed, and the
> right dictionary-alists are automagically rebuilt every time a dictionary
> package is installed in the user box. Note that for emacs and aspell this
> would not be a big problem because of the autodetection, but is a problem
> for emacs and ispell as well as for xemacs.

> The other advantage is that we do not try to have ispell.el entries for
> every language having a dict, so original alist is smaller.

>> > (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.
>> 
>> I must say that I generally prefer if there aren't any hooks and when we
>> don't use dynamic scoping, so I'm not thrilled about this code.
>> I understand that there might be a need for some hook, but I'd rather it
>> doesn't use dynamic scoping.  Could you get away with a normal hook run
>> at the end of ispell-set-spellchecker-params, which would work by
>> modifying ispell-dictionary-alist?

> Disclaimer: Although I do some elisp hacking, I am far from being an elisp
> expert, so do not be surprised if I need some reading before understanding
> some details, or even finally do not fully understand them. Thanks in
> advance for your patience.

> I think the important point is at which time is this extra distro info to be
> processed if present (or of course if minimal support for this possible
> extra info belongs to pristine ispell.el or is something expected from
> distros).

I think it's perfectly fine to include it in ispell.el (it doesn't have
to be limited to the use of distro's tho, so I'd rather not use the word
"distro".  As a general rule, variables and functions should take names
that describe what they do rather than in what context they'll be used)

> I think this is better done as currently, just in the middle, that
> is, after parsed aspell dicts, but before base-dicts. You do not know where
> things come from if things are done at the end, and if you need to check it
> you may end reusing half of the function.

Is it ever necessary/important to distinguish whether it comes from the
base or fom the parsed dicts?

> If the problem is about using a hook there, something like

No, the problem is the dynamically scoped variables.  But if there's no
easy way to do without them, it's OK to use such things.  Just use more
descriptive names, and be sure to document them in the hook's docstring.


        Stefan

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

[Agustin Martin]

[-- Attachment #1: Type: text/plain, Size: 2144 bytes --]

On Thu, Apr 17, 2008 at 09:38:51PM -0400, Stefan Monnier wrote:

> I think it's perfectly fine to include it in ispell.el (it doesn't have
> to be limited to the use of distro's tho, so I'd rather not use the word
> "distro".  As a general rule, variables and functions should take names
> that describe what they do rather than in what context they'll be used)

Thanks, I now notice that this may also be used systemwide by sysadmins
for e.g., non-standard dicts, so removing the 'distro' word completely and
using ``ispell-initialize-spellchecker-hook'' instead.

> 
> > I think this is better done as currently, just in the middle, that
> > is, after parsed aspell dicts, but before base-dicts. You do not know where
> > things come from if things are done at the end, and if you need to check it
> > you may end reusing half of the function.
> 
> Is it ever necessary/important to distinguish whether it comes from the
> base or fom the parsed dicts?

I think that only in the function.

> > If the problem is about using a hook there, something like
> 
> No, the problem is the dynamically scoped variables.  But if there's no
> easy way to do without them, it's OK to use such things.  Just use more
> descriptive names, and be sure to document them in the hook's docstring.

Thanks for all the suggestions, I am attaching updated patches. Feel free to
change if you think there are better names or ways of doing things.


ispell.el:
==========
(ispell-set-spellchecker-params): 
  New function to make sure right params and dictionary alists are used
  after spellchecker changes.

``ispell-aspell-dictionary-alist'', ``ispell-last-program-name'',
``ispell-initialize-spellchecker-hook''
New variables and hook: used by (ispell-set-spellchecker-params)

(ispell-find-aspell-dictionaries)
  Modified to use ``ispell-aspell-dictionary-alist''

(ispell-maybe-find-aspell-dictionaries)
  Removed. Calls replaced by (ispell-set-spellchecker-params) calls.

(ispell-have-aspell-dictionaries)
  Removed.

flyspell.el:
============
(ispell-maybe-find-aspell-dictionaries)
  Calls replaced by (ispell-set-spellchecker-params) calls.

-- 
Agustin

[-- Attachment #2: ispell.el_ispell-set-spellchecker-params.2.diff --]
[-- Type: text/x-diff, Size: 6033 bytes --]

--- ispell.el.orig	2008-03-28 12:55:25.000000000 +0100
+++ ispell.el	2008-04-21 12:47:47.000000000 +0200
@@ -881,18 +881,9 @@
 
 ;; Make ispell.el work better with aspell.
 
-(defvar ispell-have-aspell-dictionaries nil
-  "Non-nil if we have queried Aspell for dictionaries at least once.")
-
-(defun ispell-maybe-find-aspell-dictionaries ()
-  "Find Aspell's dictionaries, unless already done."
-  (when (and (not ispell-have-aspell-dictionaries)
-	     (condition-case ()
-		 (progn (ispell-check-version) t)
-	       (error nil))
-	     ispell-really-aspell
-	     ispell-aspell-supports-utf8)
-    (ispell-find-aspell-dictionaries)))
+(defvar ispell-aspell-dictionary-alist nil
+  "An alist of parsed aspell dicts and associated parameters.
+Internal use.")
 
 (defun ispell-find-aspell-dictionaries ()
   "Find Aspell's dictionaries, and record in `ispell-dictionary-alist'."
@@ -915,14 +906,13 @@
     (dolist (dict ispell-dictionary-alist)
       (unless (assoc (car dict) found)
 	(setq found (nconc found (list dict)))))
-    (setq ispell-dictionary-alist found)
+    (setq ispell-aspell-dictionary-alist found)
     ;; Add a default entry
     (let* ((english-dict (assoc "en" ispell-dictionary-alist))
 	   (default-dict
 	     (cons nil (or (cdr english-dict)
 			   (cdr (car ispell-dictionary-alist-1))))))
-      (push default-dict ispell-dictionary-alist))
-    (setq ispell-have-aspell-dictionaries t)))
+      (push default-dict ispell-aspell-dictionary-alist))))
 
 (defvar ispell-aspell-data-dir nil
   "Data directory of Aspell.")
@@ -1004,11 +994,74 @@
 	      (push (cons aliasname (cdr realdict)) alist))))))
     alist))
 
+;; Set params according to the selected spellchecker
+
+(defvar ispell-last-program-name nil
+  "Last value of ispell-program name. Internal use.")
+
+(defvar ispell-initialize-spellchecker-hook nil
+  "Actions to be taken on spellchecker initialization.
+This hook is run when an spellchecker is used for the first
+time, before ``ispell-dictionary-alist'' is set. Is intended for
+sysadmins to override entries in the ispell.el base dictionary-alist
+by putting those overrides in a ``ispell-base-dicts-override-alist''
+alist with same format as ``ispell-dictionary-alist''. This alist
+will not override the auto-detected values if a recent aspell is
+used along with emacs.")
+
+(defun ispell-set-spellchecker-params ()
+  "Initialize some spellchecker parameters when changed or first used."
+  (unless (eq ispell-last-program-name ispell-program-name)
+    (setq ispell-last-program-name ispell-program-name)
+    (ispell-kill-ispell t)
+    (if (and (condition-case ()
+		 (progn
+		   (setq ispell-library-directory (ispell-check-version))
+		   t)
+	       (error nil))
+	     ispell-really-aspell
+	     ispell-aspell-supports-utf8
+	     ;; xemacs does not like [:alpha:] regexps
+	     (string-match "^[[:alpha:]]+$" "abcde"))
+	(unless ispell-aspell-dictionary-alist
+	  (ispell-find-aspell-dictionaries)))
+
+    ;; Substitute ispell-dictionary-alist with the list of dictionaries
+    ;; corresponding to the given spellchecker. If a recent aspell, use
+    ;; the list of really installed dictionaries and add to it elements
+    ;; of the original list that are not present there. Allow distro info.
+    (let ((base-dicts-alist
+	   (append ispell-dictionary-alist-1 ispell-dictionary-alist-2
+		   ispell-dictionary-alist-3 ispell-dictionary-alist-4
+		   ispell-dictionary-alist-5 ispell-dictionary-alist-6))
+	  (found-dicts-alist
+	   (if (and ispell-really-aspell
+		    ispell-aspell-supports-utf8)
+	       ispell-aspell-dictionary-alist
+	     nil))
+	  ispell-base-dicts-override-alist ; Override only base-dicts-alist
+	  all-dicts-alist)
+
+      (run-hooks 'ispell-initialize-spellchecker-hook)
+
+      ;; Add dicts to ``ispell-dictionary-alist'' unless already present.
+      (dolist (dict (append found-dicts-alist
+			    ispell-base-dicts-override-alist
+			    base-dicts-alist))
+	(unless (assoc (car dict) all-dicts-alist)
+	  (add-to-list 'all-dicts-alist dict)))
+      (setq ispell-dictionary-alist all-dicts-alist))))
+
+
 (defun ispell-valid-dictionary-list ()
   "Returns a list of valid dictionaries.
 The variable `ispell-library-directory' defines the library location."
-  ;; If Ispell is really Aspell, query it for the dictionary list.
-  (ispell-maybe-find-aspell-dictionaries)
+  ;; Initialize variables and dictionaries alists for desired spellchecker.
+  ;; Make sure ispell.el is loaded to avoid some autoload loops in xemacs
+  ;; (and may be others)
+  (if (featurep 'ispell)
+      (ispell-set-spellchecker-params))
+
   (let ((dicts (append ispell-local-dictionary-alist ispell-dictionary-alist))
 	(dict-list (cons "default" nil))
 	name load-dict)
@@ -1566,7 +1619,7 @@
     (ispell-region (region-beginning) (region-end)))
    (continue (ispell-continue))
    (t
-    (ispell-maybe-find-aspell-dictionaries)
+    (ispell-set-spellchecker-params)    ; Initialize variables and dicts alists
     (ispell-accept-buffer-local-defs)	; use the correct dictionary
     (let ((cursor-location (point))	; retain cursor location
 	  (word (ispell-get-word following))
@@ -2602,7 +2655,7 @@
 	       (mapcar 'list (ispell-valid-dictionary-list)))
 	  nil t)
 	 current-prefix-arg))
-  (ispell-maybe-find-aspell-dictionaries)
+  (ispell-set-spellchecker-params) ; Initilize variables and dicts alists
   (unless arg (ispell-buffer-local-dict 'no-reload))
   (if (equal dict "default") (setq dict nil))
   ;; This relies on completing-read's bug of returning "" for no match
@@ -2653,7 +2706,7 @@
 Return nil if spell session is quit,
  otherwise returns shift offset amount for last line processed."
   (interactive "r")			; Don't flag errors on read-only bufs.
-  (ispell-maybe-find-aspell-dictionaries)
+  (ispell-set-spellchecker-params)      ; Initialize variables and dicts alists
   (if (not recheckp)
       (ispell-accept-buffer-local-defs)) ; set up dictionary, local words, etc.
   (let ((skip-region-start (make-marker))

[-- Attachment #3: flyspell.el_ispell-set-spellchecker-params.diff --]
[-- Type: text/x-diff, Size: 1536 bytes --]

--- flyspell.el.orig	2008-03-28 12:55:25.000000000 +0100
+++ flyspell.el	2008-03-28 12:58:02.000000000 +0100
@@ -578,7 +578,7 @@
 ;;*---------------------------------------------------------------------*/
 (defun flyspell-mode-on ()
   "Turn Flyspell mode on.  Do not use this; use `flyspell-mode' instead."
-  (ispell-maybe-find-aspell-dictionaries)
+  (ispell-set-spellchecker-params) ; Initialize variables and dicts alists
   (setq ispell-highlight-face 'flyspell-incorrect)
   ;; local dictionaries setup
   (or ispell-local-dictionary ispell-dictionary
@@ -1016,6 +1016,7 @@
 (defun flyspell-word (&optional following)
   "Spell check a word."
   (interactive (list ispell-following-word))
+  (ispell-set-spellchecker-params)    ; Initialize variables and dicts alists
   (save-excursion
     ;; use the correct dictionary
     (flyspell-accept-buffer-local-defs)
@@ -1531,7 +1532,7 @@
     ;; this is done, we can start checking...
     (if flyspell-issue-message-flag (message "Checking region..."))
     (set-buffer curbuf)
-    (ispell-check-version)
+    (ispell-set-spellchecker-params)  ; Initialize variables and dicts alists
     ;; Local dictionary becomes the global dictionary in use.
     (setq ispell-current-dictionary
 	  (or ispell-local-dictionary ispell-dictionary))
@@ -1590,6 +1591,7 @@
 (defun flyspell-region (beg end)
   "Flyspell text between BEG and END."
   (interactive "r")
+  (ispell-set-spellchecker-params)  ; Initialize variables and dicts alists
   (if (= beg end)
       ()
     (save-excursion

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

[Stefan Monnier]

>>>>> "Agustin" == Agustin Martin <agustin.martin@hispalinux.es> writes:

> On Thu, Apr 17, 2008 at 09:38:51PM -0400, Stefan Monnier wrote:

>> I think it's perfectly fine to include it in ispell.el (it doesn't have
>> to be limited to the use of distro's tho, so I'd rather not use the word
>> "distro".  As a general rule, variables and functions should take names
>> that describe what they do rather than in what context they'll be used)

> Thanks, I now notice that this may also be used systemwide by sysadmins
> for e.g., non-standard dicts, so removing the 'distro' word completely and
> using ``ispell-initialize-spellchecker-hook'' instead.

>> 
>> > I think this is better done as currently, just in the middle, that
>> > is, after parsed aspell dicts, but before base-dicts. You do not know where
>> > things come from if things are done at the end, and if you need to check it
>> > you may end reusing half of the function.
>> 
>> Is it ever necessary/important to distinguish whether it comes from the
>> base or fom the parsed dicts?

> I think that only in the function.

>> > If the problem is about using a hook there, something like
>> 
>> No, the problem is the dynamically scoped variables.  But if there's no
>> easy way to do without them, it's OK to use such things.  Just use more
>> descriptive names, and be sure to document them in the hook's docstring.

> Thanks for all the suggestions, I am attaching updated patches. Feel free to
> change if you think there are better names or ways of doing things.

Gracias, installed with the following changelog:

    * ispell.el (ispell-set-spellchecker-params): New function to make sure
    right params and dictionary alists are used after spellchecker changes.
    (ispell-aspell-dictionary-alist, ispell-last-program-name)
    (ispell-initialize-spellchecker-hook): New variables and hook.
    (ispell-find-aspell-dictionaries): Use ispell-aspell-dictionary-alist.
    (ispell-maybe-find-aspell-dictionaries): Remove.
    Calls replaced by (ispell-set-spellchecker-params) calls.
    (ispell-have-aspell-dictionaries): Remove.
    * flyspell.el: Replace ispell-maybe-find-aspell-dictionaries by
    ispell-set-spellchecker-params.

Please notice the format and use of present tense.


        Stefan