bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

[yyoncho]

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

To reproduce (eval in scratch buffer)

(define-key global-map (kbd "C-c C-l r")
  `(menu-item "2" switch-to-buffer
              :filter ,(lambda (cmd)
                         (print (format "Called in %s" (current-buffer)))
                         cmd)))
(with-temp-buffer
  (with-output-to-string
    (describe-buffer-bindings (get-buffer "*scratch*") [3 12] t)))

This outputs:

\"Called in  *temp*\"

Expected: the filter function must be called in *scratch* since this is
the buffer you want to check the bindings.

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

bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

[Lars Ingebrigtsen]

yyoncho <yyoncho@gmail.com> writes:

> To reproduce (eval in scratch buffer)
> (define-key global-map (kbd "C-c C-l r") 
>   `(menu-item "2" switch-to-buffer
>               :filter ,(lambda (cmd)
>                          (print (format "Called in %s" (current-buffer)))
>                          cmd)))
> (with-temp-buffer
>   (with-output-to-string
>     (describe-buffer-bindings (get-buffer "*scratch*") [3 12] t)))
>
> This outputs:
>
> \"Called in  *temp*\"

I tried this recipe, but I'm not getting the :filter function called at
all.  Do you have a recipe, starting from "emacs -Q", that reproduces
this bug?

And your report didn't include the data about the version generated by
report-emacs-bug, so it's hard to guess whether bug is only on some
specific system.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

[Andreas Schwab]

On Jan 16 2020, yyoncho wrote:

> To reproduce (eval in scratch buffer)
>
> (define-key global-map (kbd "C-c C-l r")
>   `(menu-item "2" switch-to-buffer
>               :filter ,(lambda (cmd)
>                          (print (format "Called in %s" (current-buffer)))
>                          cmd)))
> (with-temp-buffer
>   (with-output-to-string
>     (describe-buffer-bindings (get-buffer "*scratch*") [3 12] t)))
>
> This outputs:
>
> \"Called in  *temp*\"
>
> Expected: the filter function must be called in *scratch* since this is
> the buffer you want to check the bindings.

The problem is that describe-buffer-bindings only changes the buffer to
look up the bindings, but everything that generates the description
(starting with describe-map-tree) doesn't know about that other buffer.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

[yyoncho]

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

I tested with emacs27.1

emacs27.1 -q

Then pasted the code in scratch buffer, evaluated, and reproduced the
described behaviour.


Thanks,
Ivan


On Fri, Oct 30, 2020 at 4:23 PM Lars Ingebrigtsen <larsi@gnus.org> wrote:

> yyoncho <yyoncho@gmail.com> writes:
>
> > To reproduce (eval in scratch buffer)
> > (define-key global-map (kbd "C-c C-l r")
> >   `(menu-item "2" switch-to-buffer
> >               :filter ,(lambda (cmd)
> >                          (print (format "Called in %s" (current-buffer)))
> >                          cmd)))
> > (with-temp-buffer
> >   (with-output-to-string
> >     (describe-buffer-bindings (get-buffer "*scratch*") [3 12] t)))
> >
> > This outputs:
> >
> > \"Called in  *temp*\"
>
> I tried this recipe, but I'm not getting the :filter function called at
> all.  Do you have a recipe, starting from "emacs -Q", that reproduces
> this bug?
>
> And your report didn't include the data about the version generated by
> report-emacs-bug, so it's hard to guess whether bug is only on some
> specific system.
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no
>

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

bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> yyoncho <yyoncho@gmail.com> writes:
>
>> To reproduce (eval in scratch buffer)
>> (define-key global-map (kbd "C-c C-l r")
>>   `(menu-item "2" switch-to-buffer
>>               :filter ,(lambda (cmd)
>>                          (print (format "Called in %s" (current-buffer)))
>>                          cmd)))
>> (with-temp-buffer
>>   (with-output-to-string
>>     (describe-buffer-bindings (get-buffer "*scratch*") [3 12] t)))
>>
>> This outputs:
>>
>> \"Called in  *temp*\"
>
> I tried this recipe, but I'm not getting the :filter function called at
> all.  Do you have a recipe, starting from "emacs -Q", that reproduces
> this bug?
>
> And your report didn't include the data about the version generated by
> report-emacs-bug, so it's hard to guess whether bug is only on some
> specific system.

I can reproduce the above on Emacs 26.3, but not on master.

Note that there might have been changes here with the merge of
scratch/substitute-command-keys.  I tried hard to not introduce any
regressions or even changes in behavior with that branch, but I might
have failed to include the above bug...

What is the result output here?  Here, the last form evaluates to this
on master:

"\f
Global Bindings Starting With C-c C-l:
key             binding
---             -------

\f
Function key map translations Starting With C-c C-l:
key             binding
---             -------

"

Is this correct?  Should it do anything else in addition that it
currently does not?

bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

[Stefan Kangas]

Andreas Schwab <schwab@linux-m68k.org> writes:

> The problem is that describe-buffer-bindings only changes the buffer to
> look up the bindings, but everything that generates the description
> (starting with describe-map-tree) doesn't know about that other buffer.

BTW, does anyone know why describe-map and describe-map-tree currently
inserts its result into buffer instead of just returning a string?

bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

[yyoncho]

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

Even better will be if there is some intermediate structured result which
can be used by packages like which-key instead of parsing the string.

On Fri, Oct 30, 2020 at 6:23 PM Stefan Kangas <stefankangas@gmail.com>
wrote:

> Andreas Schwab <schwab@linux-m68k.org> writes:
>
> > The problem is that describe-buffer-bindings only changes the buffer to
> > look up the bindings, but everything that generates the description
> > (starting with describe-map-tree) doesn't know about that other buffer.
>
> BTW, does anyone know why describe-map and describe-map-tree currently
> inserts its result into buffer instead of just returning a string?
>

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

bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

[Andreas Schwab]

On Okt 30 2020, Stefan Kangas wrote:

> BTW, does anyone know why describe-map and describe-map-tree currently
> inserts its result into buffer instead of just returning a string?

That's what they always did.  It's also more efficient.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

[Basil L. Contovounesios]

Stefan Kangas <stefankangas@gmail.com> writes:

> Note that there might have been changes here with the merge of
> scratch/substitute-command-keys.  I tried hard to not introduce any
> regressions or even changes in behavior with that branch, but I might
> have failed to include the above bug...
>
> What is the result output here?  Here, the last form evaluates to this
> on master:
>
> "\f
> Global Bindings Starting With C-c C-l:
> key             binding
> ---             -------
>
> \f
> Function key map translations Starting With C-c C-l:
> key             binding
> ---             -------
>
> "

FWIW, emacs-27 and master behave differently in this respect;
see e.g. https://github.com/abo-abo/swiper/issues/2709.

The headings used to be inserted in the current buffer along with the
bindings, but the headings alone are now printed to standard-output
instead.

Is that a problem?

Thanks,

-- 
Basil

bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

[Stefan Kangas]

Stefan Kangas <stefankangas@gmail.com> writes:

>>> To reproduce (eval in scratch buffer)
>>> (define-key global-map (kbd "C-c C-l r")
>>>   `(menu-item "2" switch-to-buffer
>>>               :filter ,(lambda (cmd)
>>>                          (print (format "Called in %s" (current-buffer)))
>>>                          cmd)))
>>> (with-temp-buffer
>>>   (with-output-to-string
>>>     (describe-buffer-bindings (get-buffer "*scratch*") [3 12] t)))
>>>
>>> This outputs:
>>>
>>> \"Called in  *temp*\"
>>
>> I tried this recipe, but I'm not getting the :filter function called at
>> all.  Do you have a recipe, starting from "emacs -Q", that reproduces
>> this bug?
>
> I can reproduce the above on Emacs 26.3, but not on master.

This change in behavior was caused by this commit:

commit 8a1441310aa151e739cfed3bd2eff3358edc8001
Author: Stefan Kangas <stefan@marxist.se>
Date:   Sun Oct 18 15:32:22 2020 +0200

    Prefer Lisp version of describe-map-tree

So as suspected, there is a difference in behavior between the new Lisp
version of describe-map-tree and the new C version.

bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

[Stefan Kangas]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> FWIW, emacs-27 and master behave differently in this respect;
> see e.g. https://github.com/abo-abo/swiper/issues/2709.
>
> The headings used to be inserted in the current buffer along with the
> bindings, but the headings alone are now printed to standard-output
> instead.
>
> Is that a problem?

Yes, it is a bug.  I have pushed a fix to master.  Could you please test
that it works for you?

(The fix should mean that you don't have to bind `standard-output' in
swiper.)

bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

[Basil L. Contovounesios]

Stefan Kangas <stefankangas@gmail.com> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> FWIW, emacs-27 and master behave differently in this respect;
>> see e.g. https://github.com/abo-abo/swiper/issues/2709.
>>
>> The headings used to be inserted in the current buffer along with the
>> bindings, but the headings alone are now printed to standard-output
>> instead.
>>
>> Is that a problem?
>
> Yes, it is a bug.  I have pushed a fix to master.  Could you please test
> that it works for you?

Thanks, it does.

> (The fix should mean that you don't have to bind `standard-output' in
> swiper.)

Done.

-- 
Basil

bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> yyoncho <yyoncho@gmail.com> writes:
>
>> To reproduce (eval in scratch buffer)
>> (define-key global-map (kbd "C-c C-l r")
>>   `(menu-item "2" switch-to-buffer
>>               :filter ,(lambda (cmd)
>>                          (print (format "Called in %s" (current-buffer)))
>>                          cmd)))
>> (with-temp-buffer
>>   (with-output-to-string
>>     (describe-buffer-bindings (get-buffer "*scratch*") [3 12] t)))
>>
>> This outputs:
>>
>> \"Called in  *temp*\"
>
> I tried this recipe, but I'm not getting the :filter function called at
> all.  Do you have a recipe, starting from "emacs -Q", that reproduces
> this bug?

I have pushed a fix to master (f08e6538dc) ensuring that we correctly
run the :filter function.

The bug can now be reproduced (on master and 26.3) using:

(progn
  (require 'ert)
  (let (was-in-buffer)
    (unwind-protect
        (progn
          (define-key global-map (kbd "C-c C-l r")
            `(menu-item "2" identity
                        :filter ,(lambda (cmd)
                                   (setq was-in-buffer
                                         (current-buffer))
                                   cmd)))
          (with-temp-buffer
            (let ((buf (get-buffer "*scratch*")))
              (describe-buffer-bindings buf nil t)
              (should was-in-buffer)
              (should (eq was-in-buffer buf)))))
      (define-key global-map (kbd "C-c C-l r") nil)
      (define-key global-map (kbd "C-c C-l") nil))))

