More problems with flyspell

More problems with flyspell

[Piet van Oostrum]

Some recent changes in flyspell.el (flyspell-accept-buffer-local-defs) have
caused me a problem. It hit me when I updated my CVS emacs this week:

I want my email and news messages to be flyspell-checked automatically,
preferably with the right language. So in mail-mode-hook, message-mode-hook
and vm-mail-mode-hook I run a function that installs a first-change-hook.
In the latter I do some regular expression grepping to guess what language
the message is in. This only works when replying to a message, for an new
message the default is "Dutch". When I am going to enter a new message in
English, I manually run a command to reset it to English. Each time I call
(flyspell-mode 1). This used to work, but stopped with the recent version.
The reason is the optimization in flyspell-accept-buffer-local-defs. It
won't restart ispell because there was no buffer change. So I think when
flyspell-accept-buffer-local-defs is called from flyspell-mode-on it should
do its work unconditionally. This can be done by setting
flyspell-last-buffer to nil in flyspell-mode-on.

Or maybe another solution is to give flyspell-accept-buffer-local-defs a
parameter that tells whether to do the optimization and only do it in
time-critical situations.
-- 
Piet van Oostrum <piet@cs.uu.nl>
URL: http://www.cs.uu.nl/~piet [PGP 8DAE142BE17999C4]
Private email: piet@vanoostrum.org

Re: More problems with flyspell

[Stefan Monnier]

> Some recent changes in flyspell.el (flyspell-accept-buffer-local-defs) have
> caused me a problem. It hit me when I updated my CVS emacs this week:
[...]
> Or maybe another solution is to give flyspell-accept-buffer-local-defs a
> parameter that tells whether to do the optimization and only do it in
> time-critical situations.

That's what I've done.
Thank you.  It should be fixed now.


        Stefan

Re: More problems with flyspell

[Agustin Martin]

On Thu, Jan 05, 2006 at 02:37:42PM +0100, Piet van Oostrum wrote:
> Some recent changes in flyspell.el (flyspell-accept-buffer-local-defs) have
> caused me a problem. It hit me when I updated my CVS emacs this week:
> 
> I want my email and news messages to be flyspell-checked automatically,
> preferably with the right language. So in mail-mode-hook, message-mode-hook
> and vm-mail-mode-hook I run a function that installs a first-change-hook.
> In the latter I do some regular expression grepping to guess what language
> the message is in. This only works when replying to a message, for an new
> message the default is "Dutch". When I am going to enter a new message in
> English, I manually run a command to reset it to English. Each time I call
> (flyspell-mode 1). This used to work, but stopped with the recent version.
> The reason is the optimization in flyspell-accept-buffer-local-defs. It
> won't restart ispell because there was no buffer change.
> So I think when
> flyspell-accept-buffer-local-defs is called from flyspell-mode-on it should
> do its work unconditionally. This can be done by setting
> flyspell-last-buffer to nil in flyspell-mode-on.
> 
> Or maybe another solution is to give flyspell-accept-buffer-local-defs a
> parameter that tells whether to do the optimization and only do it in
> time-critical situations.

While we are with this, I have noticed that
flyspell-accept-buffer-local-defs is called from flyspell-large-region, but
not from flyspell-small-region. By the way, since none of both is supposed
to be called directly, might be better to move that call to flyspell-region.
that is the interactive function that will internally call
flyspell-{large,small}-region. Since I would expect this to be affected by
the same problem the preferred fix from those you propose above should also
be applied here.

-- 
Agustin

Re: More problems with flyspell

[Richard M. Stallman]

    While we are with this, I have noticed that
    flyspell-accept-buffer-local-defs is called from flyspell-large-region, but
    not from flyspell-small-region.

flyspell-small-region works by calling flyspell-word.  I thought that
flyspell-word arranged to do flyspell-accept-buffer-local-defs, or
the equivalent.  Doesn't it do that?

However, if calling flyspell-accept-buffer-local-defs twice does no
harm, it would be fine to move that call into flyspell-region.

