Re: Bug#142608: emacs21: please apply flyspell patch from preview-latex package

Re: Bug#142608: emacs21: please apply flyspell patch from preview-latex package

[Rob Browning]

A recent report suggests Emacs 21.2 may have a bug which interacts
with the preview-latex package.  I've included the two Emacs patches
provided by the preview-latex source below in case you haven't
received them yet through other channels.

Please let me know if you'd like me to include these patches in the
current Debian Emacs 21.2 package.

Thanks

  Agthorr <agthorr@barsoom.org> writes:

  > Package: emacs21
  > Version: 21.2-1
  > Severity: normal
  > Tags: patch
  >
  > The preview-latex package uses the middle mouse button to toggle
  > between previewing images and seeing the LaTeX code.  However, bugs in
  > flyspell.el and mouse-drag.el cause the button to paste instead, if
  > one is also using one of those packages (and I swear by flyspell :>).
  > The preview-latex source package includes patches to fix these bugs.
  > If you could please apply them, it'd be greatly appreciated!



Here are the patches from the preview-latex source
(http://sourceforge.net/projects/preview-latex):

Apply this patch only if middle-mouse clicks on preview-icons paste
instead of toggling when using mouse-drag mode.  After version 21.1 of
Emacs, this should be fixed already.

--- mouse-drag.el.old	Mon Jan  8 20:12:12 2001
+++ mouse-drag.el	Tue Nov 13 22:47:36 2001
@@ -208,7 +208,7 @@
 	 (start-col (car (posn-col-row start-posn)))
 	 (old-selected-window (selected-window))
 	 event end row mouse-delta scroll-delta
-	 have-scrolled point-event-p old-binding
+	 have-scrolled
 	 window-last-row
 	 col mouse-col-delta window-last-col
 	 (scroll-col-delta 0)
@@ -261,18 +261,13 @@
 	      (mouse-drag-safe-scroll scroll-delta scroll-col-delta)
 	      (mouse-drag-repeatedly-safe-scroll scroll-delta scroll-col-delta))))) ;xxx
     ;; If it was a click and not a drag, prepare to pass the event on.
-    ;; Note:  We must determine the pass-through event before restoring
-    ;; the window, but invoke it after.  Sigh.
+    ;; Is there a more correct way to reconstruct the event?
     (if (and (not have-scrolled)
 	     (mouse-drag-events-are-point-events-p start-posn end))
-	(setq point-event-p t
-	      old-binding (key-binding
-			   (vector (event-basic-type start-event)))))
+	(push (cons (event-basic-type start-event) (cdr start-event))
+	      unread-command-events))
     ;; Now restore the old window.
-    (select-window old-selected-window)
-    ;; For clicks, call the old function.
-    (if point-event-p
-	(call-interactively old-binding))))
+    (select-window old-selected-window)))
 
 (defun mouse-drag-drag (start-event)
   "\"Drag\" the page according to a mouse drag.
@@ -297,7 +292,7 @@
 	 (start-col (car (posn-col-row start-posn)))
 	 (old-selected-window (selected-window))
 	 event end row mouse-delta scroll-delta
-	 have-scrolled point-event-p old-binding
+	 have-scrolled
 	 window-last-row
 	 col mouse-col-delta window-last-col
 	 (scroll-col-delta 0)
@@ -335,18 +330,14 @@
 		(setq have-scrolled t)
 		(mouse-drag-safe-scroll scroll-delta scroll-col-delta)))))))
     ;; If it was a click and not a drag, prepare to pass the event on.
-    ;; Note:  We must determine the pass-through event before restoring
-    ;; the window, but invoke it after.  Sigh.
+    ;; Is there a more correct way to reconstruct the event?
     (if (and (not have-scrolled)
 	     (mouse-drag-events-are-point-events-p start-posn end))
-	(setq point-event-p t
-	      old-binding (key-binding
-			   (vector (event-basic-type start-event)))))
+	(push (cons (event-basic-type start-event) (cdr start-event))
+	      unread-command-events))
     ;; Now restore the old window.
