GNU bug report logs - #73386
[PATCH] Fix keybinding support for *-auto-reveal

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 73386 in the body.
You can then email your comments to 73386 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Nelson <ultrono <at> gmail.com>
To: bug-auctex <at> gnu.org
Subject: [PATCH] Fix keybinding support for *-auto-reveal
Date: Fri, 20 Sep 2024 16:52:48 +0200

[Message part 1 (text/plain, inline)]

Hello,

I just realized that my earlier patch
8db1e90b6d25beb72b5ebbc706b6def64794dcf8 broke key-binding support for
auto-reveal (i.e., using left/right rather than C-f/b to open previews
and folds).  The attached patch remedies this.  Details below.

Thanks, best,

Paul

---

The user option *-auto-reveal-commands consists of both functions and
lists for keybinding:

(defcustom preview-auto-reveal-commands
  '((key-binding [left])
    (key-binding [right])
    backward-char
    forward-char
    pop-to-mark-command
    undo)
  "List of commands that may cause a preview to be revealed.
This list is consulted by the default value of `preview-auto-reveal'."
  :type '(repeat (choice (function :tag "Function")
                         (sexp :tag "Key binding"))))

When we call (apply #'preview-arrived-via
preview-auto-reveal-commands), the lists for keybinding are not
evaluated, so we never pick up the command to which left/right are
bound.  The attach patch remedies this issue by first evaluating any
keybinding.

In retrospect, a more uniform approach would have been to have all
elements of *-auto-reveal-commands be "something that gets evaluated",
i.e.,

(defcustom preview-auto-reveal-commands
  '((key-binding [left])
    (key-binding [right])
    #'backward-char
    #'forward-char
    #'pop-to-mark-command
    #'undo)
    ...)

This would allow a more uniform default implementation of
*-auto-reveal.  Given that the docstring for *-auto-reveal-commands
says that it should consist of either functions or key bindings, I
think the proposed solution is acceptable.  It's also a bit simpler
for me, since I have a couple packages that add functions (rather than
function symbols) to *-auto-reveal-commands.

[0001-Fix-keybinding-support-for-auto-reveal.patch (application/octet-stream, attachment)]

Message #8 received at 73386 <at> debbugs.gnu.org (full text, mbox):

From: Arash Esbati <arash <at> gnu.org>
To: Paul Nelson <ultrono <at> gmail.com>
Cc: 73386 <at> debbugs.gnu.org
Subject: Re: bug#73386: [PATCH] Fix keybinding support for *-auto-reveal
Date: Wed, 25 Sep 2024 09:21:31 +0200

Hi Paul,

Paul Nelson <ultrono <at> gmail.com> writes:

> I just realized that my earlier patch
> 8db1e90b6d25beb72b5ebbc706b6def64794dcf8 broke key-binding support for
> auto-reveal (i.e., using left/right rather than C-f/b to open previews
> and folds).  The attached patch remedies this.  Details below.

Thanks.  I have one question: Is it possible to set the LEXICAL argument
of `eval' to t?

,----[ C-h f eval RET ]
| eval is a primitive-function in ‘C source code’.
| 
| (eval FORM &optional LEXICAL)
| 
| Evaluate FORM and return its value.
| If LEXICAL is ‘t’, evaluate using lexical binding by default.
| This is the recommended value.
| 
| If absent or ‘nil’, use dynamic scoping only. 
`----

I didn't test your change, but think it should work as well with my
request.

Best, Arash

Message #11 received at 73386 <at> debbugs.gnu.org (full text, mbox):

From: Paul Nelson <ultrono <at> gmail.com>
To: Arash Esbati <arash <at> gnu.org>
Cc: 73386 <at> debbugs.gnu.org
Subject: Re: bug#73386: [PATCH] Fix keybinding support for *-auto-reveal
Date: Wed, 25 Sep 2024 09:47:35 +0200

Hi Arash,

> Thanks.  I have one question: Is it possible to set the LEXICAL argument
> of `eval' to t?