Re: More problems with flyspell

[Agustin Martin]

On Sun, Jan 08, 2006 at 09:47:58AM -0500, Richard M. Stallman wrote:
>     While we are with this, I have noticed that
>     flyspell-accept-buffer-local-defs is called from flyspell-large-region, but
>     not from flyspell-small-region.
> 
> flyspell-small-region works by calling flyspell-word.  I thought that
> flyspell-word arranged to do flyspell-accept-buffer-local-defs, or
> the equivalent.  Doesn't it do that?

Not always with the optimization Piet mentioned, e.g. in next sequence, with
flyspell-region operating over a region that will call
flyspell-small-region

a) Load file and run flyspell-region on a small region. O.K.
   => Starting new Ispell process [francais0] ...
   => Spell Checking completed.
b) Notice you used the wrong dict, ispell-change-dictionary to the new one
   => Loading cl-macs...done
   => Ispell process killed
   => Local Ispell dictionary set to francais
c) Run again flyspell-region on the same region
   => if: Buffer french-LIZEZMOI has no process

> However, if calling flyspell-accept-buffer-local-defs twice does no
> harm, it would be fine to move that call into flyspell-region.

However, you are right, (flyspell-word) is the key here and if instead of
(c) you run flyspell-word over a word the same error appears. I think we can
handle this from ispell.el as part of the
ispell-change-dictionary+double-call problem making sure
flyspell-last-buffer is nil'ed on dict change if flyspell is loaded. this
way first call to flyspell-word would re-run
flyspell-accept-buffer-local-defs and skip that for further calls from the
same buffer. No need to add an explicit call in flyspell-small-region or
flyspell-region.

This might also fix the original problem Piet described, but in a different
way (compatible with the current fix, no need for changes there).

-- 
Agustin

Re: More problems with flyspell

[Agustin Martin]

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

On Mon, Jan 09, 2006 at 01:53:13PM +0100, Agustin Martin wrote:
> c) Run again flyspell-region on the same region
>    => if: Buffer french-LIZEZMOI has no process

Just noticed that this becomes evident when ispell patch for
"flyspell needs ispell-change-dictionary twice" is applied. When using
current plain ispell.el from CVS this error message does not appear, but
things are also buggy; although silently, dict is not changed.

> 
> > However, if calling flyspell-accept-buffer-local-defs twice does no
> > harm, it would be fine to move that call into flyspell-region.
> 
> However, you are right, (flyspell-word) is the key here and if instead of
> (c) you run flyspell-word over a word the same error appears. I think we can
> handle this from ispell.el as part of the
> ispell-change-dictionary+double-call problem making sure
> flyspell-last-buffer is nil'ed on dict change if flyspell is loaded. this
> way first call to flyspell-word would re-run
> flyspell-accept-buffer-local-defs and skip that for further calls from the
> same buffer. No need to add an explicit call in flyspell-small-region or
> flyspell-region.
> 
> This might also fix the original problem Piet described, but in a different
> way (that looks compatible with the current fix, no need for changes there).

I am attaching a possible patch for that. It is done on top of the (not yet
installed) "flyspell needs ispell-change-dictionary twice" patch. It is the
incremental one,

[ispell.el.ispell-internal-change-dictionary+incremental.diff]

(ispell-internal-change-dictionary)
 - Clear out flyspell-last-buffer if flyspell is loaded and dict is changed,
   so (flyspell-accept-buffer-local-defs) is re-run for the new values.

I am also attaching the full patch for current ispell.el from CVS, so it can
be tested better if previous patch is not yet installed when reading this
mail.

[ispell.el.ispell-internal-change-dictionary+full.diff]

-- 
Agustin

