bug#58799: Make winner key sequences repeatable in repeat-mode

bug#58799: Make winner key sequences repeatable in repeat-mode

[Damien Cassou]

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

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-winner-key-sequences-repeatable-in-repeat-mode.patch --]
[-- Type: text/patch, Size: 1001 bytes --]

From 2f26bbad08f71cb4332cd60fa5698a8c18b4abd2 Mon Sep 17 00:00:00 2001
From: Damien Cassou <damien@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'.
---
 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)
+
+(put #'winner-undo 'repeat-map 'winner-repeat-map)
+(put #'winner-redo 'repeat-map 'winner-repeat-map)
+
 \f
 ;;;###autoload
 (define-minor-mode winner-mode
-- 
2.36.2

bug#58799: Make winner key sequences repeatable in repeat-mode

[Stefan Kangas]

Damien Cassou <damien@cassou.me> writes:

> From 2f26bbad08f71cb4332cd60fa5698a8c18b4abd2 Mon Sep 17 00:00:00 2001
> From: Damien Cassou <damien@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)
> +
>  \f
>  ;;;###autoload
>  (define-minor-mode winner-mode
> --
> 2.36.2

bug#58799: Make winner key sequences repeatable in repeat-mode

[Robert Pluim]

>>>>> On Wed, 26 Oct 2022 12:52:29 -0700, Stefan Kangas <stefankangas@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
-- 

bug#58799: Make winner key sequences repeatable in repeat-mode

[Stefan Kangas]

Robert Pluim <rpluim@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)

bug#58799: Make winner key sequences repeatable in repeat-mode

[Robert Pluim]

>>>>> On Thu, 27 Oct 2022 03:58:38 -0700, Stefan Kangas <stefankangas@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
-- 

bug#58799: Make winner key sequences repeatable in repeat-mode

[Stefan Kangas]

Robert Pluim <rpluim@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.

bug#58799: Make winner key sequences repeatable in repeat-mode

[Robert Pluim]

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

    Stefan> Robert Pluim <rpluim@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)

bug#58799: Make winner key sequences repeatable in repeat-mode

[Stefan Kangas]

Robert Pluim <rpluim@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.

bug#58799: Make winner key sequences repeatable in repeat-mode

[Robert Pluim]

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

    Stefan> Robert Pluim <rpluim@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
-- 

bug#58799: Make winner key sequences repeatable in repeat-mode

[Robert Pluim]

>>>>> On Thu, 27 Oct 2022 18:18:37 +0200, Robert Pluim <rpluim@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
-- 

bug#58799: Make winner key sequences repeatable in repeat-mode

[Robert Pluim]

>>>>> On Thu, 27 Oct 2022 09:10:12 -0700, Stefan Kangas <stefankangas@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
-- 

bug#58799: Make winner key sequences repeatable in repeat-mode

[Stefan Kangas]

Robert Pluim <rpluim@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))

?

bug#58799: Make winner key sequences repeatable in repeat-mode

[Robert Pluim]

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

    Stefan> Robert Pluim <rpluim@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
-- 

bug#58799: Make winner key sequences repeatable in repeat-mode

[Stefan Kangas]

close 58799 29.1
thanks

Robert Pluim <rpluim@gmail.com> writes:

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

Agreed.  Now done.

bug#58799: Make winner key sequences repeatable in repeat-mode

[Damien Cassou]

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

Hi Stefan,

Stefan Kangas <stefankangas@gmail.com> writes:
> Damien Cassou <damien@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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-winner-key-sequences-repeatable-in-repeat-mode.patch --]
[-- Type: text/x-patch, Size: 953 bytes --]

From f23ee8ce89bf6e60d97d738f61e4663fd1e2b3dc Mon Sep 17 00:00:00 2001
From: Damien Cassou <damien@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'.
---
 lisp/winner.el | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lisp/winner.el b/lisp/winner.el
index 174b698e7b..d2040c7e5d 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)
+
+(put #'winner-undo 'repeat-map 'winner-repeat-map)
+(put #'winner-redo 'repeat-map 'winner-repeat-map)
+
 \f
 ;;;###autoload
 (define-minor-mode winner-mode
-- 
2.36.2

bug#58799: Make winner key sequences repeatable in repeat-mode

[Stefan Kangas]

Damien Cassou <damien@cassou.me> writes:

> From f23ee8ce89bf6e60d97d738f61e4663fd1e2b3dc Mon Sep 17 00:00:00 2001
> From: Damien Cassou <damien@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'.