bug#17007: 24.3.50; describe-key/function evaluates documentation function in the wrong buffer

bug#17007: 24.3.50; describe-key/function evaluates documentation function in the wrong buffer

[João Távora]

Hi

So yasnippet has this bit of code (simplified for this report)

    (put 'yas-expand  'function-documentation
         '(yas--expand-from-trigger-key-doc t))
    (defun yas--expand-from-trigger-key-doc (context)
      "A doc synthesizer for `yas-expand'."
      (concat "Expand a snippet before point. If no snippet
    expansion is possible, "
              (let* ((fallback (yas--keybinding-beyond-yasnippet)))
                (or (and fallback
                         (format "call command `%s'"
                                 (pp-to-string fallback)))
                    "do nothing (`yas-expand' doesn't shadow\nanything)."))))

This used to work fine and output something like

    <tab> runs the command yas-expand, which is an alias for
    `yas-expand-from-trigger-key' in `yasnippet.el'.
     
    (yas-expand &optional FIELD)
     
    Expand a snippet before point. If no snippet
    expansion is possible, call command `indent-for-tab-command'

This has stopped working in 24.3.5 since `with-help-window' started
replaced `with-output-to-temp-buffer' with
`with-temp-buffer-window'. The former just binds `standard-output' while
the latter also sets the current buffer to the *Help* buffer.

The result is that the fallback keybinding reported is always
"forward-button", which is almost always wrong.

One could either

1. revert that change (was it just a cleanup?)
2. fix/parametrize that particular behaviour of
`with-temp-buffer-window'
3. pass an extra original-buffer arg to `describe-function-1'
4. dynamically bind some new `help-original-buffer' var.

Even though a better mechanism for "fallback keybindings" is being
discussed (and by then yasnippet can get rid of its own technique, which
is half-baked but working since emacs 22), it'd be nice if the current
function-documentation trick is kept working for upcoming emacs 24.4.

This originated in https://github.com/capitaomorte/yasnippet/issues/468

http://github.com/capitaomorte/autopair does something similar and is
probably also affected, but I plan to deprecate autopair in favor of
24.4's electric-pair-mode anyway.

Thanks,
João
    

In GNU Emacs 24.3.50.1 (i686-pc-mingw32)
 of 2014-01-04 on LEG570

bug#17007: 24.3.50; describe-key/function evaluates documentation function in the wrong buffer

[Glenn Morris]

João Távora wrote:

> This has stopped working in 24.3.5 since `with-help-window' started
> replaced `with-output-to-temp-buffer' with
> `with-temp-buffer-window'. The former just binds `standard-output' while
> the latter also sets the current buffer to the *Help* buffer.

The former was changed 2014-02-28, so presumably the issue you are
reporting is already gone in the current trunk.

> In GNU Emacs 24.3.50.1 (i686-pc-mingw32)
>  of 2014-01-04 on LEG570

bug#17007: 24.3.50; describe-key/function evaluates documentation function in the wrong buffer

[João Távora]

Glenn Morris <rgm@gnu.org> writes:

> João Távora wrote:
>
>> This has stopped working in 24.3.5 since `with-help-window' started
>> replaced `with-output-to-temp-buffer' with
>> `with-temp-buffer-window'. The former just binds `standard-output' while
>> the latter also sets the current buffer to the *Help* buffer.
>
> The former was changed 2014-02-28, so presumably the issue you are
> reporting is already gone in the current trunk.

Ah, the former is good news, I'll try it out.

João

bug#17007: 24.3.50; describe-key/function evaluates documentation function in the wrong buffer

[João Távora]

joaotavora@gmail.com (João Távora) writes:

> Glenn Morris <rgm@gnu.org> writes:
>> João Távora wrote:
>>> replaced `with-output-to-temp-buffer' with
>>> `with-temp-buffer-window'. The former just binds `standard-output' while
>> The former was changed 2014-02-28, so presumably the issue you are
>> reporting is already gone in the current trunk.
> Ah, the former is good news, I'll try it out.

Alas, the problem persists, but it was probably solved for a week in
February.

I assumed you were talking about revno, which is the only relevant one I
found from 2014-02-28.

    r116606: Revert recent with-temp-buffer-window chang

But I can confirm that it's not working now, with a build from precisely
that day and revno r116614.

It probably worked when the change that 116606 reverts was in place. I
can't find a note in the commit message stating the reason for the
reversion.

=== modified file 'lisp/window.el'
--- a/lisp/window.el    2014-02-21 11:04:27 +0000
+++ b/lisp/window.el    2014-02-28 09:10:55 +0000
@@ -189,8 +188,8 @@
     `(let* ((,buffer (temp-buffer-window-setup ,buffer-or-name))
            (standard-output ,buffer)
            ,window ,value)
-       (setq ,value (progn ,@body))
        (with-current-buffer ,buffer
+        (setq ,value (progn ,@body))
         (setq ,window (temp-buffer-window-show ,buffer ,action)))
 
        (if (functionp ,quit-function)

João

bug#17007: 24.3.50; describe-key/function evaluates documentation function in the wrong buffer

[martin rudalics]

 > This has stopped working in 24.3.5 since `with-help-window' started
 > replaced `with-output-to-temp-buffer' with
 > `with-temp-buffer-window'. The former just binds `standard-output' while
 > the latter also sets the current buffer to the *Help* buffer.
 >
 > The result is that the fallback keybinding reported is always
 > "forward-button", which is almost always wrong.
 >
 > One could either
 >
 > 1. revert that change (was it just a cleanup?)
 > 2. fix/parametrize that particular behaviour of
 > `with-temp-buffer-window'
 > 3. pass an extra original-buffer arg to `describe-function-1'
 > 4. dynamically bind some new `help-original-buffer' var.
 >
 > Even though a better mechanism for "fallback keybindings" is being
 > discussed (and by then yasnippet can get rid of its own technique, which
 > is half-baked but working since emacs 22), it'd be nice if the current
 > function-documentation trick is kept working for upcoming emacs 24.4.
 >
 > This originated in https://github.com/capitaomorte/yasnippet/issues/468
 >
 > http://github.com/capitaomorte/autopair does something similar and is
 > probably also affected, but I plan to deprecate autopair in favor of
 > 24.4's electric-pair-mode anyway.

Sorry, I messed this up too often already.  Hopefully, it's sufficient
to add one `with-current-buffer' binding at some particular level.
Could you please send me the calling sequence used by yasnippet, so I
can identify the location where this is necessary?

martin

bug#17007: 24.3.50; describe-key/function evaluates documentation function in the wrong buffer

[João Távora]

martin rudalics <rudalics@gmx.at> writes:

> Sorry, I messed this up too often already.  Hopefully, it's sufficient
> to add one `with-current-buffer' binding at some particular level.
> Could you please send me the calling sequence used by yasnippet, so I
> can identify the location where this is necessary?

It's simpler if I give you an emacs -Q recipe, right? In your *scratch*
buffer:

    (defun foo ())
    (defun foo-doc ()
      (format "Foo does nothing, and by the way your tab does `%s'"
            	     (key-binding "\t")))
    (put 'foo 'function-documentation '(foo-doc))
    (describe-function 'foo)

This fails on the latest Emacs, i.e. the last line returned by the last
form is

    "Foo does nothing, and by the way your tab does `forward-button'"

Whereas it should report, as in Emacs 24.3

    "Foo does nothing, and by the way your tab does `indent-for-tab-command'"

The trace of functions is the following:

1 -> (describe-function foo)
| 2 -> (describe-function-1 foo)
| | 3 -> (documentation foo t)
| | 3 <- documentation: "Foo does nothing, and by the way your tab does `forward-button'"
| 2 <- describe-function-1: nil
1 <- describe-function: "foo is a Lisp function.

But that doesn't show how the macro `with-temp-buffer-window', expanded
from `with-help-window', eventually wraps the latter's BODY in a
`with-current-buffer' call, making its forms be evaluated in the newly
created *Help* buffer, instead of *scratch*. One of those forms is the
call to `describe-function-1', which eventually calls `foo-doc'.

As I explained in my last email (which got sent from the wrong address
but is here
http://lists.gnu.org/archive/html/bug-gnu-emacs/2014-03/msg00411.html)
it was the reversion of two lines in `with-temp-buffer-window' that
broke it.

But maybe it was fixing something else that I don't understand. Can you
help me understand what you were achieving with the original fix and the
reversion?
    
João

    

bug#17007: 24.3.50; describe-key/function evaluates documentation function in the wrong buffer

[martin rudalics]

 > It's simpler if I give you an emacs -Q recipe, right? In your *scratch*
 > buffer:
 >
 >     (defun foo ())
 >     (defun foo-doc ()
 >       (format "Foo does nothing, and by the way your tab does `%s'"
 >             	     (key-binding "\t")))
 >     (put 'foo 'function-documentation '(foo-doc))
 >     (describe-function 'foo)
 >
 > This fails on the latest Emacs, i.e. the last line returned by the last
 > form is
 >
 >     "Foo does nothing, and by the way your tab does `forward-button'"
 >
 > Whereas it should report, as in Emacs 24.3
 >
 >     "Foo does nothing, and by the way your tab does `indent-for-tab-command'"

Confirmed.

 > The trace of functions is the following:
 >
 > 1 -> (describe-function foo)
 > | 2 -> (describe-function-1 foo)
 > | | 3 -> (documentation foo t)
 > | | 3 <- documentation: "Foo does nothing, and by the way your tab does `forward-button'"
 > | 2 <- describe-function-1: nil
 > 1 <- describe-function: "foo is a Lisp function.
 >
 > But that doesn't show how the macro `with-temp-buffer-window', expanded
 > from `with-help-window', eventually wraps the latter's BODY in a
 > `with-current-buffer' call, making its forms be evaluated in the newly
 > created *Help* buffer, instead of *scratch*.

Indeed.

 > One of those forms is the
 > call to `describe-function-1', which eventually calls `foo-doc'.
 >
 > As I explained in my last email (which got sent from the wrong address
 > but is here
 > http://lists.gnu.org/archive/html/bug-gnu-emacs/2014-03/msg00411.html)
 > it was the reversion of two lines in `with-temp-buffer-window' that
 > broke it.

Not really.  The current version of `with-temp-buffer-window' and that
released with Emacs 24.3 are the same.  It changed only intermittently
and that's what you noticed.  Or can you spot a difference now?

 >  But maybe it was fixing something else that I don't understand. Can
 > you help me understand what you were achieving with the original fix
 > and the reversion?

I first reacted to a bug report by Nicolas Richard

http://debbugs.gnu.org/cgi/bugreport.cgi?bug=16816

and tried to make it behave as in the doc-string.  Unfortunately, this
broke things as subsequently remarked by Juanma in the same thread and
also broke code Thierry wrote as reported in

http://debbugs.gnu.org/cgi/bugreport.cgi?bug=16882

so I decided to restore the 24.3 behavior.  I'm still not fine with that
but it's now some time since 24.3 is out and I'm a bit reluctant to
change it back again, mostly so because I don't know how many bugs such
a change might produce and how soon they'd get detected.

And I'm still too silly to understand how this (apparently?) worked with
24.3 and why it's broken now.  Any ideas?

martin

bug#17007: 24.3.50; describe-key/function evaluates documentation function in the wrong buffer

[Nicolas Richard]

martin rudalics <rudalics@gmx.at> writes:
> And I'm still too silly to understand how this (apparently?) worked with
> 24.3 and why it's broken now.  Any ideas?

The 24.3 version of with-help-window used with-output-to-temp-buffer
AFAICT. I'm looking at a commit made on Sun Mar 10 19:35:23 2013 -0700
by Glenn Morris, tagged emacs-24.3 in the git repo (commit 3a1ce06)

Annotating, it was changed on Date: Sat Nov 30 10:25:31 2013 +0100 as
part of the commit (cc07bdb7) entitled "Support resizing frames and
windows pixelwise."

(Sorry for using only git hashes, I don't have the bzr repo up to date)

-- 
Nico.

bug#17007: 24.3.50; describe-key/function evaluates documentation function in the wrong buffer

[martin rudalics]

> The 24.3 version of with-help-window used with-output-to-temp-buffer
> AFAICT.

You're right.  I must fix this.

Thanks, martin

bug#17007: 24.3.50; describe-key/function evaluates documentation function in the wrong buffer

[martin rudalics]

 > The 24.3 version of with-help-window used with-output-to-temp-buffer
 > AFAICT.

I have tried to fix all problems I found in this area.  If you have some
spare time could you please try to give the new `with-help-window' and
`with-temp-buffer-window' code some checking?  After all, you were the
one who detected the inconsistency that was the root of all evil here
and I trust that you could detect any remaining inconsistencies as well.

Thanks, martin

bug#17007: 24.3.50; describe-key/function evaluates documentation function in the wrong buffer

[Nicolas Richard]

martin rudalics <rudalics@gmx.at> writes:
>> The 24.3 version of with-help-window used with-output-to-temp-buffer
>> AFAICT.
>
> I have tried to fix all problems I found in this area.  If you have some
> spare time could you please try to give the new `with-help-window' and
> `with-temp-buffer-window' code some checking?  After all, you were the
> one who detected the inconsistency that was the root of all evil here
> and I trust that you could detect any remaining inconsistencies as well.

The fact that I found an inconsistency in the first place is because I'm
not a heavy user of that kind of code, hence I relied on the docstring.
Of course I'll report anything that I find, but I don't expect much from
myself.

Thanks for your fixes.

-- 
Nico.

bug#17007: 24.3.50; describe-key/function evaluates documentation function in the wrong buffer

[João Távora]

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

On Sun, Mar 16, 2014 at 10:02 AM, martin rudalics <rudalics@gmx.at> wrote:

> > The 24.3 version of with-help-window used with-output-to-temp-buffer
> > AFAICT.
>
> `with-temp-buffer-window' code some checking?  After all, you were the
> one who detected the inconsistency that was the root of all evil here
> and I trust that you could detect any remaining inconsistencies as well.
>
> Thanks, martin
>

Thanks, will do, as soon as I have time to make a new build.

I wonder if one could write unit tests for these scenarios...

Just a thought, not thoroughly tested, and could be brittle:

(ert-deftest help-window-describe-function-in-correct-buffer
    ()
  "Check if `function-documentation' is eval'ed in right
  buffer."
  (defun help-foo ())
  (defun help-foo-doc ()
    (format "Does nothing, but your tab does `%s'"
            (key-binding "\t")))
  (put 'help-foo 'function-documentation '(foo-doc))
  (let* ((selected-window-before (selected-window))
         (text (describe-function 'help-foo))
         (help-windows (get-buffer-window-list "*Help*")))
    (unwind-protect
        (progn
          (should (eq selected-window-before (selected-window)))
          (should (= 1 (length help-windows)))
          (with-selected-window
              (car help-windows)
            (should-error (search-forward "forward-button"))))
      (delete-other-windows))))

[-- Attachment #2: Type: text/html, Size: 2166 bytes --]

bug#17007: 24.3.50; describe-key/function evaluates documentation function in the wrong buffer

[Juanma Barranquero]

Closing, presumably fixed in

2014-03-16  Martin Rudalics  <rudalics@gmx.at>

        Fix behavior of with-temp-buffer-window (Bug#16816, Bug#17007).

bug#17007: 24.3.50; describe-key/function evaluates documentation function in the wrong buffer

[martin rudalics]

 > It's simpler if I give you an emacs -Q recipe, right? In your *scratch*
 > buffer:
 >
 >     (defun foo ())
 >     (defun foo-doc ()
 >       (format "Foo does nothing, and by the way your tab does `%s'"
 >             	     (key-binding "\t")))
 >     (put 'foo 'function-documentation '(foo-doc))
 >     (describe-function 'foo)
 >
 > This fails on the latest Emacs, i.e. the last line returned by the last
 > form is
 >
 >     "Foo does nothing, and by the way your tab does `forward-button'"
 >
 > Whereas it should report, as in Emacs 24.3
 >
 >     "Foo does nothing, and by the way your tab does `indent-for-tab-command'"

Should work now as intended.  Please try again.

Thanks, martin