GNU bug report logs - #58799
Make winner key sequences repeatable in repeat-mode

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 58799 in the body.
You can then email your comments to 58799 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: Damien Cassou <damien <at> cassou.me>
To: bug-gnu-emacs <at> gnu.org
Subject: Make winner key sequences repeatable in repeat-mode
Date: Wed, 26 Oct 2022 17:01:03 +0200

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

Tags: patch





In GNU Emacs 28.2 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.34, cairo version 1.16.0)
Windowing system distributor 'The X.Org Foundation', version 11.0.12014000
System Description: Fedora Linux 36 (Workstation Edition)

Configured using:
 'configure
 --prefix=/nix/store/1ihp8r45mw29fdipjmqrm2vk0fvwd86x-emacs-28.2
 --disable-build-details --with-modules --with-x-toolkit=gtk3 --with-xft
 --with-cairo --with-native-compilation'


-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

[0001-Make-winner-key-sequences-repeatable-in-repeat-mode.patch (text/patch, attachment)]

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Damien Cassou <damien <at> cassou.me>, 58799 <at> debbugs.gnu.org
Subject: Re: bug#58799: Make winner key sequences repeatable in repeat-mode
Date: Wed, 26 Oct 2022 12:52:29 -0700

Damien Cassou <damien <at> cassou.me> writes:

> From 2f26bbad08f71cb4332cd60fa5698a8c18b4abd2 Mon Sep 17 00:00:00 2001
> From: Damien Cassou <damien <at> cassou.me>
> Date: Wed, 26 Oct 2022 16:53:23 +0200
> Subject: [PATCH] Make winner key sequences repeatable in repeat-mode
>
> * lisp/winner.el (winner-repeat-map): New variable.
> (winner-undo): Put 'repeat-map' property with 'winner-repeat-map'.
> (winner-redo): Put 'repeat-map' property with 'winner-repeat-map'.

There is no need to repeat the same text twice, so you can delete the
first occurrence.