[-- Attachment #2: ispell.el.ispell-internal-change-dictionary+incremental.diff --]
[-- Type: text/plain, Size: 700 bytes --]

--- ispell.el.0	2006-01-09 12:49:13.000000000 +0100
+++ ispell.el	2006-01-09 13:22:27.000000000 +0100
@@ -2641,10 +2641,11 @@
       (ispell-kill-ispell t)
       (setq ispell-current-dictionary dict)
       ;; If needed, start ispell process and clear out flyspell word cache
-      (when (and (featurep 'flyspell)
-                 flyspell-mode)
-        (ispell-init-process)
-        (setq flyspell-word-cache-word nil)))))
+      (when (featurep 'flyspell)
+	(setq flyspell-last-buffer nil) ;; re-read buffer-local-defs
+	(when flyspell-mode
+	  (ispell-init-process)
+	  (setq flyspell-word-cache-word nil))))))
 
 ;;; Spelling of comments are checked when ispell-check-comments is non-nil.
 

[-- Attachment #3: ispell.el.ispell-internal-change-dictionary+full.diff --]
[-- Type: text/plain, Size: 1734 bytes --]

--- ispell.el.orig0	2005-12-20 15:06:29.000000000 +0100
+++ ispell.el	2006-01-09 13:22:27.000000000 +0100
@@ -2504,7 +2504,8 @@
       (setq ispell-filter nil ispell-filter-continue nil)
     ;; may need to restart to select new personal dictionary.
     (ispell-kill-ispell t)
-    (message "Starting new Ispell process...")
+    (message "Starting new Ispell process [%s] ..."
+	     (or ispell-local-dictionary ispell-dictionary "default"))
     (sit-for 0)
     (setq ispell-library-directory (ispell-check-version)
 	  ispell-process-directory default-directory
@@ -2619,6 +2620,14 @@
 	       (setq ispell-local-dictionary dict)
 	       (setq ispell-local-dictionary-overridden t))
 	   (error "Undefined dictionary: %s" dict))
+	 ;; For global setting clear out flyspell word cache when needed
+         (when (and arg
+		    (featurep 'flyspell))
+	   (dolist (buf (buffer-list))
+	     (with-current-buffer buf
+	       (when flyspell-mode
+		 (setq flyspell-word-cache-word nil)))))
+	 (ispell-internal-change-dictionary)
 	 (message "%s Ispell dictionary set to %s"
 		  (if arg "Global" "Local")
 		  dict))))
@@ -2630,8 +2639,13 @@
   (let ((dict (or ispell-local-dictionary ispell-dictionary)))
     (unless (equal ispell-current-dictionary dict)
       (ispell-kill-ispell t)
-      (setq ispell-current-dictionary dict))))
-
+      (setq ispell-current-dictionary dict)
+      ;; If needed, start ispell process and clear out flyspell word cache
+      (when (featurep 'flyspell)
+	(setq flyspell-last-buffer nil) ;; re-read buffer-local-defs
+	(when flyspell-mode
+	  (ispell-init-process)
+	  (setq flyspell-word-cache-word nil))))))
 
 ;;; Spelling of comments are checked when ispell-check-comments is non-nil.
 

[-- Attachment #4: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: More problems with flyspell

[Stefan Monnier]

> @@ -2630,8 +2639,13 @@
>    (let ((dict (or ispell-local-dictionary ispell-dictionary)))
>      (unless (equal ispell-current-dictionary dict)
>        (ispell-kill-ispell t)
> -      (setq ispell-current-dictionary dict))))
> -
> +      (setq ispell-current-dictionary dict)
> +      ;; If needed, start ispell process and clear out flyspell word cache
> +      (when (featurep 'flyspell)
> +	(setq flyspell-last-buffer nil) ;; re-read buffer-local-defs
> +	(when flyspell-mode
> +	  (ispell-init-process)
> +	  (setq flyspell-word-cache-word nil))))))
 
Looking at this patch, I feel like my flyspell-last-buffer should probably
be moved from flyspell.el to ispell.el (and renamed ispell-last-buffer).

If someone could do that for me, that'd be swell,


        Stefan


PS: Maybe the same should be done with flyspell-word-cache-word, of course,
but I'm not familiar enough with it.

Re: More problems with flyspell

