Re: master a6a5d6a: Move 'revert-buffer' global binding to 'C-x g g'

Re: master a6a5d6a: Move 'revert-buffer' global binding to 'C-x g g'

[Stefan Kangas]

larsi@gnus.org (Lars Ingebrigtsen) writes:

> branch: master
> commit a6a5d6a27a86396ab96662fa158cdcc854bd777b
> Author: Sean Whitton <spwhitton@spwhitton.name>
> Commit: Lars Ingebrigtsen <larsi@gnus.org>
>
>     Move 'revert-buffer' global binding to 'C-x g g'
>
>     * lisp/bindings.el: Define ctl-x-g-map and bind 'revert-buffer' to
>     'C-x x g' globally.
>     * doc/emacs/files.texi: Replace 'C-x g' with 'C-x x g'.
>     * etc/NEWS: Document the change (bug#46300).

One drawback of this change is that messages such as the one you get
here are less obvious:

  M-: (ask-user-about-supersession-threat "foo") RET
  C-h

The help text now says:

    Usually, you should type ‘n’ and then ‘C-x x g’,
    to get the latest version of the file, then make the change again.

The "C-x x g" part used to be "M-x revert-buffer" and therefore more
self-explanatory.  (This is due to subsitute-command-keys replacing the
"\\[revert-buffer]" with the new keybinding.)

So perhaps this change should be accompanied with grepping our sources
for "\\[revert-buffer]" and looking over the relevant substitutions to
make sure the help is sufficiently clear?  I see 16 such matches.

Re: master a6a5d6a: Move 'revert-buffer' global binding to 'C-x g g'

[Stefan Monnier]

>     Usually, you should type ‘n’ and then ‘C-x x g’,
>     to get the latest version of the file, then make the change again.
>
> The "C-x x g" part used to be "M-x revert-buffer" and therefore more
> self-explanatory.  (This is due to subsitute-command-keys replacing the
> "\\[revert-buffer]" with the new keybinding.)

I'm tempted to say that in many cases it would make sense to show both.
Maybe doing it for \\[...] would be too disruptive, but maybe we should
introduce a new form which will gets expanded to both the key binding and
the name of the command.


        Stefan

Re: master a6a5d6a: Move 'revert-buffer' global binding to 'C-x g g'

[Stefan Kangas]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>     Usually, you should type ‘n’ and then ‘C-x x g’,
>>     to get the latest version of the file, then make the change again.
>>
>> The "C-x x g" part used to be "M-x revert-buffer" and therefore more
>> self-explanatory.  (This is due to subsitute-command-keys replacing the
>> "\\[revert-buffer]" with the new keybinding.)
>
> I'm tempted to say that in many cases it would make sense to show both.
> Maybe doing it for \\[...] would be too disruptive, but maybe we should
> introduce a new form which will gets expanded to both the key binding and
> the name of the command.

Sure, why not?

I also see a need for a form that expands to the exact text specified
but adds the `help-key-binding' face (to be used e.g. when there is no
keymap to refer to, as in userlock.el, or when we want to display RET as
C-m as in `help-for-help').

I don't know what notation to use here though.  Maybe something like:

    \\[cmd]  -> works as today
    \\[=C-h] -> just adds the formatting to the exact string
    \\[+cmd] -> adds both the key binding and its M-x form

(This would of course break for symbols starting with = or +.)

Re: master a6a5d6a: Move 'revert-buffer' global binding to 'C-x g g'

[Juri Linkov]

> I don't know what notation to use here though.  Maybe something like:
>
>     \\[cmd]  -> works as today
>     \\[=C-h] -> just adds the formatting to the exact string
>     \\[+cmd] -> adds both the key binding and its M-x form
>
> (This would of course break for symbols starting with = or +.)

Maybe displaying both the keybinding and the command name should be
defined by a command symbol's property like 'advertised-binding'
defines the default keybinding to display:

  (put 'revert-buffer :advertised-binding "M-x revert-buffer or C-x x g")

Re: master a6a5d6a: Move 'revert-buffer' global binding to 'C-x g g'

[Stefan Kangas]

Juri Linkov <juri@linkov.net> writes:

>> I don't know what notation to use here though.  Maybe something like:
>>
>>     \\[cmd]  -> works as today
>>     \\[=C-h] -> just adds the formatting to the exact string
>>     \\[+cmd] -> adds both the key binding and its M-x form
>>
>> (This would of course break for symbols starting with = or +.)
>
> Maybe displaying both the keybinding and the command name should be
> defined by a command symbol's property like 'advertised-binding'
> defines the default keybinding to display:
>
>   (put 'revert-buffer :advertised-binding "M-x revert-buffer or C-x x g")

Good idea; it has the benefit that it would be more consistent.  It
would also be nice to add a corresponding `declare'-form.

But we should be careful to make it to also respect local bindings, for
example `revert-buffer' is bound to "g" in `special-mode-map'.

I'm thinking out loud here, but perhaps in some cases we don't want
consistency?  Could it be different in some situations?  Perhaps we
could first review some examples, to see if consistency is indeed what
always we want.  If it always makes sense, then your proposal is a
no-brainer.

Do we have any other commands than `revert-buffer' that could be
candidates for such a property?  I'm asking because it might be useful
to review them with consistency in mind too.

Re: master a6a5d6a: Move 'revert-buffer' global binding to 'C-x g g'

[Juri Linkov]

>>> I don't know what notation to use here though.  Maybe something like:
>>>
>>>     \\[cmd]  -> works as today
>>>     \\[=C-h] -> just adds the formatting to the exact string
>>>     \\[+cmd] -> adds both the key binding and its M-x form
>>>
>>> (This would of course break for symbols starting with = or +.)
>>
>> Maybe displaying both the keybinding and the command name should be
>> defined by a command symbol's property like 'advertised-binding'
>> defines the default keybinding to display:
>>
>>   (put 'revert-buffer :advertised-binding "M-x revert-buffer or C-x x g")
>
> Good idea; it has the benefit that it would be more consistent.  It
> would also be nice to add a corresponding `declare'-form.
>
> But we should be careful to make it to also respect local bindings, for
> example `revert-buffer' is bound to "g" in `special-mode-map'.
>
> I'm thinking out loud here, but perhaps in some cases we don't want
> consistency?  Could it be different in some situations?  Perhaps we
> could first review some examples, to see if consistency is indeed what
> always we want.  If it always makes sense, then your proposal is a
> no-brainer.
>
> Do we have any other commands than `revert-buffer' that could be
> candidates for such a property?  I'm asking because it might be useful
> to review them with consistency in mind too.

I don't know, maybe a special syntax like you proposed above is still needed.

Re: master a6a5d6a: Move 'revert-buffer' global binding to 'C-x g g'

[Stefan Kangas]

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

Stefan Kangas <stefan@marxist.se> writes:

> I don't know what notation to use here though.  Maybe something like:
>
>     \\[cmd]  -> works as today
>     \\[=C-h] -> just adds the formatting to the exact string
>     \\[+cmd] -> adds both the key binding and its M-x form

Introducing the \\[=C-h] substitution (patch 1) does seem to bring
useful simplifications (patch 2).

[-- Attachment #2: 0001-Add-new-format-for-literal-keybindings-to-substitute.patch --]
[-- Type: text/x-diff, Size: 4480 bytes --]

From e427472e6f79007996afd47fd87d1d41bf1dec62 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Thu, 11 Mar 2021 05:28:58 +0100
Subject: [PATCH 1/2] Add new format for literal keybindings to
 substitute-command-keys

* lisp/help.el (substitute-command-keys): Add new format for
literal keybindings.
* test/lisp/help-tests.el
(help-tests-substitute-command-keys/literal-keybinding):
(help-tests-substitute-key-bindings/face-help-key-binding): New tests.
---
 lisp/help.el            | 34 ++++++++++++++++++++++++----------
 test/lisp/help-tests.el | 14 ++++++++++++++
 2 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/lisp/help.el b/lisp/help.el
index 79d8296cfe..d1a817a59d 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -984,6 +984,9 @@ substitute-command-keys
 keystroke sequence that invokes COMMAND, or \"M-x COMMAND\" if COMMAND
 is not on any keys.  Keybindings will use the face `help-key-binding'.
 
+Each substring of the form \\\\=[=KEYBINDING] will be replaced by
+KEYBINDING and use the `help-key-binding' face.
+
 Each substring of the form \\\\={MAPVAR} is replaced by a summary of
 the value of MAPVAR as a keymap.  This summary is similar to the one
 produced by ‘describe-bindings’.  The summary ends in two newlines
@@ -1054,18 +1057,29 @@ substitute-command-keys
                     (setq fun (aref key 1))
                     (setq key (with-current-buffer orig-buf
                                 (where-is-internal fun keymap t))))
-                  (if (not key)
-                      ;; Function is not on any key.
-                      (let ((op (point)))
-                        (insert "M-x ")
-                        (goto-char (+ end-point 3))
-                        (add-text-properties op (point)
-                                             '( face help-key-binding
-                                                font-lock-face help-key-binding))
-                        (delete-char 1))
+                  (cond
+                   ((looking-at "=")
+                    ;; Literal replacement.
+                    (let ((op (point)))
+                      (delete-char 1)
+                      (goto-char (- end-point 2))
+                      (delete-char 1)
+                      (add-text-properties op (point)
+                                           '( face help-key-binding
+                                              font-lock-face help-key-binding))))
+                   ((not key)
+                    ;; Function is not on any key.
+                    (let ((op (point)))
+                      (insert "M-x ")
+                      (goto-char (+ end-point 3))
+                      (add-text-properties op (point)
+                                           '( face help-key-binding
+                                              font-lock-face help-key-binding))
+                      (delete-char 1)))
+                   (t
                     ;; Function is on a key.
                     (delete-char (- end-point (point)))
-                    (insert (help--key-description-fontified key)))))
+                    (insert (help--key-description-fontified key))))))
                ;; 1D. \{foo} is replaced with a summary of the keymap
                ;;            (symbol-value foo).
                ;;     \<foo> just sets the keymap used for \[cmd].
diff --git a/test/lisp/help-tests.el b/test/lisp/help-tests.el
index b2fec5c1bd..09ac01ee64 100644
--- a/test/lisp/help-tests.el
+++ b/test/lisp/help-tests.el
@@ -88,6 +88,20 @@ help-tests-substitute-command-keys/commands
    (test "\\[emacs-version]\\[next-line]" "M-x emacs-versionC-n")
    (test-re "\\[emacs-version]`foo'" "M-x emacs-version[`'‘]foo['’]")))
 
+(ert-deftest help-tests-substitute-command-keys/literal-keybinding ()
+  "Literal replacement."
+  (with-substitute-command-keys-test
+   (test "\\[=C-m]" "C-m")
+   (test "\\[=C-m]\\[=C-j]" "C-mC-j")
+   (test "foo\\[=C-m]bar\\[=C-j]baz" "fooC-mbarC-jbaz")))
+
+(ert-deftest help-tests-substitute-key-bindings/face-help-key-binding ()
+  (should (eq (get-text-property 0 'face (substitute-command-keys "\\[next-line]"))
+              'help-key-binding))
+  (should (eq (get-text-property 0 'face (substitute-command-keys "\\[=f]"))
+              'help-key-binding)))
+
+
 (ert-deftest help-tests-substitute-command-keys/keymaps ()
   (with-substitute-command-keys-test
    (test "\\{minibuffer-local-must-match-map}"
-- 
2.30.1


[-- Attachment #3: 0002-Use-substitute-command-keys-in-userlock.el.patch --]
[-- Type: text/x-diff, Size: 5642 bytes --]

From f5b32d46ce9b29d65a8bf5fee58eca5d65ec4354 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Thu, 11 Mar 2021 05:32:29 +0100
Subject: [PATCH 2/2] Use substitute-command-keys in userlock.el

* lisp/userlock.el (userlock--fontify-key): Remove function.
(ask-user-about-lock, ask-user-about-lock-help)
(ask-user-about-supersession-threat)
(ask-user-about-supersession-help): Use substitute-command-keys.
---
 lisp/userlock.el | 65 +++++++++++++++++-------------------------------
 1 file changed, 23 insertions(+), 42 deletions(-)

diff --git a/lisp/userlock.el b/lisp/userlock.el
index 57311ac99c..f0dbbe7b4c 100644
--- a/lisp/userlock.el
+++ b/lisp/userlock.el
@@ -39,10 +39,6 @@
 
 (define-error 'file-locked "File is locked" 'file-error)
 
-(defun userlock--fontify-key (key)
-  "Add the `help-key-binding' face to string KEY."
-  (propertize key 'face 'help-key-binding))
-
 ;;;###autoload
 (defun ask-user-about-lock (file opponent)
   "Ask user what to do when he wants to edit FILE but it is locked by OPPONENT.
@@ -68,12 +64,9 @@ ask-user-about-lock
 			  (match-string 0 opponent)))
 	      opponent))
       (while (null answer)
-        (message "%s locked by %s: (%s, %s, %s, %s)? "
-                 short-file short-opponent
-                 (userlock--fontify-key "s")
-                 (userlock--fontify-key "q")
-                 (userlock--fontify-key "p")
-                 (userlock--fontify-key "?"))
+        (message (substitute-command-keys
+                  "%s locked by %s: (\\[=s], \\[=q], \\[=p], \\[=?])? ")
+                 short-file short-opponent)
 	(if noninteractive (error "Cannot resolve lock conflict in batch mode"))
 	(let ((tem (let ((inhibit-quit t)
 			 (cursor-in-echo-area t))
@@ -88,12 +81,9 @@ ask-user-about-lock
 				      (?? . help))))
 	    (cond ((null answer)
 		   (beep)
-                   (message "Please type %s, %s, or %s; or %s for help"
-                            (userlock--fontify-key "q")
-                            (userlock--fontify-key "s")
-                            (userlock--fontify-key "p")
-                            ;; FIXME: Why do we use "?" here and "C-h" below?
-                            (userlock--fontify-key "?"))
+                   ;; FIXME: Why do we use "?" here and "C-h" below?
+                   (message (substitute-command-keys
+                             "Please type \\[=q], \\[=s], or \\[=p]; or \\[=?] for help"))
 		   (sit-for 3))
 		  ((eq (cdr answer) 'help)
 		   (ask-user-about-lock-help)
@@ -106,17 +96,14 @@ ask-user-about-lock-help
   (with-output-to-temp-buffer "*Help*"
     (with-current-buffer standard-output
       (insert
-       (format
+       (substitute-command-keys
         "It has been detected that you want to modify a file that someone else has
 already started modifying in Emacs.
 
-You can <%s>teal the file; the other user becomes the
+You can <\\[=s]>teal the file; the other user becomes the
   intruder if (s)he ever unmodifies the file and then changes it again.
-You can <%s>roceed; you edit at your own (and the other user's) risk.
-You can <%s>uit; don't modify this file."
-        (userlock--fontify-key "s")
-        (userlock--fontify-key "p")
-        (userlock--fontify-key "q")))
+You can <\\[=p]>roceed; you edit at your own (and the other user's) risk.
+You can <\\[=q]>uit; don't modify this file."))
       (help-mode))))
 
 (define-error 'file-supersession nil 'file-error)
@@ -168,14 +155,11 @@ ask-user-about-supersession-threat
   (discard-input)
   (save-window-excursion
     (let ((prompt
-	   (format "%s changed on disk; \
-really edit the buffer? (%s, %s, %s or %s) "
-                   (file-name-nondirectory filename)
-                   (userlock--fontify-key "y")
-                   (userlock--fontify-key "n")
-                   (userlock--fontify-key "r")
-                   ;; FIXME: Why do we use "C-h" here and "?" above?
-                   (userlock--fontify-key "C-h")))
+           ;; FIXME: Why do we use "C-h" here and "?" above?
+           (format (substitute-command-keys
+                    "%s changed on disk; \
+really edit the buffer? (\\[=y], \\[=n], \\[=r] or \\[=C-h]) ")
+                   (file-name-nondirectory filename)))
 	  (choices '(?y ?n ?r ?? ?\C-h))
 	  answer)
       (when noninteractive
@@ -206,21 +190,18 @@ ask-user-about-supersession-help
       (with-current-buffer standard-output
         (insert
          (format
-          "You want to modify a buffer whose disk file has changed
+          (substitute-command-keys
+           "You want to modify a buffer whose disk file has changed
 since you last read it in or saved it with this buffer.
 
-If you say %s to go ahead and modify this buffer,
+If you say \\[=y] to go ahead and modify this buffer,
 you risk ruining the work of whoever rewrote the file.
-If you say %s to revert, the contents of the buffer are refreshed
+If you say \\[=r] to revert, the contents of the buffer are refreshed
 from the file on disk.
-If you say %s, the change you started to make will be aborted.
-
-Usually, you should type %s and then %s,
-to get the latest version of the file, then make the change again."
-          (userlock--fontify-key "y")
-          (userlock--fontify-key "r")
-          (userlock--fontify-key "n")
-          (userlock--fontify-key "n")
+If you say \\[=n], the change you started to make will be aborted.
+
+Usually, you should type \\[=n] and then \\[=M-x revert-buffer] (or %s),
+to get the latest version of the file, then make the change again.")
           revert-buffer-binding))
         (help-mode)))))
 
-- 
2.30.1

Re: master a6a5d6a: Move 'revert-buffer' global binding to 'C-x g g'

[Dmitry Gutov]

On 10.03.2021 04:53, Stefan Kangas wrote:
> The help text now says:
> 
>      Usually, you should type ‘n’ and then ‘C-x x g’,
>      to get the latest version of the file, then make the change again.
> 
> The "C-x x g" part used to be "M-x revert-buffer" and therefore more
> self-explanatory.  (This is due to subsitute-command-keys replacing the
> "\\[revert-buffer]" with the new keybinding.)

If the command has a global binding now (which I don't particularly 
like, mind you), shouldn't we advertise it?

To make the users more aware of it.

Re: master a6a5d6a: Move 'revert-buffer' global binding to 'C-x g g'

[Stefan Kangas]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> The "C-x x g" part used to be "M-x revert-buffer" and therefore more
>> self-explanatory.  (This is due to subsitute-command-keys replacing the
>> "\\[revert-buffer]" with the new keybinding.)
>
> If the command has a global binding now (which I don't particularly
> like, mind you), shouldn't we advertise it?

(I was not a fan of the new binding, but that's a moot point as it now
exists.  :-))

> To make the users more aware of it.

Yes, this is reasonable.  I think we arrived at using both the
keybinding and the more didactic "M-x revert-buffer".  But I suppose the
same effect could be had by simply explaining what the key does, so this
might be situational.