> ---
>  lisp/winner.el | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/lisp/winner.el b/lisp/winner.el
> index 174b698e7b..30b0a6ada8 100644
> --- a/lisp/winner.el
> +++ b/lisp/winner.el
> @@ -328,6 +328,14 @@ winner-mode-map
>      map)
>    "Keymap for Winner mode.")
>
> +(defvar-keymap winner-repeat-map
> +  :doc "Keymap to repeat winner key sequences.  Used in `repeat-mode'."
> +  [left] #'winner-undo
> +  [right] #'winner-redo)

It should be "<right>" and "<left>" here, I think.

> +
> +(put #'winner-undo 'repeat-map 'winner-repeat-map)
> +(put #'winner-redo 'repeat-map 'winner-repeat-map)
> +
>  
>  ;;;###autoload
>  (define-minor-mode winner-mode
> --
> 2.36.2

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

From: Robert Pluim <rpluim <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Damien Cassou <damien <at> cassou.me>, 58799 <at> debbugs.gnu.org
Subject: Re: bug#58799: Make winner key sequences repeatable in repeat-mode
Date: Thu, 27 Oct 2022 11:04:29 +0200

>>>>> On Wed, 26 Oct 2022 12:52:29 -0700, Stefan Kangas <stefankangas <at> gmail.com> said:
    >> +(defvar-keymap winner-repeat-map
    >> +  :doc "Keymap to repeat winner key sequences.  Used in `repeat-mode'."
    >> +  [left] #'winner-undo
    >> +  [right] #'winner-redo)

    Stefan> It should be "<right>" and "<left>" here, I think.

Hmm, thatʼs an interesting one:

    (key-valid-p "[right]") => nil
    (key-valid-p "<right>") => t

but

    (kbd "<right>") => [right]
    (keymap-set global-map (kbd "<right>") #ʼignore) => error

:-)

Robert
-- 

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: Damien Cassou <damien <at> cassou.me>, 58799 <at> debbugs.gnu.org
Subject: Re: bug#58799: Make winner key sequences repeatable in repeat-mode
Date: Thu, 27 Oct 2022 03:58:38 -0700

Robert Pluim <rpluim <at> gmail.com> writes:

> Hmm, thatʼs an interesting one:
>
>     (key-valid-p "[right]") => nil
>     (key-valid-p "<right>") => t
>
> but
>
>     (kbd "<right>") => [right]
>     (keymap-set global-map (kbd "<right>") #ʼignore) => error

I don't think I see any contradiction, as `keymap-set' requires its
second argument to be `key-valid-p'.  So that last one should just be:

    (keymap-set global-map "<right>" #'ignore)

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

From: Robert Pluim <rpluim <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Damien Cassou <damien <at> cassou.me>, 58799 <at> debbugs.gnu.org
Subject: Re: bug#58799: Make winner key sequences repeatable in repeat-mode
Date: Thu, 27 Oct 2022 13:39:37 +0200

>>>>> On Thu, 27 Oct 2022 03:58:38 -0700, Stefan Kangas <stefankangas <at> gmail.com> said:

    Stefan> I don't think I see any contradiction, as `keymap-set' requires its
    Stefan> second argument to be `key-valid-p'.  So that last one should just be:

    Stefan>     (keymap-set global-map "<right>" #'ignore)

Now that Iʼve actually run some more tests, I see where the confusion
is coming from. You canʼt actually eval this:

    (defvar-keymap winner-repeat-map
      :doc "Keymap to repeat winner key sequences.  Used in `repeat-mode'."
      [left] #'winner-undo
      [right] #'winner-redo)

or this:

    (defvar-keymap winner-repeat-map
      :doc "Keymap to repeat winner key sequences.  Used in `repeat-mode'."
      <left> #'winner-undo
      <right> #'winner-redo)

(but you can stick them in a .el file)

But you can eval this:

    (defvar-keymap winner-repeat-map
      :doc "Keymap to repeat winner key sequences.  Used in `repeat-mode'."
      "<left>" #'winner-undo
      "<right>" #'winner-redo)

Well, TIL. I donʼt think itʼs going to result in any patches from me
though, unless we want to disallow `[left]' and `[right]' in
`defvar-keymap' :-)

Robert
-- 

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: Damien Cassou <damien <at> cassou.me>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 58799 <at> debbugs.gnu.org
Subject: Re: bug#58799: Make winner key sequences repeatable in repeat-mode
Date: Thu, 27 Oct 2022 05:31:21 -0700

Robert Pluim <rpluim <at> gmail.com> writes:

>     (defvar-keymap winner-repeat-map
>       :doc "Keymap to repeat winner key sequences.  Used in `repeat-mode'."
>       <left> #'winner-undo
>       <right> #'winner-redo)
>
> (but you can stick them in a .el file)

Hmm, right.  I think that's a bug though, because when loading such a
.elc file I get:

keymap--check: [left] is not a valid key definition; see ‘key-valid-p’

So we should probably make sure that the `defvar-keymap' macro errors
out.

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

From: Robert Pluim <rpluim <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Damien Cassou <damien <at> cassou.me>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 58799 <at> debbugs.gnu.org
Subject: Re: bug#58799: Make winner key sequences repeatable in repeat-mode
Date: Thu, 27 Oct 2022 15:38:17 +0200

>>>>> On Thu, 27 Oct 2022 05:31:21 -0700, Stefan Kangas <stefankangas <at> gmail.com> said:

    Stefan> Robert Pluim <rpluim <at> gmail.com> writes:
    >> (defvar-keymap winner-repeat-map
    >> :doc "Keymap to repeat winner key sequences.  Used in `repeat-mode'."
    >> <left> #'winner-undo
    >> <right> #'winner-redo)
    >> 
    >> (but you can stick them in a .el file)

    Stefan> Hmm, right.  I think that's a bug though, because when loading such a
    Stefan> .elc file I get:

    Stefan> keymap--check: [left] is not a valid key definition; see ‘key-valid-p’

    Stefan> So we should probably make sure that the `defvar-keymap' macro errors
    Stefan> out.

Thatʼs easy enough (putting it in `define-keymap' would bring out the
backwards-compatibility police, I think). Probably needs an update to
the `defvar-keymap' docstring as well.

Robert
-- 

diff --git a/lisp/keymap.el b/lisp/keymap.el
index 107565590c..73a9e657fe 100644
--- a/lisp/keymap.el
+++ b/lisp/keymap.el
@@ -581,6 +581,10 @@ defvar-keymap
         (setq key (pop defs))
         (pop defs)
         (when (not (eq key :menu))
+          (when (not (key-valid-p key))
+            (error
+             "Invalid key '%s' in keymap '%s' (see `key-valid-p')"
+             key variable-name))
           (if (member key seen-keys)
               (error "Duplicate definition for key '%s' in keymap '%s'"
                      key variable-name)

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: Damien Cassou <damien <at> cassou.me>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 58799 <at> debbugs.gnu.org
Subject: Re: bug#58799: Make winner key sequences repeatable in repeat-mode
Date: Thu, 27 Oct 2022 09:10:12 -0700

Robert Pluim <rpluim <at> gmail.com> writes:

>     Stefan> So we should probably make sure that the `defvar-keymap' macro errors
>     Stefan> out.
>
> Thatʼs easy enough (putting it in `define-keymap' would bring out the
> backwards-compatibility police, I think). Probably needs an update to
> the `defvar-keymap' docstring as well.

`define-keymap' is new in Emacs 29.1, so I think it will be okay to
change it.

> diff --git a/lisp/keymap.el b/lisp/keymap.el
> index 107565590c..73a9e657fe 100644
> --- a/lisp/keymap.el
> +++ b/lisp/keymap.el
> @@ -581,6 +581,10 @@ defvar-keymap
>          (setq key (pop defs))
>          (pop defs)
>          (when (not (eq key :menu))
> +          (when (not (key-valid-p key))
> +            (error
> +             "Invalid key '%s' in keymap '%s' (see `key-valid-p')"
> +             key variable-name))
>            (if (member key seen-keys)
>                (error "Duplicate definition for key '%s' in keymap '%s'"
>                       key variable-name)

LGTM.

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

From: Robert Pluim <rpluim <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Damien Cassou <damien <at> cassou.me>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 58799 <at> debbugs.gnu.org
Subject: Re: bug#58799: Make winner key sequences repeatable in repeat-mode
Date: Thu, 27 Oct 2022 18:18:37 +0200

>>>>> On Thu, 27 Oct 2022 09:10:12 -0700, Stefan Kangas <stefankangas <at> gmail.com> said:

    Stefan> Robert Pluim <rpluim <at> gmail.com> writes:
    Stefan> So we should probably make sure that the `defvar-keymap' macro errors
    Stefan> out.
    >> 
    >> Thatʼs easy enough (putting it in `define-keymap' would bring out the
    >> backwards-compatibility police, I think). Probably needs an update to
    >> the `defvar-keymap' docstring as well.

    Stefan> `define-keymap' is new in Emacs 29.1, so I think it will be okay to
    Stefan> change it.

OK. Itʼs the logical place to put it.

    >> diff --git a/lisp/keymap.el b/lisp/keymap.el
    >> index 107565590c..73a9e657fe 100644
    >> --- a/lisp/keymap.el
    >> +++ b/lisp/keymap.el
    >> @@ -581,6 +581,10 @@ defvar-keymap
    >> (setq key (pop defs))
    >> (pop defs)
    >> (when (not (eq key :menu))
    >> +          (when (not (key-valid-p key))
    >> +            (error
    >> +             "Invalid key '%s' in keymap '%s' (see `key-valid-p')"
    >> +             key variable-name))
    >> (if (member key seen-keys)
    >> (error "Duplicate definition for key '%s' in keymap '%s'"
    >> key variable-name)

    Stefan> LGTM.

Hmm, in light of the above, maybe the duplicate detection should be
moved to `define-keymap' as well.

Robert
-- 

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

From: Robert Pluim <rpluim <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Damien Cassou <damien <at> cassou.me>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 58799 <at> debbugs.gnu.org
Subject: Re: bug#58799: Make winner key sequences repeatable in repeat-mode
Date: Thu, 27 Oct 2022 18:33:49 +0200

>>>>> On Thu, 27 Oct 2022 18:18:37 +0200, Robert Pluim <rpluim <at> gmail.com> said:

    Robert> Hmm, in light of the above, maybe the duplicate detection should be
    Robert> moved to `define-keymap' as well.

Except itʼs already there. I really must look at my own previous
commits before commenting :-)

Robert
-- 

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

From: Robert Pluim <rpluim <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Damien Cassou <damien <at> cassou.me>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 58799 <at> debbugs.gnu.org
Subject: Re: bug#58799: Make winner key sequences repeatable in repeat-mode
Date: Fri, 28 Oct 2022 11:36:15 +0200

>>>>> On Thu, 27 Oct 2022 09:10:12 -0700, Stefan Kangas <stefankangas <at> gmail.com> said:

    >> diff --git a/lisp/keymap.el b/lisp/keymap.el
    >> index 107565590c..73a9e657fe 100644
    >> --- a/lisp/keymap.el
    >> +++ b/lisp/keymap.el
    >> @@ -581,6 +581,10 @@ defvar-keymap
    >> (setq key (pop defs))
    >> (pop defs)
    >> (when (not (eq key :menu))
    >> +          (when (not (key-valid-p key))
    >> +            (error
    >> +             "Invalid key '%s' in keymap '%s' (see `key-valid-p')"
    >> +             key variable-name))
    >> (if (member key seen-keys)
    >> (error "Duplicate definition for key '%s' in keymap '%s'"
    >> key variable-name)

    Stefan> LGTM.

I now think this would be a bad idea, because of the following type of
code:

    (defvar-keymap footnote-minor-mode-map
      :doc "Keymap used for binding footnote minor mode."
      (key-description footnote-prefix) footnote-mode-map)

Thatʼs perfectly valid, but fails with the above patch unless we

1. Check (key-valid-p (eval key))
2. Add an autoload cookie for footnote-minor-mode-map

That seems too high a price to pay for avoiding the original problem.

Robert
-- 

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: Damien Cassou <damien <at> cassou.me>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 58799 <at> debbugs.gnu.org
Subject: Re: bug#58799: Make winner key sequences repeatable in repeat-mode
Date: Fri, 28 Oct 2022 08:41:37 -0700

Robert Pluim <rpluim <at> gmail.com> writes:

>     (defvar-keymap footnote-minor-mode-map
>       :doc "Keymap used for binding footnote minor mode."
>       (key-description footnote-prefix) footnote-mode-map)
>
> Thatʼs perfectly valid, but fails with the above patch unless we
>
> 1. Check (key-valid-p (eval key))
> 2. Add an autoload cookie for footnote-minor-mode-map
>
> That seems too high a price to pay for avoiding the original problem.

Hmm, that's a too high price to pay.

Could we check for some common mistakes without having to say `(eval
key)' though?  For example by requiring KEY to be

    (or (stringp key) (listp key))

?

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

From: Damien Cassou <damien <at> cassou.me>
To: Stefan Kangas <stefankangas <at> gmail.com>, 58799 <at> debbugs.gnu.org
Subject: Re: bug#58799: Make winner key sequences repeatable in repeat-mode
Date: Sun, 30 Oct 2022 07:39:21 +0100

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

Hi Stefan,

Stefan Kangas <stefankangas <at> gmail.com> writes:
> Damien Cassou <damien <at> cassou.me> writes:
>> * lisp/winner.el (winner-repeat-map): New variable.
>> (winner-undo): Put 'repeat-map' property with 'winner-repeat-map'.
>> (winner-redo): Put 'repeat-map' property with 'winner-repeat-map'.
>
> There is no need to repeat the same text twice, so you can delete the
> first occurrence.


Indeed, fixed.


>> +(defvar-keymap winner-repeat-map
>> +  :doc "Keymap to repeat winner key sequences.  Used in `repeat-mode'."
>> +  [left] #'winner-undo
>> +  [right] #'winner-redo)
>
> It should be "<right>" and "<left>" here, I think.


Fixed.

Thank you very much for your reviews.

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

[0001-Make-winner-key-sequences-repeatable-in-repeat-mode.patch (text/x-patch, attachment)]

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Damien Cassou <damien <at> cassou.me>, 58799 <at> debbugs.gnu.org
Subject: Re: bug#58799: Make winner key sequences repeatable in repeat-mode
Date: Sun, 30 Oct 2022 05:38:28 -0700

Damien Cassou <damien <at> cassou.me> writes:

> From f23ee8ce89bf6e60d97d738f61e4663fd1e2b3dc Mon Sep 17 00:00:00 2001
> From: Damien Cassou <damien <at> cassou.me>
> Date: Wed, 26 Oct 2022 16:53:23 +0200
> Subject: [PATCH] Make winner key sequences repeatable in repeat-mode
>
> * lisp/winner.el (winner-repeat-map): New variable.
> (winner-undo):
> (winner-redo): Put 'repeat-map' property with 'winner-repeat-map'.

Thanks for the patch.  I pushed it with this improved commit message
(also adding the bug number):

    Make winner key sequences repeatable in repeat-mode

    * lisp/winner.el (winner-repeat-map): New variable.
    (winner-undo, winner-redo): Put 'repeat-map' property with
    'winner-repeat-map'.  (Bug#58799)

I'm leaving the bug open as we are still discussing some improvements to
`defvar-keymap'.

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

From: Robert Pluim <rpluim <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Damien Cassou <damien <at> cassou.me>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 58799 <at> debbugs.gnu.org
Subject: Re: bug#58799: Make winner key sequences repeatable in repeat-mode
Date: Mon, 31 Oct 2022 15:25:02 +0100

>>>>> On Fri, 28 Oct 2022 08:41:37 -0700, Stefan Kangas <stefankangas <at> gmail.com> said:

    Stefan> Robert Pluim <rpluim <at> gmail.com> writes:
    >> (defvar-keymap footnote-minor-mode-map
    >> :doc "Keymap used for binding footnote minor mode."
    >> (key-description footnote-prefix) footnote-mode-map)
    >> 
    >> Thatʼs perfectly valid, but fails with the above patch unless we
    >> 
    >> 1. Check (key-valid-p (eval key))
    >> 2. Add an autoload cookie for footnote-minor-mode-map
    >> 
    >> That seems too high a price to pay for avoiding the original problem.

    Stefan> Hmm, that's a too high price to pay.

    Stefan> Could we check for some common mistakes without having to say `(eval
    Stefan> key)' though?  For example by requiring KEY to be

    Stefan>     (or (stringp key) (listp key))

People also do things like this:

(defcustom pong-left-key "4"
  "Alternate key to press for bat 1 to go up (primary one is [left])."
  :type '(restricted-sexp :match-alternatives (stringp vectorp)))

.

(defvar-keymap pong-mode-map
  :doc "Modemap for pong-mode."
  :name 'pong-mode-map
  pong-left-key  #'pong-move-left


so Iʼm inclined to either fix it in `define-keymap' only, or just let
the existing compiler macro for keymaps handle it.

In any case, I think we can close this particular bug.

Robert
-- 

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: Damien Cassou <damien <at> cassou.me>, Lars Ingebrigtsen <larsi <at> gnus.org>,
 58799 <at> debbugs.gnu.org
Subject: Re: bug#58799: Make winner key sequences repeatable in repeat-mode
Date: Mon, 31 Oct 2022 07:47:36 -0700

close 58799 29.1
thanks

Robert Pluim <rpluim <at> gmail.com> writes:

> In any case, I think we can close this particular bug.

Agreed.  Now done.

Previous Next

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