ctl-x-map key binding conventions for new major/minor modes

ctl-x-map key binding conventions for new major/minor modes

[Tino Calancha]

(info "(elisp) Key Binding Conventions")

Do we need a recommendation in this node about bindings with
prefix 'C-x'?
There is no guidance in the node above about this topic.

Just loading a new mode shouldn't shadow some previous
C-x bindings, right?

For instance,

* timeclock.el suggests some C-x bindings in the comentary, that is,
   it doesn't define these bindings by default.

* dired-x.el has the user option `dired-bind-jump' to control if
   `dired-jump' must be bound or not to 'C-x C-j'.

* find-func.el has the autoloaded function `find-function-setup-keys',
   which binds some commands into `ctl-x-map'.

Regards,
Tino

RE: ctl-x-map key binding conventions for new major/minor modes

[Drew Adams]

> (info "(elisp) Key Binding Conventions")
> Do we need a recommendation in this node about bindings with
> prefix 'C-x'?  There is no guidance in the node above about
> this topic.  Just loading a new mode shouldn't shadow some
> previous C-x bindings, right?
> 
> For instance,
> * timeclock.el suggests some C-x bindings in the comentary, that is,
>    it doesn't define these bindings by default.
> * dired-x.el has the user option `dired-bind-jump' to control if
>    `dired-jump' must be bound or not to 'C-x C-j'.
> * find-func.el has the autoloaded function `find-function-setup-keys',
>    which binds some commands into `ctl-x-map'.