-    (select-window old-selected-window)
-    ;; For clicks, call the old function.
-    (if point-event-p
-	(call-interactively old-binding))))
+    (select-window old-selected-window)))
+
 
 (provide 'mouse-drag)

Apply this patch only if middle-mouse clicks on preview-icons paste
instead of toggling when in fly-spell mode.  After version 21.1 of
Emacs, this should be fixed already.

--- /usr/local/emacs-21/share/emacs/21.1/lisp/textmodes/flyspell.el.old	Mon Jul 16 14:22:59 2001
+++ /usr/local/emacs-21/share/emacs/21.1/lisp/textmodes/flyspell.el	Wed Nov  7 16:18:54 2001
@@ -378,9 +378,7 @@
 	  (setq minor-mode-map-alist
 		(cons (cons 'flyspell-mode flyspell-mode-map)
 		      minor-mode-map-alist)))
-  (define-key flyspell-mode-map "\M-\t" 'flyspell-auto-correct-word)
-  (define-key flyspell-mode-map [(mouse-2)]
-    (function flyspell-correct-word/local-keymap)))
+  (define-key flyspell-mode-map "\M-\t" 'flyspell-auto-correct-word))
 
 
 ;; the name of the overlay property that defines the keymap


-- 
Rob Browning
rlb @defaultvalue.org, @linuxdevel.com, and @debian.org
Previously @cs.utexas.edu
GPG=1C58 8B2C FB5E 3F64 EA5C  64AE 78FE E5FE F0CB A0AD

Re: Bug#142608: emacs21: please apply flyspell patch from preview-latex package

[David Kastrup]

I have now checked the patch from Emacs-21.1 to Emacs-21.2.  The fixes
are not contained in Emacs-21.2, so it seems they would have been
judged not important enough for the bug fix release 21.2, which I find
unfortunate.  The bugs are not present in the current CVS version,
however, so it may be hoped that they will not make it into 21.3.  As
the main author of preview-latex, and the author of these patches, I
would recommend applying them at the very least when you are
installing preview-latex.  They are rather certainly not causing more
trouble than the original behavior.  They have been tested, and either
they. or equivalent changes, have been put into the current Emacs
sources long ago.

The bugs will affect every package using overlays with keymaps of
their own, not just preview-latex.  I don't know, however, whether
there are packages around where the impact is as annoying as here.

rlb@defaultvalue.org (Rob Browning) writes:

> A recent report suggests Emacs 21.2 may have a bug which interacts
> with the preview-latex package.  I've included the two Emacs patches
> provided by the preview-latex source below in case you haven't
> received them yet through other channels.
> 
> Please let me know if you'd like me to include these patches in the
> current Debian Emacs 21.2 package.

I will update preview-latex documentation to state that those bugs
remain in Emacs-21.2.  Pity.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum
Email: David.Kastrup@t-online.de

Re: Bug#142608: emacs21: please apply flyspell patch from preview-latex package

[Richard Stallman]

If these bugs are bothering people, let's put the fixes into 21.3.
Could you check them in to RC?
21.3 will be made from RC.

Re: Bug#142608: emacs21: please apply flyspell patch from preview-latex package

[David Kastrup]

Richard Stallman <rms@gnu.org> writes:

> If these bugs are bothering people, let's put the fixes into 21.3.
> Could you check them in to RC?
> 21.3 will be made from RC.

I would first have to get developer access to GNU Emacs, I am
registered as "dak" at Savannah and have signed all the necessary
papers.

However, since the HEAD branch of Emacs has had those particular bugs
fixed for perhaps half a year now (and still has), I would really be
surprised if the fixes (or their supersession by larger changes taking
them into account) would not already be present in RC.  IIRC, 21.3 was
not supposed to be mere bug fixes as compared to 21.2, was it?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum
Email: David.Kastrup@t-online.de

Re: Bug#142608: emacs21: please apply flyspell patch from preview-latex package

[Eli Zaretskii]

On 16 Apr 2002, David Kastrup wrote:

> I would first have to get developer access to GNU Emacs, I am
> registered as "dak" at Savannah and have signed all the necessary
> papers.

I can set you up for write access to the CVS tree, if you want.

> However, since the HEAD branch of Emacs has had those particular bugs
> fixed for perhaps half a year now (and still has), I would really be
> surprised if the fixes (or their supersession by larger changes taking
> them into account) would not already be present in RC.

RC got only safe fixes for grave bugs, so it's possible that it doesn't 
have those changes.  Can you check and say for sure?  (You don't need 
write access for this; anon CVS will do.)

> IIRC, 21.3 was
> not supposed to be mere bug fixes as compared to 21.2, was it?

Originally, yes; but we've changed our minds, since 21.2 has a small 
number of annoying bugs, some of them cause crashes and similar nuisance.

So now it is planned to make 21.3 out of the RC branch.

Re: Bug#142608: emacs21: please apply flyspell patch from preview-latex package

[David Kastrup]

Eli Zaretskii <eliz@is.elta.co.il> writes:

> On 16 Apr 2002, David Kastrup wrote:
> 
> > I would first have to get developer access to GNU Emacs, I am
> > registered as "dak" at Savannah and have signed all the necessary
> > papers.
> 
> I can set you up for write access to the CVS tree, if you want.
> 
> > However, since the HEAD branch of Emacs has had those particular bugs
> > fixed for perhaps half a year now (and still has), I would really be
> > surprised if the fixes (or their supersession by larger changes taking
> > them into account) would not already be present in RC.
> 
> RC got only safe fixes for grave bugs, so it's possible that it doesn't 
> have those changes.  Can you check and say for sure?  (You don't need 
> write access for this; anon CVS will do.)
> 
> > IIRC, 21.3 was
> > not supposed to be mere bug fixes as compared to 21.2, was it?
> 
> Originally, yes; but we've changed our minds, since 21.2 has a small 
> number of annoying bugs, some of them cause crashes and similar nuisance.
> 
> So now it is planned to make 21.3 out of the RC branch.

Where is it?  When looking via the Web interface at the repository, I
only find the branches

patches_21_0
gerd_int
gerd_defvaralias
gerd_dbe
gerd_big
fx-branch
EMACS_21_1_RC
emacs-unicode

- Non-branch tags -

raeburn-tag-7-for-export
raeburn-tag-6-for-export
raeburn-tag-4-for-export
patches_21_0_base
MAIN
HEAD
fx-branch-base
EMACS_PRETEST_21_0_99
EMACS_PRETEST_21_0_98
EMACS_PRETEST_21_0_97
EMACS_PRETEST_21_0_96
EMACS_PRETEST_21_0_95
EMACS_PRETEST_21_0_93
EMACS_PRETEST_21_0_92
EMACS_PRETEST_21_0_91
EMACS_PRETEST_21_0_90
EMACS_PRETEST_21_0_106
EMACS_PRETEST_21_0_105
EMACS_PRETEST_21_0_104
EMACS_PRETEST_21_0_103
EMACS_PRETEST_21_0_102
EMACS_PRETEST_21_0_101
EMACS_PRETEST_21_0_100
EMACS_21_1_BASE
EMACS_21_1
EMACS_20_4
EMACS_20_2
emacs-unicode-base


-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum
Email: David.Kastrup@t-online.de

Re: Bug#142608: emacs21: please apply flyspell patch from preview-latex package

[Juanma Barranquero]

On 16 Apr 2002 16:00:11 +0200, David.Kastrup@t-online.de (David Kastrup) wrote:

> Eli Zaretskii <eliz@is.elta.co.il> writes:

> > So now it is planned to make 21.3 out of the RC branch.
> 
> Where is it?  When looking via the Web interface at the repository, I
> only find the branches

> EMACS_21_1_RC

That's the branch (other than the HEAD) where commits are being made.

                                                           /L/e/k/t/u

Re: Bug#142608: emacs21: please apply flyspell patch from preview-latex package

[Eli Zaretskii]

> From: David.Kastrup@t-online.de (David Kastrup)
> Date: 16 Apr 2002 16:00:11 +0200
> 
> > So now it is planned to make 21.3 out of the RC branch.
> 
> Where is it?

EMACS_21_1_RC is the tag of the RC branch.  Sorry I didn't say that
earlier, I keep forgetting that this isn't common knowledge ;-)

Re: Bug#142608: emacs21: please apply flyspell patch from preview-latex package

[Richard Stallman]

    fixed for perhaps half a year now (and still has), I would really be
    surprised if the fixes (or their supersession by larger changes taking
    them into account) would not already be present in RC.

Mere passage of time does not put anything into RC.
(How could it?)  That happens only under human control.
That is why I suggested you put these into RC.

      IIRC, 21.3 was
    not supposed to be mere bug fixes as compared to 21.2, was it?

We have decided to base 21.3 on RC.  The release from HEAD
will be 21.4.

    I would first have to get developer access to GNU Emacs, I am
    registered as "dak" at Savannah and have signed all the necessary
    papers.

Eli, can you do that?

Re: Bug#142608: emacs21: please apply flyspell patch from preview-latex package

[David Kastrup]

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

Richard Stallman <rms@gnu.org> writes:

>     fixed for perhaps half a year now (and still has), I would really be
>     surprised if the fixes (or their supersession by larger changes taking
>     them into account) would not already be present in RC.
> 
> Mere passage of time does not put anything into RC.
> (How could it?)  That happens only under human control.
> That is why I suggested you put these into RC.
> 
>       IIRC, 21.3 was
>     not supposed to be mere bug fixes as compared to 21.2, was it?
> 
> We have decided to base 21.3 on RC.  The release from HEAD
> will be 21.4.
> 
>     I would first have to get developer access to GNU Emacs, I am
>     registered as "dak" at Savannah and have signed all the necessary
>     papers.
> 
> Eli, can you do that?

I have now verified that indeed flyspell.el and mouse-drag.el do not
contain any fixes to the annoying behavior.  The problem in both of
those cases was that the implemented behavior of the packages stole
mouse-click events on overlays with a keymap property.

The change for flyspell.el is minimally invasive: it just removes the
keybinding causing the problem (the function called would check
whether the click was on the flyspell overlay and call the appropriate
handler if it was, and the local keymap if it wasn't, ignoring any
overlay maps that might actually have priority).  While this
superficially looks like this would stop flyspell's use of the mouse
button itself, this is not the case: flyspell itself uses a keymap on
its overlay, and that functions cleably.  I can only guess that there
must have been some point of time where this did not work reliably,
and thus the local keymap was employed as well, leading to the
problem that when flyspell.el decided it did not really need to treat
the click, it passed it to the wrong handler.

I believe that quite more was changed in HEAD as a result of my report
at the time.  The patch from me is rather small and has tested well.

The change in mouse-drag.el is more thorough: again we have a package
that takes control over an event and then decides whether it will use
the event itself, or pass it on.  Again, this decision is done wrong
as it does not take clicks on overlays into account.  In this case,
the solution was a bit more complicated, but still is shorter than
the original version: I push the event back into the event queue
(after doing the changes to it that mouse-drag does, too) and let
Emacs' event handler figure out what to do instead of trying to
second-guess it.

Both patches have been used for some time by me and appear safe.  I
include them here again and would be glad if they made it into RC:
both have been reported several times as a bug for preview-latex.

I would also be glad to apply them myself if you gave me developer
access.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Make flyspell.el not unnecessarily steal events --]
[-- Type: text/x-patch, Size: 801 bytes --]

Apply this patch only if middle-mouse clicks on preview-icons paste
instead of toggling when in fly-spell mode.  After version 21.2 of
Emacs, this should be fixed already.

--- /usr/local/emacs-21/share/emacs/21.1/lisp/textmodes/flyspell.el.old	Mon Jul 16 14:22:59 2001
+++ /usr/local/emacs-21/share/emacs/21.1/lisp/textmodes/flyspell.el	Wed Nov  7 16:18:54 2001
@@ -378,9 +378,7 @@
 	  (setq minor-mode-map-alist
 		(cons (cons 'flyspell-mode flyspell-mode-map)
 		      minor-mode-map-alist)))
-  (define-key flyspell-mode-map "\M-\t" 'flyspell-auto-correct-word)
-  (define-key flyspell-mode-map [(mouse-2)]
-    (function flyspell-correct-word/local-keymap)))
+  (define-key flyspell-mode-map "\M-\t" 'flyspell-auto-correct-word))
 
 
 ;; the name of the overlay property that defines the keymap


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Make mouse-drag pass events on correctly --]
[-- Type: text/x-patch, Size: 2807 bytes --]

Apply this patch only if middle-mouse clicks on preview-icons paste
instead of toggling when using mouse-drag mode.  After version 21.2 of
Emacs, this should be fixed already.

--- mouse-drag.el.old	Mon Jan  8 20:12:12 2001
+++ mouse-drag.el	Tue Nov 13 22:47:36 2001
@@ -208,7 +208,7 @@
 	 (start-col (car (posn-col-row start-posn)))
 	 (old-selected-window (selected-window))
 	 event end row mouse-delta scroll-delta
-	 have-scrolled point-event-p old-binding
+	 have-scrolled
 	 window-last-row
 	 col mouse-col-delta window-last-col
 	 (scroll-col-delta 0)
@@ -261,18 +261,13 @@
 	      (mouse-drag-safe-scroll scroll-delta scroll-col-delta)
 	      (mouse-drag-repeatedly-safe-scroll scroll-delta scroll-col-delta))))) ;xxx
     ;; If it was a click and not a drag, prepare to pass the event on.