If I understand correctly, your suggestion is to use something like
the following?

(defcustom preview-auto-reveal
      '(eval . ((apply #'preview-arrived-via
                       (mapcar (lambda (cmd)
                                 (if (and (listp cmd) (eq (car cmd)
'key-binding))
                                     (eval cmd)
                                   cmd))
                               preview-auto-reveal-commands))
                t))
...)

I assume this will have no effect, but would be happy to test it out
for a couple days.

Thanks, best,

Paul

Message #14 received at 73386 <at> debbugs.gnu.org (full text, mbox):

From: Arash Esbati <arash <at> gnu.org>
To: Paul Nelson <ultrono <at> gmail.com>
Cc: 73386 <at> debbugs.gnu.org
Subject: Re: bug#73386: [PATCH] Fix keybinding support for *-auto-reveal
Date: Wed, 25 Sep 2024 10:05:25 +0200

Paul Nelson <ultrono <at> gmail.com> writes:

>> Thanks.  I have one question: Is it possible to set the LEXICAL argument
>> of `eval' to t?
>
> If I understand correctly, your suggestion is to use something like
> the following?

Yes, plus the one addition below.

> (defcustom preview-auto-reveal
>       '(eval . ((apply #'preview-arrived-via
>                        (mapcar (lambda (cmd)
>                                  (if (and (listp cmd) (eq (car cmd) 'key-binding))
>                                      (eval cmd)
                                       (eval cmd t)
>                                    cmd))
>                                preview-auto-reveal-commands))
>                 t))
> ...)
>
> I assume this will have no effect, but would be happy to test it out
> for a couple days.

Great, thanks, looking forward to your feedback.

Best, Arash

Message #17 received at 73386 <at> debbugs.gnu.org (full text, mbox):

From: Paul Nelson <ultrono <at> gmail.com>
To: Arash Esbati <arash <at> gnu.org>
Cc: 73386 <at> debbugs.gnu.org
Subject: Re: bug#73386: [PATCH] Fix keybinding support for *-auto-reveal
Date: Thu, 26 Sep 2024 20:43:04 +0200

[Message part 1 (text/plain, inline)]

Seems fine on my end, so I've attached the updated patch.

Thanks, best,
Paul

On Wed, Sep 25, 2024 at 10:05 AM Arash Esbati <arash <at> gnu.org> wrote:
>
> Paul Nelson <ultrono <at> gmail.com> writes:
>
> >> Thanks.  I have one question: Is it possible to set the LEXICAL argument
> >> of `eval' to t?
> >
> > If I understand correctly, your suggestion is to use something like
> > the following?
>
> Yes, plus the one addition below.
>
> > (defcustom preview-auto-reveal
> >       '(eval . ((apply #'preview-arrived-via
> >                        (mapcar (lambda (cmd)
> >                                  (if (and (listp cmd) (eq (car cmd) 'key-binding))
> >                                      (eval cmd)
>                                        (eval cmd t)
> >                                    cmd))
> >                                preview-auto-reveal-commands))
> >                 t))
> > ...)
> >
> > I assume this will have no effect, but would be happy to test it out
> > for a couple days.
>
> Great, thanks, looking forward to your feedback.
>
> Best, Arash

[0001-Fix-keybinding-support-for-auto-reveal.patch (application/octet-stream, attachment)]

Message #22 received at 73386-done <at> debbugs.gnu.org (full text, mbox):

From: Arash Esbati <arash <at> gnu.org>
To: Paul Nelson <ultrono <at> gmail.com>
Cc: 73386-done <at> debbugs.gnu.org
Subject: Re: bug#73386: [PATCH] Fix keybinding support for *-auto-reveal
Date: Thu, 26 Sep 2024 22:57:03 +0200

Paul Nelson <ultrono <at> gmail.com> writes:

> Seems fine on my end, so I've attached the updated patch.

Thanks, I installed your patch.  Closing.

Best, Arash

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.