Are you asking whether code that is distributed with Emacs
should bind keys with prefix `C-x'?  That's usually discussed
and decided case by case, e.g. on this list.

Are you asking whether we should make particular recommendations
in this regard for 3rd-party code?  If so, I think not.

Loading a 3rd-party library is optional.  If a library binds keys
just by loading it then that should at least be made clear in the
library doc/commentary.  A user can always override any bindings
that might be made by a library, but s?he should be aware of what
happens when the library is loaded or a mode is turned on.

Have you noticed an actual problem in this regard?

RE: ctl-x-map key binding conventions for new major/minor modes

[Tino Calancha]

On Fri, 12 May 2017, Drew Adams wrote:

>> (info "(elisp) Key Binding Conventions")
>> Do we need a recommendation in this node about bindings with
>> prefix 'C-x'?  There is no guidance in the node above about
>> this topic.  Just loading a new mode shouldn't shadow some
>> previous C-x bindings, right?
>>
>> For instance,
>> * timeclock.el suggests some C-x bindings in the comentary, that is,
>>    it doesn't define these bindings by default.
>> * dired-x.el has the user option `dired-bind-jump' to control if
>>    `dired-jump' must be bound or not to 'C-x C-j'.
>> * find-func.el has the autoloaded function `find-function-setup-keys',
>>    which binds some commands into `ctl-x-map'.
>
> Are you asking whether code that is distributed with Emacs
> should bind keys with prefix `C-x'?  That's usually discussed
> and decided case by case, e.g. on this list.
>
> Are you asking whether we should make particular recommendations
> in this regard for 3rd-party code?  If so, I think not.
I am asking for both.
For example, magit.el suggests the binding 'C-x g' for `magit-status'.  It
doesn't establish the binding automatically, though.

Imagine one user sets that binding in her .emacs file.  Now she loads
a lib `foo.el' which automatically sets a global binding
'C-x g' to `foo-whatever'.

In the examples above (timeclock, dired-x, etc) the user can easily
control such key binding collisions.  It's something good for people
loading many libs.
We might add a recommendation in that info node about 'C-x ...'
as we do with 'C-c ...'.

> Loading a 3rd-party library is optional.  If a library binds keys
> just by loading it then that should at least be made clear in the
> library doc/commentary.  A user can always override any bindings
> that might be made by a library, but s?he should be aware of what
> happens when the library is loaded or a mode is turned on.
>
> Have you noticed an actual problem in this regard?
No yet.

Re: ctl-x-map key binding conventions for new major/minor modes

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Sat, 13 May 2017 14:33:45 +0900 (JST)
> Cc: Emacs developers <emacs-devel@gnu.org>,
> 	Tino Calancha <tino.calancha@gmail.com>
> 
> Imagine one user sets that binding in her .emacs file.  Now she loads
> a lib `foo.el' which automatically sets a global binding
> 'C-x g' to `foo-whatever'.

Perhaps instead of adding recommendations about this, we could have a
feature where define-key invoked as part as 'load' or 'require' would
check, once, if the key being rebound already has a binding, and ask
the user what she would like to do about, with 3 possible answers
being "rebind", "don't rebind", and "error out of 'load'"?

Re: ctl-x-map key binding conventions for new major/minor modes

[Kaushal Modi]

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

On Sat, May 13, 2017 at 3:11 AM Eli Zaretskii <eliz@gnu.org> wrote:

> Perhaps instead of adding recommendations about this, we could have a
> feature where define-key invoked as part as 'load' or 'require' would
> check, once, if the key being rebound already has a binding,



> and ask
> the user what she would like to do about, with 3 possible answers
> being "rebind", "don't rebind", and "error out of 'load'"?
>

Would that halt emacs startup with that prompt? If so, I believe that would
cause an annoyance for a lot of users who might have already shadowed
bindings.

What if define-key is instead made to throw a message or a warning?

In addition, current signature of define-key is:

    (define-key KEYMAP KEY DEF)

If it is made into

    (define-key KEYMAP KEY DEF &optional ALLOW-OVERRIDE)

- By default, the overriding will happen as now, but the above mentioned
warning will also be put out in *Messages*.
- If ALLOW-OVERRIDE is 'prevent, the overriding will be blocked silently.
- If ALLOW-OVERRIDE is any other non-nil value, overriding will happen and
that warning is not thrown.
-- 

Kaushal Modi

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

Re: ctl-x-map key binding conventions for new major/minor modes

[Eli Zaretskii]

> From: Kaushal Modi <kaushal.modi@gmail.com>
> Date: Sat, 13 May 2017 14:16:46 +0000
> Cc: drew.adams@oracle.com, emacs-devel@gnu.org
> 
>  Perhaps instead of adding recommendations about this, we could have a
>  feature where define-key invoked as part as 'load' or 'require' would
>  check, once, if the key being rebound already has a binding, 
> 
>  and ask
>  the user what she would like to do about, with 3 possible answers
>  being "rebind", "don't rebind", and "error out of 'load'"?
> 
> Would that halt emacs startup with that prompt?

It would stop the load of the features which redefines the keys.

> If so, I believe that would cause an annoyance for a lot of
> users who might have already shadowed bindings.

That's why I proposed to do that only once, the first time this
ever happens for a key or a group of keys.

> What if define-key is instead made to throw a message or a warning?

What's the difference between a message and a warning?

Anyway, this could also be good, but its disadvantage is that it
doesn't let the user an opportunity to reject the rebinding.

> (define-key KEYMAP KEY DEF) 
> 
> If it is made into
> 
> (define-key KEYMAP KEY DEF &optional ALLOW-OVERRIDE)
> 
> - By default, the overriding will happen as now, but the above mentioned warning will also be put out in
> *Messages*.
> - If ALLOW-OVERRIDE is 'prevent, the overriding will be blocked silently.
> - If ALLOW-OVERRIDE is any other non-nil value, overriding will happen and that warning is not thrown. 

I think this should be an interactive feature, not a non-interactive
one.

Re: ctl-x-map key binding conventions for new major/minor modes

[Kaushal Modi]

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

On Sat, May 13, 2017 at 10:34 AM Eli Zaretskii <eliz@gnu.org> wrote:

> >  and ask
> >  the user what she would like to do about, with 3 possible answers
> >  being "rebind", "don't rebind", and "error out of 'load'"?
> >
> > Would that halt emacs startup with that prompt?
>
> It would stop the load of the features which redefines the keys.
>

What I meant is that what if the user is starting emacs using emacsclient
and that prompt interferes with the startup process. It can get very
irritating.

> If so, I believe that would cause an annoyance for a lot of
> > users who might have already shadowed bindings.
>
> That's why I proposed to do that only once, the first time this
> ever happens for a key or a group of keys.
>

So the preference would be saved via Customize interface.


> > What if define-key is instead made to throw a message or a warning?
>
> What's the difference between a message and a warning?
>

I meant just a message.


> Anyway, this could also be good, but its disadvantage is that it
> doesn't let the user an opportunity to reject the rebinding.
>

That right.

I think this should be an interactive feature, not a non-interactive
> one.
>

I thought of above so that the people don't shadow the bindings unknowingly
in their config. I personally never set bindings interactively; may be many
others do the same. So I cannot tell how much impact this will have if this
happens just interactively.
-- 

Kaushal Modi

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

Re: ctl-x-map key binding conventions for new major/minor modes

[Tino Calancha]

On Sat, 13 May 2017, Eli Zaretskii wrote:

>> From: Tino Calancha <tino.calancha@gmail.com>
>> Date: Sat, 13 May 2017 14:33:45 +0900 (JST)
>> Cc: Emacs developers <emacs-devel@gnu.org>,
>> 	Tino Calancha <tino.calancha@gmail.com>
>>
>> Imagine one user sets that binding in her .emacs file.  Now she loads
>> a lib `foo.el' which automatically sets a global binding
>> 'C-x g' to `foo-whatever'.
>
> Perhaps instead of adding recommendations about this, we could have a
> feature where define-key invoked as part as 'load' or 'require' would
> check, once, if the key being rebound already has a binding, and ask
> the user what she would like to do about, with 3 possible answers
> being "rebind", "don't rebind", and "error out of 'load'"?

That feature might be a new minor mode; where the user can customize
what key bindings protect and/or what directories to look at/ignore.

===============================================================================
I) pkb.el:
===============================================================================

--8<-----------------------------cut here---------------start------------->8---
;;; pkb.el --- Prevent unnoticed key rebindings  -*- lexical-binding: t; -*-

;; Copyright (C) 2017  Tino Calancha

;; Author: Tino Calancha <tino.calancha@gmail.com>
;; Version: 0.2
;; Package-Version: 20170517.1359
;; Package-Requires: ((emacs "24.4"))
;; Keywords: lisp, convenience, keyboard

;; This program is free software; you can redistribute it and/or modify
;; it under the terms of the GNU General Public License as published by
;; the Free Software Foundation, either version 3 of the License, or
;; (at your option) any later version.

;; This file is NOT part of GNU Emacs.
;;
;; This program is distributed in the hope that it will be useful,
;; but WITHOUT ANY WARRANTY; without even the implied warranty of
;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
;; GNU General Public License for more details.

;; You should have received a copy of the GNU General Public License
;; along with this program.  If not, see <http://www.gnu.org/licenses/>.

;;; Commentary:
;;
;; This file prevents from inadvertently changes on key bindings
;; during the load of an elisp library.
;;
;; The file defines a new minor mode `pkb-protect-key-bindings'.
;; When the mode is enabled and a library is loaded, ask what
;; to do if a call to `define-key' rebinds a previous key.
;; Possible actions are:
;; y: Rebind it.
;; n: Skip it.
;; Y or !: Rebind all and not prompt me more.
;; N: Skip all and not prompt me more.
;; The latter two actions are permanent.  After them, you must toggle
;; OFF/ON the mode if you want to be prompted again when loading
;; a new library.
;;
;; By default, the protected keys are those reserved for users.  You can
;; add more protected keys by customizing `pkb-protected-prefices'.
;;