-    ;; Note:  We must determine the pass-through event before restoring
-    ;; the window, but invoke it after.  Sigh.
+    ;; Is there a more correct way to reconstruct the event?
     (if (and (not have-scrolled)
 	     (mouse-drag-events-are-point-events-p start-posn end))
-	(setq point-event-p t
-	      old-binding (key-binding
-			   (vector (event-basic-type start-event)))))
+	(push (cons (event-basic-type start-event) (cdr start-event))
+	      unread-command-events))
     ;; Now restore the old window.
-    (select-window old-selected-window)
-    ;; For clicks, call the old function.
-    (if point-event-p
-	(call-interactively old-binding))))
+    (select-window old-selected-window)))
 
 (defun mouse-drag-drag (start-event)
   "\"Drag\" the page according to a mouse drag.
@@ -297,7 +292,7 @@
 	 (start-col (car (posn-col-row start-posn)))
 	 (old-selected-window (selected-window))
 	 event end row mouse-delta scroll-delta
-	 have-scrolled point-event-p old-binding
+	 have-scrolled
 	 window-last-row
 	 col mouse-col-delta window-last-col
 	 (scroll-col-delta 0)
@@ -335,18 +330,14 @@
 		(setq have-scrolled t)
 		(mouse-drag-safe-scroll scroll-delta scroll-col-delta)))))))
     ;; If it was a click and not a drag, prepare to pass the event on.