As Andreas Schwab has pointed out, the problem is that describe-map (and
describe_map before it) has no idea about the buffer argument to
describe-bindings.  So if the above is to be made to work, we have to
make sure that it does.

I see two ways of doing that:

a) Add a new argument to describe-map with the original buffer.  Ensure
   that the :filter function is run there.

b) Change describe-map to return its output as a string.

The problem with a) is that describe-map already has a pretty long
argument list.  It also seems to me that the resulting code would not be
very easy to follow.

My preference would therefore be looking into option b) first.
If we can make it work, we could then just change the buffer in
describe-buffer-bindings before calling describe-map.

(It is true that the old C function "describe_map" has always inserted
its output in the current buffer.  It also true that it has always been
an internal function, and never exposed to Lisp directly.  So we should
be free to change its behavior, and we should probably better do it
before Emacs 28.1.)

Thoughts?

---

Also, a tangent:

IMHO, using `menu-item' in this way works as it stands, but it is a bit
strange conceptually.  We are binding a key to a `menu-item' that is
never intended to be used for a menu.  Yet IIUC this is the recommended
way of using :filter.

It would perhaps be better to have a specific format that allows for
these extended features, without needing to provide a `menu-item'.

(See `(elisp) Format of Keymaps' for more.)

bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sat, 14 Nov 2020 16:24:07 -0800
> Cc: 39149@debbugs.gnu.org
> 
> a) Add a new argument to describe-map with the original buffer.  Ensure
>    that the :filter function is run there.
> 
> b) Change describe-map to return its output as a string.

Unless I'm missing something, b) would be a backward-incompatible
change, so I don't think we should consider it.

If adding an argument is deemed not a good idea, how about binding a
special variable to convey that information to
describe-buffer-bindings?

bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Kangas <stefankangas@gmail.com>
>> Date: Sat, 14 Nov 2020 16:24:07 -0800
>> Cc: 39149@debbugs.gnu.org
>>
>> a) Add a new argument to describe-map with the original buffer.  Ensure
>>    that the :filter function is run there.
>>
>> b) Change describe-map to return its output as a string.
>
> Unless I'm missing something, b) would be a backward-incompatible
> change, so I don't think we should consider it.

No: `describe-map' was added on master a month ago as a Lisp translation
of the old C function "describe_map".  It has never been exposed to Lisp
directly, but was used internally on the C level by
`substitute-command-keys' and `describe-buffer-bindings'.

What is true is that:

- `substitute-command-keys' has always returned a string.
- `describe-buffer-bindings' has always inserted into the current
  buffer.

Whether we go with option a) or b) shouldn't need to change that.

So we should think carefully about what their interface should be rather
than nail it down to match the old C code.

BTW, while we're on it, I'm not at all sure it would make sense to use
these functions outside of help.el.  I think we might want to consider
renaming `describe-map' to `help--describe-map', and similarly for
`describe-map-tree'.

> If adding an argument is deemed not a good idea, how about binding a
> special variable to convey that information to
> describe-buffer-bindings?

Yes, that could be a third option c).  Personally, I like option b)
better.

bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sun, 15 Nov 2020 08:08:18 -0800
> Cc: larsi@gnus.org, yyoncho@gmail.com, 39149@debbugs.gnu.org
> 
> > If adding an argument is deemed not a good idea, how about binding a
> > special variable to convey that information to
> > describe-buffer-bindings?
> 
> Yes, that could be a third option c).  Personally, I like option b)
> better.

If we need to insert stuff into a buffer, why make it a string and
then insert? why not insert directly?  The latter means less consing,
so less GC.

bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

[Lars Ingebrigtsen]

Stefan Kangas <stefankangas@gmail.com> writes:

> I see two ways of doing that:
>
> a) Add a new argument to describe-map with the original buffer.  Ensure
>    that the :filter function is run there.

I've now done this in Emacs 29.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sun, 17 Apr 2022 17:37:17 +0200
> Cc: 39149@debbugs.gnu.org, yyoncho <yyoncho@gmail.com>
> 
> Stefan Kangas <stefankangas@gmail.com> writes:
> 
> > I see two ways of doing that:
> >
> > a) Add a new argument to describe-map with the original buffer.  Ensure
> >    that the :filter function is run there.
> 
> I've now done this in Emacs 29.

But you didn't push, did you?

bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> But you didn't push, did you?

'course not.  🫠

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no