bug#8147: 24.0.50; Inserting *Help* buffer can lead to data loss

bug#8147: 24.0.50; Inserting *Help* buffer can lead to data loss

[Stephen Berman]

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

In GNU Emacs 24.0.50.1 (i686-pc-linux-gnu, GTK+ Version 2.20.1)
 of 2011-02-23 on escher
Windowing system distributor `The X.Org Foundation', version 11.0.10800000
configured using `configure  '--without-toolkit-scroll-bars' 'CFLAGS=-g -O2 -fno-optimize-sibling-calls''

1. emacs -Q
2. Start editing some buffer.
3. Bring up a doc string in a *Help* buffer with content that contains
   buttons to other doc strings, e.g. `C-h f completing-read'.
4. Return to the buffer in step 2 and type `M-x insert-buffer RET
   *Help*'.  Note that the buttons in the inserted text are active.
5. Click on one of the buttons in the inserted text that links to
   another doc string, e.g. `try-completion'.
=> The previous content of the buffer you were editing is replaced by
the doc string of the just clicked button (e.g. that of
`try-completion').  This buffer is read-only, but if you make it
writable and type `C-x _' to restore the previous content, you get the
message "No undo information in this buffer".  So whatever unsaved data
was in the buffer prior to clicking the button is unrecoverable.

I reported this problem to emacs-devel in May 2008 (see
http://thread.gmane.org/gmane.emacs.devel/97367/) and there were a few
ideas about how to deal with it, but none were pursued.  I just got bit
by it again -- more fool me, perhaps, but it gives me the opportunity to
put it in the bug tracker.  And I offer a fix to the immediate problem
in the following patch, which prevents insert-buffer from inserting
active help buttons.  This extends to insert-buffer the default behavior
of yanking the content of *Help*, since the help button text properties
are in yank-excluded-properties by default.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: insert-buffer patch --]
[-- Type: text/x-emacs-lisp, Size: 426 bytes --]

*** /data/steve/bzr/emacs/trunk/lisp/simple.el	2011-02-23 15:11:01.000000000 +0100
--- /data/steve/bzr/emacs/quickfixes/lisp/simple.el	2011-03-01 15:04:22.000000000 +0100
***************
*** 3626,3631 ****
--- 3626,3632 ----
     (save-excursion
       (insert-buffer-substring (get-buffer buffer))
       (point)))
+   (remove-yank-excluded-properties (point) (mark))
    nil)
  
  (defun append-to-buffer (buffer start end)

[-- Attachment #3: Type: text/plain, Size: 14 bytes --]


Steve Berman

bug#8147: 24.0.50; Inserting *Help* buffer can lead to data loss

[Stefan Monnier]

> I reported this problem to emacs-devel in May 2008 (see
> http://thread.gmane.org/gmane.emacs.devel/97367/) and there were a few
> ideas about how to deal with it, but none were pursued.  I just got bit
> by it again -- more fool me, perhaps, but it gives me the opportunity to
> put it in the bug tracker.  And I offer a fix to the immediate problem

Maybe another approach is for those buttons to check that the current
buffer is (derived-mode-p 'help-mode) before reusing it.


        Stefan

bug#8147: 24.0.50; Inserting *Help* buffer can lead to data loss

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I reported this problem to emacs-devel in May 2008 (see
>> http://thread.gmane.org/gmane.emacs.devel/97367/) and there were a few
>> ideas about how to deal with it, but none were pursued.  I just got bit
>> by it again -- more fool me, perhaps, but it gives me the opportunity to
>> put it in the bug tracker.  And I offer a fix to the immediate problem
>
> Maybe another approach is for those buttons to check that the current
> buffer is (derived-mode-p 'help-mode) before reusing it.

I've committed just such a change.

bug#8147: 24.0.50; Inserting *Help* buffer can lead to data loss

[Stephen Berman]

On Sat, 05 Mar 2011 16:10:20 -0500 Chong Yidong <cyd@stupidchicken.com> wrote:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> I reported this problem to emacs-devel in May 2008 (see
>>> http://thread.gmane.org/gmane.emacs.devel/97367/) and there were a few
>>> ideas about how to deal with it, but none were pursued.  I just got bit
>>> by it again -- more fool me, perhaps, but it gives me the opportunity to
>>> put it in the bug tracker.  And I offer a fix to the immediate problem
>>
>> Maybe another approach is for those buttons to check that the current
>> buffer is (derived-mode-p 'help-mode) before reusing it.
>
> I've committed just such a change.

I just tested this with the doc string of help-buffer in *Help*.  There
are two links in this doc string: clicking on `help-xref-following'
shows the error message "Current buffer is not in Help mode", which is
certainly better than overwriting the content of the buffer; but
clicking on `help-mode.el' finds that file and puts point on the
beginning of help-buffer's definition, i.e., still does what this kind
of link has always done.  It is confusing to have this divergence in
behavior between the two kinds of links.  Instead of signalling an
error, couldn't the help-xref-following buttons just show the help in
the *Help* buffer, as in the following patch?

*** /data/steve/bzr/emacs/trunk/lisp/help-mode.el	2011-02-23 15:11:01.000000000 +0100
--- /data/steve/bzr/emacs/quickfixes/lisp/help-mode.el	2011-03-06 01:13:36.000000000 +0100
***************
*** 408,419 ****
  ;;;###autoload
  (defun help-buffer ()
    "Return the name of a buffer for inserting help.
! If `help-xref-following' is non-nil, this is the name of the
! current buffer.
! Otherwise, it is *Help*; if no buffer with that name currently
! exists, it is created."
    (buffer-name				;for with-output-to-temp-buffer
!    (if help-xref-following
         (current-buffer)
       (get-buffer-create "*Help*"))))
  