;;; Code:



(require 'cl-lib)

(defgroup pkb nil
   "Prevent unnoticed key rebindings."
   :prefix "pkb-"
   :group 'keyboard)

(defcustom pkb-protect-global-bindings-only t
   "If nil, then protect local key bindings as well."
   :type 'boolean
   :group 'protect-key-bindings
   :version "26.1")

;; Protect keys reserved for users. You might also want "C-x" and "M-".
(defcustom pkb-protected-prefices
   '("\\`C-c [a-zA-Z]\\'" "\\`<f5>\\'" "\\`<f6>\\'" "\\`<f7>\\'"
     "\\`<f8>\\'" "\\`<f9>\\'")
   "List of protected key bindings when loading a library."
   :type 'list
   :group 'protect-key-bindings
   :version "26.1")

(defcustom pkb-dirs-to-ignore nil
   "List of directories to ignore for key rebindings."
   :type '(choice
           (const :tag "Unset" nil)
           (repeat :tag "Directories to ignore" string))
   :group 'protect-key-bindings
   :version "26.1")

(defcustom pkb-dirs-to-check nil
   "List of directories to check for key rebindings."
   :type '(choice
           (const :tag "Unset" nil)
           (repeat :tag "Directories to check" string))
   :group 'protect-key-bindings
   :version "26.1")

(defcustom pkb-protect-action 'ask
   "What to do when the load of a lib changes a protected key binding."
   :type '(choice
           (const :tag "Unset" nil)
           (const :tag "Ask" ask)
           (const :tag "Accept all" accept)
           (const :tag "Cancel all" cancel)
           (const :tag "Signal Error" signal-error))
   :group 'keyboard
   :version "26.1")