[Agustin Martin]

On Mon, Jan 09, 2006 at 04:53:31PM -0500, Stefan Monnier wrote:
> Looking at this patch, I feel like my flyspell-last-buffer should probably
> be moved from flyspell.el to ispell.el (and renamed ispell-last-buffer).

If I am not wrong nothing in ispell.el really needs it, is (flyspell-word) in
a loop what needs it. If patch is correct, it will only be set if
flyspell.el is loaded, so flyspell.el seems not that a bad place.  And it
indeed shows name of last buffer accessed by a flyspell operation. Its
presence in ispell.el is a bit surprising, but harmless when flyspell.el is
not loaded.

> PS: Maybe the same should be done with flyspell-word-cache-word, of course,
> but I'm not familiar enough with it.

That is flyspell only, stores last word checked by (flyspell-word), so
flyspell-word is not called again unless cursor goes to a different word.
However is good to have it cleared when dict is changed to force re-checking
of current word, and the dict change is done from ispell.el. I think this
one should not be moved.

-- 
Agustin

Re: More problems with flyspell

[Stefan Monnier]

>> Looking at this patch, I feel like my flyspell-last-buffer should probably
>> be moved from flyspell.el to ispell.el (and renamed ispell-last-buffer).

> If I am not wrong nothing in ispell.el really needs it, is (flyspell-word)
> in a loop what needs it.  If patch is correct, it will only be set if
> flyspell.el is loaded, so flyspell.el seems not that a bad place.  And it
> indeed shows name of last buffer accessed by a flyspell operation.
> Its presence in ispell.el is a bit surprising, but harmless when
> flyspell.el is not loaded.

ispell.el is not just an end-user package: it's also a library that provides
services for flyspell.  The flyspell-last-buffer variable was introduced to
optimize a particular operation because flyspell needs that optimization,
but the right place to put that optimization is in the ispell.el library,
even if the ispell.el end-user commands don't directly benefit from it.

>> PS: Maybe the same should be done with flyspell-word-cache-word, of course,
>> but I'm not familiar enough with it.

> That is flyspell only, stores last word checked by (flyspell-word), so
> flyspell-word is not called again unless cursor goes to a different word.
> However is good to have it cleared when dict is changed to force re-checking
> of current word, and the dict change is done from ispell.el. I think this
> one should not be moved.

Then maybe rather than move flyspell-last-buffer and
flyspell-word-cache-word to ispell.el, ispell.el should provide an
ispell-start-process-hook or ispell-change-dict-hook that flyspell could use
to reset those vars.

Doing a (featurep 'flyspell) test is just really ugly.


        Stefan

Re: More problems with flyspell

[Agustin Martin]

On Mon, Jan 09, 2006 at 11:16:05PM -0500, Stefan Monnier wrote:
> 
> ispell.el is not just an end-user package: it's also a library that provides
> services for flyspell.  The flyspell-last-buffer variable was introduced to
> optimize a particular operation because flyspell needs that optimization,
> but the right place to put that optimization is in the ispell.el library,
> even if the ispell.el end-user commands don't directly benefit from it.
> 
> >> PS: Maybe the same should be done with flyspell-word-cache-word, of course,
> >> but I'm not familiar enough with it.
> 
> I wrote:
> 
> > That is flyspell only, stores last word checked by (flyspell-word), so
> > flyspell-word is not called again unless cursor goes to a different word.
> > However is good to have it cleared when dict is changed to force re-checking
> > of current word, and the dict change is done from ispell.el. I think this
> > one should not be moved.
> 
> Then maybe rather than move flyspell-last-buffer and
> flyspell-word-cache-word to ispell.el, ispell.el should provide an
> ispell-start-process-hook or ispell-change-dict-hook that flyspell could use
> to reset those vars.

This sounds good to me. What others think?

> Doing a (featurep 'flyspell) test is just really ugly.

I admit that even visually, seeing explicit "flyspell" strings and code in
ispell.el is something I dislike too. 

-- 
Agustin