--- 408,420 ----
  ;;;###autoload
  (defun help-buffer ()
    "Return the name of a buffer for inserting help.
! If `help-xref-following' is non-nil and the current buffer is
! derived from `help-mode', this is the name of the current buffer.
! Otherwise, return \"*Help*\", creating a buffer with that name if
! it does not already exist."
    (buffer-name				;for with-output-to-temp-buffer
!    (if (and help-xref-following
! 	    (derived-mode-p 'help-mode))
         (current-buffer)
       (get-buffer-create "*Help*"))))
  
Steve Berman

bug#8147: 24.0.50; Inserting *Help* buffer can lead to data loss

[Stephen Berman]

On Sun, 06 Mar 2011 01:24:27 +0100 Stephen Berman <stephen.berman@gmx.net> wrote:

> On Sat, 05 Mar 2011 16:10:20 -0500 Chong Yidong <cyd@stupidchicken.com> wrote:
>
>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>
>>>> I reported this problem to emacs-devel in May 2008 (see
>>>> http://thread.gmane.org/gmane.emacs.devel/97367/) and there were a few
>>>> ideas about how to deal with it, but none were pursued.  I just got bit
>>>> by it again -- more fool me, perhaps, but it gives me the opportunity to
>>>> put it in the bug tracker.  And I offer a fix to the immediate problem
>>>
>>> Maybe another approach is for those buttons to check that the current
>>> buffer is (derived-mode-p 'help-mode) before reusing it.
>>
>> I've committed just such a change.
>
> I just tested this with the doc string of help-buffer in *Help*.  There
> are two links in this doc string: clicking on `help-xref-following'
> shows the error message "Current buffer is not in Help mode", which is
> certainly better than overwriting the content of the buffer; but
> clicking on `help-mode.el' finds that file and puts point on the
> beginning of help-buffer's definition, i.e., still does what this kind
> of link has always done.  It is confusing to have this divergence in
> behavior between the two kinds of links.  Instead of signalling an
> error, couldn't the help-xref-following buttons just show the help in
> the *Help* buffer, as in the following patch?

Sorry, that should have been a patch against the version with your
patch:

*** /data/steve/bzr/emacs/trunk/lisp/help-mode.el	2011-03-06 17:31:58.000000000 +0100
--- /data/steve/bzr/emacs/quickfixes/lisp/help-mode.el	2011-03-06 01:13:36.000000000 +0100
***************
*** 408,424 ****
  ;;;###autoload
  (defun help-buffer ()
    "Return the name of a buffer for inserting help.
! If `help-xref-following' is non-nil, this is the name of the
! current buffer.  Signal an error if this buffer is not derived
! from `help-mode'.
  Otherwise, return \"*Help*\", creating a buffer with that name if
  it does not already exist."
    (buffer-name				;for with-output-to-temp-buffer
!    (if (not help-xref-following)
!        (get-buffer-create "*Help*")
!      (unless (derived-mode-p 'help-mode)
!        (error "Current buffer is not in Help mode"))
!      (current-buffer))))
  
  (defvar help-xref-override-view-map
    (let ((map (make-sparse-keymap)))
--- 408,422 ----
  ;;;###autoload
  (defun help-buffer ()
    "Return the name of a buffer for inserting help.
! If `help-xref-following' is non-nil and the current buffer is
! derived from `help-mode', this is the name of the current buffer.
  Otherwise, return \"*Help*\", creating a buffer with that name if
  it does not already exist."
    (buffer-name				;for with-output-to-temp-buffer
!    (if (and help-xref-following
! 	    (derived-mode-p 'help-mode))
!        (current-buffer)
!      (get-buffer-create "*Help*"))))
  
  (defvar help-xref-override-view-map
    (let ((map (make-sparse-keymap)))


Steve Berman

bug#8147: 24.0.50; Inserting *Help* buffer can lead to data loss

[Chong Yidong]

Stephen Berman <stephen.berman@gmx.net> writes:

>> I just tested this with the doc string of help-buffer in *Help*.  There
>> are two links in this doc string: clicking on `help-xref-following'
>> shows the error message "Current buffer is not in Help mode", which is
>> certainly better than overwriting the content of the buffer; but
>> clicking on `help-mode.el' finds that file and puts point on the
>> beginning of help-buffer's definition, i.e., still does what this kind
>> of link has always done.  It is confusing to have this divergence in
>> behavior between the two kinds of links.  Instead of signalling an
>> error, couldn't the help-xref-following buttons just show the help in
>> the *Help* buffer, as in the following patch?

The buffer from you pasted the button might not be *Help*; it could be
any other buffer in Help mode.  So wouldn't it be inconsistent either
way?

bug#8147: 24.0.50; Inserting *Help* buffer can lead to data loss

[Stephen Berman]

On Sun, 06 Mar 2011 15:28:43 -0500 Chong Yidong <cyd@stupidchicken.com> wrote:

> Stephen Berman <stephen.berman@gmx.net> writes:
>
>> I just tested this with the doc string of help-buffer in *Help*.  There
>> are two links in this doc string: clicking on `help-xref-following'
>> shows the error message "Current buffer is not in Help mode", which is
>> certainly better than overwriting the content of the buffer; but
>> clicking on `help-mode.el' finds that file and puts point on the
>> beginning of help-buffer's definition, i.e., still does what this kind
>> of link has always done.  It is confusing to have this divergence in
>> behavior between the two kinds of links.  Instead of signalling an
>> error, couldn't the help-xref-following buttons just show the help in
>> the *Help* buffer, as in the following patch?
>
> The buffer from you pasted the button might not be *Help*; it could be
> any other buffer in Help mode.  So wouldn't it be inconsistent either
> way?

Do you mean that if the button inserted[1] into a buffer A comes from a
help-mode buffer other than *Help* -- call it B --, you expect that
clicking on the button in A would display the help in B rather than in
*Help*?  This is not my expectation; rather, I would expect the help to
be displayed in *Help*, so there would be no inconsisency.  Do you know
of any cases where it is, or is clearly intended, to be displayed in B
instead of *Help*?  If there isn't any, then maybe help-buffer should
simply always use *Help*, never current-buffer.  I found some
problematic cases in Emacs that seem to support this conclusion.

One case is strokes-help in strokes.el: it uses a help-mode buffer
called *Help with Strokes*, so this is buffer B above.  When I click on
a help link in that buffer, the help is displayed in the same buffer
(using either the original help-buffer, the one with your patch, or the
one with mine) -- but there is no back button, so the only way to see
the strokes help again is to reinvoke strokes-help (which now overwrites
the content of the previous help).  If the links used *Help* instead of
the current buffer, there would be now problem.  Two other cases are
describe-current-coding-system in mule-diag.el and r2b-help in
refbib.el.  Both of these use *Help*, but apparently do not add to
help-xref-stack: if a standard help command is called, the back and
forward buttons never return to the coding system or refbib help, so
here, too, the only way to see the help again is to reinvoke the
command.  These commands should, it seems, either use help-xref-stack or
not call their help buffer *Help*.

Footnotes: 
[1]  Not yanked, since the link property is excluded from yanked text.
Given the complications, maybe it wouldn't be such a bad idea to exclude
it also from insert-buffer, as in my first patch in this bug thread....

bug#8147: 24.0.50; Inserting *Help* buffer can lead to data loss

[Stephen Berman]

Sorry, I mistakenly sent my followup from a different email address.

Steve Berman

On Mon, 07 Mar 2011 00:41:10 +0100 Stephen Berman <Stephen.Berman@rub.de> wrote:

> On Sun, 06 Mar 2011 15:28:43 -0500 Chong Yidong <cyd@stupidchicken.com> wrote:
>
>> Stephen Berman <stephen.berman@gmx.net> writes:
>>
>>> I just tested this with the doc string of help-buffer in *Help*.  There
>>> are two links in this doc string: clicking on `help-xref-following'
>>> shows the error message "Current buffer is not in Help mode", which is
>>> certainly better than overwriting the content of the buffer; but
>>> clicking on `help-mode.el' finds that file and puts point on the
>>> beginning of help-buffer's definition, i.e., still does what this kind
>>> of link has always done.  It is confusing to have this divergence in
>>> behavior between the two kinds of links.  Instead of signalling an
>>> error, couldn't the help-xref-following buttons just show the help in
>>> the *Help* buffer, as in the following patch?
>>
>> The buffer from you pasted the button might not be *Help*; it could be
>> any other buffer in Help mode.  So wouldn't it be inconsistent either
>> way?
>
> Do you mean that if the button inserted[1] into a buffer A comes from a
> help-mode buffer other than *Help* -- call it B --, you expect that
> clicking on the button in A would display the help in B rather than in
> *Help*?  This is not my expectation; rather, I would expect the help to
> be displayed in *Help*, so there would be no inconsisency.  Do you know
> of any cases where it is, or is clearly intended, to be displayed in B
> instead of *Help*?  If there isn't any, then maybe help-buffer should
> simply always use *Help*, never current-buffer.  I found some
> problematic cases in Emacs that seem to support this conclusion.
>
> One case is strokes-help in strokes.el: it uses a help-mode buffer
> called *Help with Strokes*, so this is buffer B above.  When I click on
> a help link in that buffer, the help is displayed in the same buffer
> (using either the original help-buffer, the one with your patch, or the
> one with mine) -- but there is no back button, so the only way to see
> the strokes help again is to reinvoke strokes-help (which now overwrites
> the content of the previous help).  If the links used *Help* instead of
> the current buffer, there would be now problem.  Two other cases are
> describe-current-coding-system in mule-diag.el and r2b-help in
> refbib.el.  Both of these use *Help*, but apparently do not add to
> help-xref-stack: if a standard help command is called, the back and
> forward buttons never return to the coding system or refbib help, so
> here, too, the only way to see the help again is to reinvoke the
> command.  These commands should, it seems, either use help-xref-stack or
> not call their help buffer *Help*.
>
> Footnotes: 
> [1]  Not yanked, since the link property is excluded from yanked text.
> Given the complications, maybe it wouldn't be such a bad idea to exclude
> it also from insert-buffer, as in my first patch in this bug thread....

bug#8147: 24.0.50; Inserting *Help* buffer can lead to data loss

[Stefan Monnier]

> The buffer from you pasted the button might not be *Help*; it could be
> any other buffer in Help mode.  So wouldn't it be inconsistent either
> way?

I don't think that's a problem.  His patch looks good to me.
The main issue is to avoid replacing the buffer's content when that
content is not prepared for such replacement, and help-mode buffers are
supposed to be prepared for it (e.g. proving the help-xref-stack to
"undo").


        Stefan