(defun pkb--current-def (keymap key)
   (let ((special-global-maps (list ctl-x-map ctl-x-4-map ctl-x-5-map esc-map)))
     (if (memq keymap special-global-maps)
         (lookup-key keymap key)
       (or (global-key-binding key)
           (and (null pkb-protect-global-bindings-only)
                (lookup-key keymap key))))))

(defun pkb--read-input (key-descr keymap)
   (let* ((special-global-maps (list ctl-x-map ctl-x-4-map ctl-x-5-map esc-map))
          (id-prefix '((0 . "C-x ") (1 . "C-x 4 ") (2 . "C-x 5 ") (3 . "M-")))
          (lib (file-name-nondirectory load-file-name))
          (prompt
           (format
            "[%s] Key '%s%s' already bound.  Rebind? [ynY!N] "
            lib
            (if (memq keymap special-global-maps)
                (cdr (assq (cl-position keymap special-global-maps) id-prefix))
              "")
            key-descr))
          (res (string (read-char prompt)))
          (valid-answers '("y" "n" "Y" "!" "N")))
     (if (member res valid-answers)
         res
       (while (not (member res valid-answers))
         (ding)
         (let ((prompt (concat prompt "Valid answers are chars in [ynY!N] ")))
           (setq res (string (read-char prompt))))) res)))

(defvar pkb--rebind-all nil)
(defvar pkb--cancel-all-rebindings nil)
(defun pkb-define-key--check-rebindings (fn keymap key def &rest args)
   "What to do when the load of a lib changes a protected key binding."
   (let* ((key-descr (key-description key))
          (current-def (pkb--current-def keymap key))
          (auto (or pkb--rebind-all pkb--cancel-all-rebindings))
          (rebind-all (or (eq pkb-protect-action 'accept)
                          (and auto pkb--rebind-all)))
          (cancel-all (or (eq pkb-protect-action 'cancel)
                          (and auto pkb--cancel-all-rebindings)))
          (in-dirs-ignore-p
           (and pkb-dirs-to-ignore
                load-file-name
                (file-in-directory-p
                 (file-name-directory load-file-name)
                 pkb-dirs-to-ignore)))
          (not-in-dirs-check-p
           (and pkb-dirs-to-check
                load-file-name
                (not (file-in-directory-p
                      (file-name-directory load-file-name)
                      pkb-dirs-to-check)))))
     (cond ((and load-in-progress
                 (not in-dirs-ignore-p)
                 (not not-in-dirs-check-p)
                 current-def
                 (and (functionp current-def)
                      (not (eq current-def 'self-insert-command)))
                 (not (equal current-def def)))
            (let (input)
              (if (cl-notany
                   (lambda (prefix)
                     (unless (string-suffix-p " " prefix)
                       (setq prefix (concat prefix " ")))
                     (cond ((and (equal prefix "C-x ") (eq keymap ctl-x-map)) t)
                           ((and (equal prefix "C-x 4 ")
                                 (eq keymap ctl-x-4-map)) t)
                           ((and (equal prefix "C-x 5 ")
                                 (eq keymap ctl-x-5-map)) t)
                           ((and (equal prefix "M- ") (eq keymap esc-map)) t)
                           (t
                            (string-match (substring prefix 0 -1) key-descr))))
                   pkb-protected-prefices)
                  (apply fn keymap key def args)
                (cond ((eq pkb-protect-action 'signal-error)
                       (error "Key '%s' already bound" key-descr))
                      (rebind-all
                       (message "Rebinding key '%s'" key-descr)
                       (apply fn keymap key def args))
                      (cancel-all
                       (message "Prevent rebinding of key '%s'" key-descr))
                      (t
                       (setq input (pkb--read-input key-descr keymap))
                       (pcase input
                         ("y" (apply fn keymap key def args))
                         ((or "Y" "!")
                          (message "Accepting all rebindings")
                          (setq pkb--rebind-all t)
                          (apply fn keymap key def args))
                         ("N"
                          (setq pkb--cancel-all-rebindings t)
                          (message "OK, cancel '%s' rebinding and others"
                                   key-descr))
                         ("n" (message "OK, cancel '%s' rebinding"
                                       key-descr))))))))
           (t (apply fn keymap key def args)))))

(define-minor-mode pkb-protect-key-bindings
   "Toggle pkb mode.
With a prefix argument ARG, enable the mode if ARG is positive,
and disable it otherwise.  If called from Lisp, enable
the mode if ARG is omitted or nil, and toggle it if ARG is `toggle'.

Prevents from inadvertently changes on key bindings during
the load of an elisp library.
When the mode is enabled and a library is loaded, ask what
to do if a call to `define-key' rebinds a protected key.
Possible actions are:
y: Rebind it.
n: Skip it.
Y or !: Rebind all and not prompt me more.
N: Skip all and not prompt me more.
The latter two actions are permanent.  After them, you must toggle
OFF/ON the mode if you want to be prompted again when loading
a new library.
By default, the protected keys are those reserved for users.  You can
add more protected keys by customizing `pkb-protected-prefices'."
   :init-value nil
   :global t
   :lighter (:eval (if pkb-protect-key-bindings " Pkb" ""))
   :keymap nil
   (if pkb-protect-key-bindings
       (advice-add 'define-key :around 'pkb-define-key--check-rebindings)
     (advice-remove 'define-key 'pkb-define-key--check-rebindings)
     (setq pkb--rebind-all nil
           pkb--cancel-all-rebindings nil)))


(provide 'pkb)
;;; pkb.el ends here
--8<-----------------------------cut here---------------end--------------->8---

===============================================================================
II) Tests for pkb.el:
===============================================================================

--8<-----------------------------cut here---------------start------------->8---
;;; pkb-tests.el --- Test suite for pkb. -*- lexical-binding: t -*-

;; Copyright (C) 2017 Free Software Foundation, Inc.

;; This file is part of GNU Emacs.

;; GNU Emacs is free software: you can redistribute it and/or modify
;; it under the terms of the GNU General Public License as published by
;; the Free Software Foundation, either version 3 of the License, or
;; (at your option) any later version.

;; GNU Emacs is distributed in the hope that it will be useful,
;; but WITHOUT ANY WARRANTY; without even the implied warranty of
;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
;; GNU General Public License for more details.

;; You should have received a copy of the GNU General Public License
;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.

;;; Code:
(require 'ert)
(require 'pkb)


(ert-deftest pkb-test ()
   "Test for pkb mode."
   (let* ((dir (make-temp-file "pkb" 'dir))
          (default-directory dir)
          (foo (expand-file-name "foo.el" dir))
          (bar (expand-file-name "bar.el" dir))
          (qux (expand-file-name "qux.el" dir))
          ;; Restore these bindings on exit.
          (binding-cca (global-key-binding (kbd "C-c a")))
          (binding-cxf (global-key-binding (kbd "C-x f"))))
     (pkb-protect-key-bindings 1)
     (with-temp-buffer
       (insert "(global-set-key (kbd \"C-c a\") (lambda () (interactive) 'foo))\n")
       (insert "(global-set-key (kbd \"C-x f\") (lambda () (interactive) 'foo))")
       (write-region nil nil foo)
       (erase-buffer)
       (insert "(global-set-key (kbd \"C-c a\") (lambda () (interactive) 'bar))")
       (write-region nil nil bar)
       (erase-buffer)
       (insert "(global-set-key (kbd \"C-x f\") (lambda () (interactive) 'qux))")
       (write-region nil nil qux))
     (unwind-protect
         (progn
           (let ((pkb-protect-action 'signal-error))
             (global-unset-key (kbd "C-c a"))
             (should (load foo))
             (should (load foo)) ; Again.
             (should-error (load bar)) ; 'C-c a' bound to a different lambda.
             (should (load qux)) ; Ignore "C-x".
             (let ((pkb-protected-prefices (append pkb-protected-prefices (list "C-x"))))
               (should-error (load foo))))
           (let ((pkb--cancel-all-rebindings t))
             (should (eq 'foo (funcall (global-key-binding (kbd "C-c a")))))
             (should (load bar))
             (should (eq 'foo (funcall (global-key-binding (kbd "C-c a"))))))
           (let ((pkb--rebind-all t))
             (should (eq 'foo (funcall (global-key-binding (kbd "C-c a")))))
             (should (load bar))
             (should (eq 'bar (funcall (global-key-binding (kbd "C-c a")))))))
       (delete-directory dir 'recursive)
       (when binding-cca
         (global-set-key (kbd "C-c a") binding-cca))
       (when binding-cxf
         (global-set-key (kbd "C-c a") binding-cxf)))))

(provide 'pkb-tests)
;; pkb-tests.el ends here
--8<-----------------------------cut here---------------end--------------->8---