* [PATCH] textmodes/flyspell.el (flyspell-post-command-hook): Decided whether to check previous word while in correct buffer.
@ 2013-03-26 18:09 Michal Nazarewicz
2013-03-27 1:09 ` Stefan Monnier
0 siblings, 1 reply; 5+ messages in thread
From: Michal Nazarewicz @ 2013-03-26 18:09 UTC (permalink / raw)
To: Stefan Monnier; +Cc: emacs-devel
From: Michal Nazarewicz <mina86@mina86.com>
If command changed the buffer, the decision may be made based on the
current buffer even though it should based on the previous one. This
may lead to false positives and more importantly to errors since
`flyspell-pre-point' is buffer local so it may have unsanitised value
(such as nil) in previous buffer.
To be honest, I'm not sure how this can happen since
`flyspell-pre-point' is set in previous buffer, but nonetheless, I've
been encountering the error for quite some time and finally decided to
fix it. Interestingly, line making `flyspell-pre-point'
a buffer-local variable has a very revealing "Why?? --Stef" comment.
While at it, remove uneccessary (progn ...).
---
lisp/ChangeLog | 14 ++++++++++++++
lisp/textmodes/flyspell.el | 27 +++++++++++++--------------
2 files changed, 27 insertions(+), 14 deletions(-)
diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index 5b24164..ccd4c13 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,17 @@
+2013-03-26 Michal Nazarewicz <mina86@mina86.com>
+
+ * textmodes/flyspell.el (flyspell-post-command-hook): Decided whether
+ to check previous word while in correct buffer. If command changed
+ the buffer, the decision may be made based on the current buffer even
+ though it should based on the previous one. This may lead to false
+ positives and more importantly to errors since `flyspell-pre-point' is
+ buffer local so it may have unsanitised value (such as nil) in
+ previous buffer. To be honest, I'm not sure how this can happen since
+ `flyspell-pre-point' is set in previous buffer, but nonetheless, I've
+ been encountering the error for quite some time and finally decided to
+ fix it. Interestingly, line making `flyspell-pre-point'
+ a buffer-local variable has a very revealing "Why?? --Stef" comment.
+
2013-03-26 Stefan Monnier <monnier@iro.umontreal.ca>
* desktop.el (desktop--v2s): Rename from desktop-internal-v2s.
diff --git a/lisp/textmodes/flyspell.el b/lisp/textmodes/flyspell.el
index 6ab3e3d..698dac0 100644
--- a/lisp/textmodes/flyspell.el
+++ b/lisp/textmodes/flyspell.el
@@ -955,12 +955,12 @@ Mostly we check word delimiters."
(let ((command this-command)
;; Prevent anything we do from affecting the mark.
deactivate-mark)
- (if (flyspell-check-pre-word-p)
- (with-current-buffer flyspell-pre-buffer
- '(flyspell-debug-signal-pre-word-checked)
+ (when (buffer-live-p flyspell-pre-buffer)
+ (with-current-buffer flyspell-pre-buffer
+ (when (flyspell-check-pre-word-p)
(save-excursion
(goto-char flyspell-pre-point)
- (flyspell-word))))
+ (flyspell-word)))))
(if (flyspell-check-word-p)
(progn
'(flyspell-debug-signal-word-checked)
@@ -974,16 +974,15 @@ Mostly we check word delimiters."
;; FLYSPELL-CHECK-PRE-WORD-P
(setq flyspell-pre-pre-buffer (current-buffer))
(setq flyspell-pre-pre-point (point)))
- (progn
- (setq flyspell-pre-pre-buffer nil)
- (setq flyspell-pre-pre-point nil)
- ;; when a word is not checked because of a delayed command
- ;; we do not disable the ispell cache.
- (if (and (symbolp this-command)
- (get this-command 'flyspell-delayed))
- (progn
- (setq flyspell-word-cache-end -1)
- (setq flyspell-word-cache-result '_)))))
+ (setq flyspell-pre-pre-buffer nil)
+ (setq flyspell-pre-pre-point nil)
+ ;; when a word is not checked because of a delayed command
+ ;; we do not disable the ispell cache.
+ (if (and (symbolp this-command)
+ (get this-command 'flyspell-delayed))
+ (progn
+ (setq flyspell-word-cache-end -1)
+ (setq flyspell-word-cache-result '_))))
(while (and (not (input-pending-p)) (consp flyspell-changes))
(let ((start (car (car flyspell-changes)))
(stop (cdr (car flyspell-changes))))
--
1.8.1.3.718.g2a547ef
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] textmodes/flyspell.el (flyspell-post-command-hook): Decided whether to check previous word while in correct buffer.
2013-03-26 18:09 [PATCH] textmodes/flyspell.el (flyspell-post-command-hook): Decided whether to check previous word while in correct buffer Michal Nazarewicz
@ 2013-03-27 1:09 ` Stefan Monnier
2013-03-27 12:47 ` [PATCHv2] textmodes/flyspell.el: Don't check pre-word if buffer was switched Michal Nazarewicz
0 siblings, 1 reply; 5+ messages in thread
From: Stefan Monnier @ 2013-03-27 1:09 UTC (permalink / raw)
To: Michal Nazarewicz; +Cc: emacs-devel
> If command changed the buffer, the decision may be made based on the
> current buffer even though it should based on the previous one. This
> may lead to false positives and more importantly to errors since
> `flyspell-pre-point' is buffer local so it may have unsanitised value
> (such as nil) in previous buffer.
Can't we just say that if current-buffer is different from the previous
one, then don't do anything?
Stefan
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCHv2] textmodes/flyspell.el: Don't check pre-word if buffer was switched.
2013-03-27 1:09 ` Stefan Monnier
@ 2013-03-27 12:47 ` Michal Nazarewicz
2013-04-20 4:53 ` Agustin Martin
0 siblings, 1 reply; 5+ messages in thread
From: Michal Nazarewicz @ 2013-03-27 12:47 UTC (permalink / raw)
To: Stefan Monnier; +Cc: emacs-devel
If command changed the buffer, the decision may be made based on the
current buffer even though it should based on the previous one. This
may lead to false positives and more importantly to errors since
`flyspell-pre-point' is buffer local so it may have unsanitised value
(such as nil) in previous buffer.
To be honest, I'm not sure how this can happen since
`flyspell-pre-point' is set in previous buffer, but nonetheless, I've
been encountering the error for quite some time and finally decided to
fix it. Interestingly, line making `flyspell-pre-point'
a buffer-local variable has a very revealing "Why?? --Stef" comment.
To avoid the problem, change flyspell-check-pre-word-p so that it does
not allow checking of pre-word if command changed buffer
(ie. `flyspell-pre-buffer' is not current buffer).
---
lisp/ChangeLog | 11 +++++++++++
lisp/textmodes/flyspell.el | 25 +++++++++++--------------
2 files changed, 22 insertions(+), 14 deletions(-)
On Wed, Mar 27 2013, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> Can't we just say that if current-buffer is different from the previous
> one, then don't do anything?
Sure. I don't mind either way.
diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index 5b24164..e310e35 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,14 @@
+2013-03-27 Michal Nazarewicz <mina86@mina86.com>
+
+ * textmodes/flyspell.el (flyspell-check-pre-word-p): Return nil if
+ command changed buffer (ie. `flyspell-pre-buffer' is not current
+ buffer), which prevents making decisions based on invalid value of
+ `flyspell-pre-point' in the wrong buffer. Most notably, this used to
+ cause an error when `flyspell-pre-point' was nil after switching
+ buffers
+ (flyspell-post-command-hook): No longer needs to change buffers when
+ checking pre-word. While at it remove unnecessary progn.
+
2013-03-26 Stefan Monnier <monnier@iro.umontreal.ca>
* desktop.el (desktop--v2s): Rename from desktop-internal-v2s.
diff --git a/lisp/textmodes/flyspell.el b/lisp/textmodes/flyspell.el
index 6ab3e3d..81f17c8 100644
--- a/lisp/textmodes/flyspell.el
+++ b/lisp/textmodes/flyspell.el
@@ -738,7 +738,7 @@ before the current command."
(let ((ispell-otherchars (ispell-get-otherchars)))
(cond
((not (and (numberp flyspell-pre-point)
- (buffer-live-p flyspell-pre-buffer)))
+ (eq flyspell-pre-buffer (current-buffer))))
nil)
((and (eq flyspell-pre-pre-point flyspell-pre-point)
(eq flyspell-pre-pre-buffer flyspell-pre-buffer))
@@ -956,11 +956,10 @@ Mostly we check word delimiters."
;; Prevent anything we do from affecting the mark.
deactivate-mark)
(if (flyspell-check-pre-word-p)
- (with-current-buffer flyspell-pre-buffer
+ (save-excursion
'(flyspell-debug-signal-pre-word-checked)
- (save-excursion
- (goto-char flyspell-pre-point)
- (flyspell-word))))
+ (goto-char flyspell-pre-point)
+ (flyspell-word)))
(if (flyspell-check-word-p)
(progn
'(flyspell-debug-signal-word-checked)
@@ -974,16 +973,14 @@ Mostly we check word delimiters."
;; FLYSPELL-CHECK-PRE-WORD-P
(setq flyspell-pre-pre-buffer (current-buffer))
(setq flyspell-pre-pre-point (point)))
- (progn
- (setq flyspell-pre-pre-buffer nil)
- (setq flyspell-pre-pre-point nil)
- ;; when a word is not checked because of a delayed command
- ;; we do not disable the ispell cache.
- (if (and (symbolp this-command)
+ (setq flyspell-pre-pre-buffer nil)
+ (setq flyspell-pre-pre-point nil)
+ ;; when a word is not checked because of a delayed command
+ ;; we do not disable the ispell cache.
+ (when (and (symbolp this-command)
(get this-command 'flyspell-delayed))
- (progn
- (setq flyspell-word-cache-end -1)
- (setq flyspell-word-cache-result '_)))))
+ (setq flyspell-word-cache-end -1)
+ (setq flyspell-word-cache-result '_)))
(while (and (not (input-pending-p)) (consp flyspell-changes))
(let ((start (car (car flyspell-changes)))
(stop (cdr (car flyspell-changes))))
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCHv2] textmodes/flyspell.el: Don't check pre-word if buffer was switched.
2013-03-27 12:47 ` [PATCHv2] textmodes/flyspell.el: Don't check pre-word if buffer was switched Michal Nazarewicz
@ 2013-04-20 4:53 ` Agustin Martin
2013-04-20 15:46 ` Michal Nazarewicz
0 siblings, 1 reply; 5+ messages in thread
From: Agustin Martin @ 2013-04-20 4:53 UTC (permalink / raw)
To: Michal Nazarewicz; +Cc: emacs-devel
[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]
2013/3/27 Michal Nazarewicz <mina86@mina86.com>
> If command changed the buffer, the decision may be made based on the
> current buffer even though it should based on the previous one. This
> may lead to false positives and more importantly to errors since
> `flyspell-pre-point' is buffer local so it may have unsanitised value
> (such as nil) in previous buffer.
>
> To be honest, I'm not sure how this can happen since
> `flyspell-pre-point' is set in previous buffer, but nonetheless, I've
> been encountering the error for quite some time and finally decided to
> fix it. Interestingly, line making `flyspell-pre-point'
> a buffer-local variable has a very revealing "Why?? --Stef" comment.
>
> To avoid the problem, change flyspell-check-pre-word-p so that it does
> not allow checking of pre-word if command changed buffer
> (ie. `flyspell-pre-buffer' is not current buffer).
> ---
> On Wed, Mar 27 2013, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> > Can't we just say that if current-buffer is different from the previous
> > one, then don't do anything?
>
> Sure. I don't mind either way.
>
Hi, Michal
After doing some testing with flyspell-mode across buffer switches and your
changes applied I finally committed your patch. Although I could not
reproduce the original problem, patch did seem and behave harmless here.
After that I tried harder to reproduce the problem with the original
unchanged code without success, so I have my doubts about this change being
really needed or the best one, `(numberp flyspell-pre-point)' test should
already care of nil `flyspell-pre-point'.
I'd appreciate a minimal step-by-step guide to reproduce the original
problem with the original code, if it can be systematically reproduced. If
it appeared randomly, please provide as much info as you can.
Thanks a lot for your feedback,
--
Agustin
[-- Attachment #2: Type: text/html, Size: 2583 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCHv2] textmodes/flyspell.el: Don't check pre-word if buffer was switched.
2013-04-20 4:53 ` Agustin Martin
@ 2013-04-20 15:46 ` Michal Nazarewicz
0 siblings, 0 replies; 5+ messages in thread
From: Michal Nazarewicz @ 2013-04-20 15:46 UTC (permalink / raw)
To: Agustin Martin; +Cc: emacs-devel
[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]
On Sat, Apr 20 2013, Agustin Martin wrote:
> After that I tried harder to reproduce the problem with the original
> unchanged code without success, so I have my doubts about this change being
> really needed or the best one, `(numberp flyspell-pre-point)' test should
> already care of nil `flyspell-pre-point'.
Not exactly. (numberp flyspell-pre-point) checks the variable before
buffer is switched in `flyspell-post-command-hook' at which point the
variable changes the value.
I confess that I'm not sure how the variable may be nil in given buffer
with `flyspell-pre-buffer' not being nil since those are always set
together, but for some reason I experienced a behaviour which I could
only explain if the following is true:
(and flyspell-pre-buffer
(with-current-buffer flyspell-pre-buffer
(not flyspell-pre-point)))
> I'd appreciate a minimal step-by-step guide to reproduce the original
> problem with the original code, if it can be systematically
> reproduced. If it appeared randomly, please provide as much info as
> you can.
I was experiencing the problem every time I sent an email signed with
PGG. I've never experienced it doing anything else or sending the mail
without signature. The error in *Messages* reads:
Error in post-command-hook (flyspell-post-command-hook): (wrong-type-argument integer-or-marker-p nil)
Unfortunately, I wasn't able to get a minimal set of configuration which
makes the problem appear, so it may be caused by interactions which
whatever hundred of configuration options I'm using.
If you wish to take a look, my configuration files are:
* https://github.com/mina86/dot-files/blob/master/dot-emacs
* https://github.com/mina86/dot-files/blob/master/dot-mail
If you want I can try and take a deeper look later on, but cannot
promise anything this week.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--
[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]
[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-04-20 15:46 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-26 18:09 [PATCH] textmodes/flyspell.el (flyspell-post-command-hook): Decided whether to check previous word while in correct buffer Michal Nazarewicz
2013-03-27 1:09 ` Stefan Monnier
2013-03-27 12:47 ` [PATCHv2] textmodes/flyspell.el: Don't check pre-word if buffer was switched Michal Nazarewicz
2013-04-20 4:53 ` Agustin Martin
2013-04-20 15:46 ` Michal Nazarewicz
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).