-    ;; Note:  We must determine the pass-through event before restoring
-    ;; the window, but invoke it after.  Sigh.
+    ;; Is there a more correct way to reconstruct the event?
     (if (and (not have-scrolled)
 	     (mouse-drag-events-are-point-events-p start-posn end))
-	(setq point-event-p t
-	      old-binding (key-binding
-			   (vector (event-basic-type start-event)))))
+	(push (cons (event-basic-type start-event) (cdr start-event))
+	      unread-command-events))
     ;; Now restore the old window.
-    (select-window old-selected-window)
-    ;; For clicks, call the old function.
-    (if point-event-p
-	(call-interactively old-binding))))
+    (select-window old-selected-window)))
+
 
 (provide 'mouse-drag)

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



-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum
Email: David.Kastrup@t-online.de

Re: Bug#142608: emacs21: please apply flyspell patch from preview-latex package

[Stefan Monnier]

> The change for flyspell.el is minimally invasive: it just removes the
> keybinding causing the problem (the function called would check

I agree with David, this is a good patch that won't introduce
any problem.  The code is only a left over from the "pre local-map
text-properties era".  Keeping the M-tab binding on the minor-mode's
is important because it should also work when invoked from a few chars
after the overlay, but that does not apply to the mouse-2 binding.

> I would also be glad to apply them myself if you gave me developer access.

This would be a great idea.


	Stefan

Re: Bug#142608: emacs21: please apply flyspell patch from preview-latex package

[Richard Stallman]

    > The change for flyspell.el is minimally invasive: it just removes the
    > keybinding causing the problem (the function called would check

    I agree with David, this is a good patch that won't introduce
    any problem.

Could you install that change?

Re: Bug#142608: emacs21: please apply flyspell patch from preview-latex package

[Stefan Monnier]

>     > The change for flyspell.el is minimally invasive: it just removes the
>     > keybinding causing the problem (the function called would check
> 
>     I agree with David, this is a good patch that won't introduce
>     any problem.
> 
> Could you install that change?

Done,


	Stefan

Re: Bug#142608: emacs21: please apply flyspell patch from preview-latex package

[Eli Zaretskii]

> From: David.Kastrup@t-online.de (David Kastrup)
> Date: 17 Apr 2002 23:36:03 +0200
> 
> I would also be glad to apply them myself if you gave me developer
> access.

David, I've set you up for write access to the CVS tree.  In case you
didn't know, the instructions for accessing the CVS are available by
going to http://savannah.gnu.org/projects/emacs and following the link
to "CVS".

Please wait for Richard to approve these patches for the branch,
though.  Thanks.

Re: Bug#142608: emacs21: please apply flyspell patch from preview-latex package

[Richard Stallman]

    Please wait for Richard to approve these patches for the branch,
    though.  Thanks.

I didn't think there was anything else for me to do about this.
What exactly am I being asked to do?

Re: Bug#142608: emacs21: please apply flyspell patch from preview-latex package

[Eli Zaretskii]

> Date: Fri, 19 Apr 2002 12:42:36 -0600 (MDT)
> From: Richard Stallman <rms@gnu.org>
> 
>     Please wait for Richard to approve these patches for the branch,
>     though.  Thanks.
> 
> I didn't think there was anything else for me to do about this.
> What exactly am I being asked to do?

Approve those patcehs for the release branch.  Which you already did
in another message, thanks.

Re: Bug#142608: emacs21: please apply flyspell patch from preview-latex package

[David Kastrup]

"Eli Zaretskii" <eliz@is.elta.co.il> writes:

> > From: David.Kastrup@t-online.de (David Kastrup)
> > Date: 17 Apr 2002 23:36:03 +0200
> > 
> > I would also be glad to apply them myself if you gave me developer
> > access.
> 
> David, I've set you up for write access to the CVS tree.  In case you
> didn't know, the instructions for accessing the CVS are available by
> going to http://savannah.gnu.org/projects/emacs and following the link
> to "CVS".

I have verified now that I can use CVS on Savannah, thanks.  I have
also verified that swifter people than I already have applied all the
necessary patches to the RC branch that I sent you.

Thanks again.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum
Email: David.Kastrup@t-online.de