[PATCH v3 0/7] emacs: help: remap keybindings

[PATCH v3 0/7] emacs: help: remap keybindings

[Mark Walters]

This is version 3 of the series. v2 is at
id:1383870096-14627-1-git-send-email-markwalters1009@gmail.com

The changes are in response to Austin's review of v2. I have not
changed notmuch-describe-key.

The other change is to add a base-keymap to look up the remapped
commands in (since notmuch-substitute-command-keys could refer to
something other than the current keymap). Currently this never happens
but this makes the code more robust.

The final patch of the series is (probably) just for testing
purposes. It lets you call notmuch-help and specify which mode you
want to display help for. This means you can call it without having
the mode as current-mode and hence test the base-keymap code.

Finally the diff from v2 below (with the testing patch omitted) is below.

Best wishes

Mark

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index ef616d5..4b3a86e 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -261,7 +261,7 @@ It does not prepend if ACTUAL-KEY is already listed in TAIL."
 	    tail)))
     tail)
 
-(defun notmuch-describe-remaps (remap-keymap ua-keys prefix tail)
+(defun notmuch-describe-remaps (remap-keymap ua-keys base-keymap prefix tail)
   ;; Remappings are represented as a binding whose first "event" is
   ;; 'remap.  Hence, if the keymap has any remappings, it will have a
   ;; binding whose "key" is 'remap, and whose "binding" is itself a
@@ -272,11 +272,11 @@ It does not prepend if ACTUAL-KEY is already listed in TAIL."
      (mapc
       (lambda (actual-key)
 	(setq tail (notmuch-describe-key actual-key binding prefix ua-keys tail)))
-      (where-is-internal command)))
+      (where-is-internal command base-keymap)))
    remap-keymap)
   tail)
 
-(defun notmuch-describe-keymap (keymap ua-keys &optional prefix tail)
+(defun notmuch-describe-keymap (keymap ua-keys base-keymap &optional prefix tail)
   "Return a list of cons cells, each describing one binding in KEYMAP.
 
 Each cons cell consists of a string giving a human-readable
@@ -291,13 +291,12 @@ prefix argument.  PREFIX and TAIL are used internally."
    (lambda (key binding)
      (cond ((mouse-event-p key) nil)
 	   ((keymapp binding)
-	    (if (eq key 'remap)
-		(setq tail
+	    (setq tail
+		  (if (eq key 'remap)
 		      (notmuch-describe-remaps
-		       binding ua-keys prefix tail))
-	      (setq tail
+		       binding ua-keys base-keymap prefix tail)
 		    (notmuch-describe-keymap
-		     binding ua-keys (notmuch-prefix-key-description key) tail))))
+		     binding ua-keys base-keymap (notmuch-prefix-key-description key) tail))))
 	   (binding
 	    (setq tail (notmuch-describe-key (vector key) binding prefix ua-keys tail)))))
    keymap)
@@ -310,7 +309,7 @@ prefix argument.  PREFIX and TAIL are used internally."
       (let* ((keymap-name (substring doc (match-beginning 1) (match-end 1)))
 	     (keymap (symbol-value (intern keymap-name)))
 	     (ua-keys (where-is-internal 'universal-argument keymap t))
-	     (desc-alist (notmuch-describe-keymap keymap ua-keys))
+	     (desc-alist (notmuch-describe-keymap keymap ua-keys keymap))
 	     (desc-list (mapcar (lambda (arg) (concat (car arg) "\t" (cdr arg))) desc-alist))
 	     (desc (mapconcat #'identity desc-list "\n")))
 	(setq doc (replace-match desc 1 1 doc)))



Mark Walters (7):
  emacs: help: check for nil key binding
  emacs: help: remove duplicate bindings
  emacs: help: split out notmuch-describe-key as a function
  emacs: help: add base-keymap
  emacs: help: add a special function to deal with remaps
  emacs: tree: use remap for the over-ridden global bindings
  emacs: help: base-keymap-test-help

 emacs/notmuch-lib.el  |   79 ++++++++++++++++++++++++++++++++++--------------
 emacs/notmuch-tree.el |    8 ++--
 2 files changed, 60 insertions(+), 27 deletions(-)




-- 
1.7.9.1

[PATCH v3 1/7] emacs: help: check for nil key binding

[Mark Walters]

A standard way to unset a key binding is local-unset-key which is equivalent to
  (define-key (current-local-map) key nil)

Currently notmuch-help gives an error and fails if a user has done this.

To fix this we only add a help line if the binding is non-nil.
---
 emacs/notmuch-lib.el |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 921ed20..ec5a2cb 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -254,7 +254,7 @@ prefix argument.  PREFIX and TAIL are used internally."
 	    (setq tail
 		  (notmuch-describe-keymap
 		   binding ua-keys (notmuch-prefix-key-description key) tail)))
-	   (t
+	   (binding
 	    (when (and ua-keys (symbolp binding)
 		       (get binding 'notmuch-prefix-doc))
 	      ;; Documentation for prefixed command
-- 
1.7.9.1

[PATCH v3 2/7] emacs: help: remove duplicate bindings

[Mark Walters]

If the user (or a mode) overrides a keybinding from the common keymap
in one of the modes then both help lines appear in the help screen
even though only one of them is applicable.

Fix this by checking if we already have that key binding. We do this
by constructing an list of (key . docstring) pairs so it is easy to
check if we have already had that binding. Then the actual print help
routine changes these pairs into strings "key \t docstring"
---
 emacs/notmuch-lib.el |   41 ++++++++++++++++++++++++-----------------
 1 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index ec5a2cb..2195166 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -238,11 +238,12 @@ This is basically just `format-kbd-macro' but we also convert ESC to M-."
       (concat desc " "))))
 
 (defun notmuch-describe-keymap (keymap ua-keys &optional prefix tail)
-  "Return a list of strings, each describing one binding in KEYMAP.
+  "Return a list of cons cells, each describing one binding in KEYMAP.
 
-Each string gives a human-readable description of the key and a
-one-line description of the bound function.  See `notmuch-help'
-for an overview of how this documentation is extracted.
+Each cons cell consists of a string giving a human-readable
+description of the key, and a one-line description of the bound
+function.  See `notmuch-help' for an overview of how this
+documentation is extracted.
 
 UA-KEYS should be a key sequence bound to `universal-argument'.
 It will be used to describe bindings of commands that support a
@@ -255,18 +256,23 @@ prefix argument.  PREFIX and TAIL are used internally."
 		  (notmuch-describe-keymap
 		   binding ua-keys (notmuch-prefix-key-description key) tail)))
 	   (binding
-	    (when (and ua-keys (symbolp binding)
-		       (get binding 'notmuch-prefix-doc))
-	      ;; Documentation for prefixed command
-	      (let ((ua-desc (key-description ua-keys)))
-		(push (concat ua-desc " " prefix (format-kbd-macro (vector key))
-			      "\t" (get binding 'notmuch-prefix-doc))
-		      tail)))
-	    ;; Documentation for command
-	    (push (concat prefix (format-kbd-macro (vector key)) "\t"
-			  (or (and (symbolp binding) (get binding 'notmuch-doc))
-			      (notmuch-documentation-first-line binding)))
-		  tail))))
+	    (let ((key-string (concat prefix (format-kbd-macro (vector key)))))
+	      ;; We don't include documentation if the key-binding is
+	      ;; over-ridden. Note, over-riding a binding
+	      ;; automatically hides the prefixed version too.
+	      (unless (assoc key-string tail)
+		(when (and ua-keys (symbolp binding)
+			   (get binding 'notmuch-prefix-doc))
+		  ;; Documentation for prefixed command
+		  (let ((ua-desc (key-description ua-keys)))
+		    (push (cons (concat ua-desc " " prefix (format-kbd-macro (vector key)))
+				(get binding 'notmuch-prefix-doc))
+			  tail)))
+		;; Documentation for command
+		(push (cons key-string
+			    (or (and (symbolp binding) (get binding 'notmuch-doc))
+				(notmuch-documentation-first-line binding)))
+		      tail))))))
    keymap)
   tail)
 
@@ -277,7 +283,8 @@ prefix argument.  PREFIX and TAIL are used internally."
       (let* ((keymap-name (substring doc (match-beginning 1) (match-end 1)))
 	     (keymap (symbol-value (intern keymap-name)))
 	     (ua-keys (where-is-internal 'universal-argument keymap t))
-	     (desc-list (notmuch-describe-keymap keymap ua-keys))
+	     (desc-alist (notmuch-describe-keymap keymap ua-keys))
+	     (desc-list (mapcar (lambda (arg) (concat (car arg) "\t" (cdr arg))) desc-alist))
 	     (desc (mapconcat #'identity desc-list "\n")))
 	(setq doc (replace-match desc 1 1 doc)))
       (setq beg (match-end 0)))
-- 
1.7.9.1

[PATCH v3 3/7] emacs: help: split out notmuch-describe-key as a function

[Mark Walters]

The actual documentation function notmuch-describe-keymap was getting
rather complicated so split out the code for a single key into its own
function notmuch-describe-key.
---
 emacs/notmuch-lib.el |   42 +++++++++++++++++++++++++-----------------
 1 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 2195166..8852703 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -237,6 +237,30 @@ This is basically just `format-kbd-macro' but we also convert ESC to M-."
 	"M-"
       (concat desc " "))))
 
+
+(defun notmuch-describe-key (actual-key binding prefix ua-keys tail)
+  "Prepend cons cells describing prefix-arg ACTUAL-KEY and ACTUAL-KEY to TAIL
+
+It does not prepend if ACTUAL-KEY is already listed in TAIL."
+  (let ((key-string (concat prefix (format-kbd-macro actual-key))))
+    ;; We don't include documentation if the key-binding is
+    ;; over-ridden. Note, over-riding a binding automatically hides the
+    ;; prefixed version too.
+    (unless (assoc key-string tail)
+      (when (and ua-keys (symbolp binding)
+		 (get binding 'notmuch-prefix-doc))
+	;; Documentation for prefixed command
+	(let ((ua-desc (key-description ua-keys)))
+	  (push (cons (concat ua-desc " " prefix (format-kbd-macro actual-key))
+		      (get binding 'notmuch-prefix-doc))
+		tail)))
+      ;; Documentation for command
+      (push (cons key-string
+		  (or (and (symbolp binding) (get binding 'notmuch-doc))
+		      (notmuch-documentation-first-line binding)))
+	    tail)))
+    tail)
+
 (defun notmuch-describe-keymap (keymap ua-keys &optional prefix tail)
   "Return a list of cons cells, each describing one binding in KEYMAP.
 
@@ -256,23 +280,7 @@ prefix argument.  PREFIX and TAIL are used internally."
 		  (notmuch-describe-keymap
 		   binding ua-keys (notmuch-prefix-key-description key) tail)))
 	   (binding
-	    (let ((key-string (concat prefix (format-kbd-macro (vector key)))))
-	      ;; We don't include documentation if the key-binding is
-	      ;; over-ridden. Note, over-riding a binding
-	      ;; automatically hides the prefixed version too.
-	      (unless (assoc key-string tail)
-		(when (and ua-keys (symbolp binding)
-			   (get binding 'notmuch-prefix-doc))
-		  ;; Documentation for prefixed command
-		  (let ((ua-desc (key-description ua-keys)))
-		    (push (cons (concat ua-desc " " prefix (format-kbd-macro (vector key)))
-				(get binding 'notmuch-prefix-doc))
-			  tail)))
-		;; Documentation for command
-		(push (cons key-string
-			    (or (and (symbolp binding) (get binding 'notmuch-doc))
-				(notmuch-documentation-first-line binding)))
-		      tail))))))
+	    (setq tail (notmuch-describe-key (vector key) binding prefix ua-keys tail)))))
    keymap)
   tail)
 
-- 
1.7.9.1

[PATCH v3 4/7] emacs: help: add base-keymap

[Mark Walters]

To support key remapping in emacs help we need to know the base keymap
when looking at the remapping. keep track of this while we recurse
down the sub-keymaps in help.
---
 emacs/notmuch-lib.el |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 8852703..a4f481b 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -261,7 +261,7 @@ It does not prepend if ACTUAL-KEY is already listed in TAIL."
 	    tail)))
     tail)
 
-(defun notmuch-describe-keymap (keymap ua-keys &optional prefix tail)
+(defun notmuch-describe-keymap (keymap ua-keys base-keymap &optional prefix tail)
   "Return a list of cons cells, each describing one binding in KEYMAP.
 
 Each cons cell consists of a string giving a human-readable
@@ -278,7 +278,7 @@ prefix argument.  PREFIX and TAIL are used internally."
 	   ((keymapp binding)
 	    (setq tail
 		  (notmuch-describe-keymap
-		   binding ua-keys (notmuch-prefix-key-description key) tail)))
+		   binding ua-keys base-keymap (notmuch-prefix-key-description key) tail)))
 	   (binding
 	    (setq tail (notmuch-describe-key (vector key) binding prefix ua-keys tail)))))
    keymap)
@@ -291,7 +291,7 @@ prefix argument.  PREFIX and TAIL are used internally."
       (let* ((keymap-name (substring doc (match-beginning 1) (match-end 1)))
 	     (keymap (symbol-value (intern keymap-name)))
 	     (ua-keys (where-is-internal 'universal-argument keymap t))
-	     (desc-alist (notmuch-describe-keymap keymap ua-keys))
+	     (desc-alist (notmuch-describe-keymap keymap ua-keys keymap))
 	     (desc-list (mapcar (lambda (arg) (concat (car arg) "\t" (cdr arg))) desc-alist))
 	     (desc (mapconcat #'identity desc-list "\n")))
 	(setq doc (replace-match desc 1 1 doc)))
-- 
1.7.9.1

[PATCH v3 5/7] emacs: help: add a special function to deal with remaps

[Mark Walters]

remaps are a rather unusual keymap consisting of "first key" 'remap
and then "second-key" the remapped-function. Thus we do the
documentation for it separately.
---
 emacs/notmuch-lib.el |   22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index a4f481b..4b3a86e 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -261,6 +261,21 @@ It does not prepend if ACTUAL-KEY is already listed in TAIL."
 	    tail)))
     tail)
 
+(defun notmuch-describe-remaps (remap-keymap ua-keys base-keymap prefix tail)
+  ;; Remappings are represented as a binding whose first "event" is
+  ;; 'remap.  Hence, if the keymap has any remappings, it will have a
+  ;; binding whose "key" is 'remap, and whose "binding" is itself a
+  ;; keymap that maps not from keys to commands, but from old (remapped)
+  ;; functions to the commands to use in their stead.
+  (map-keymap
+   (lambda (command binding)
+     (mapc
+      (lambda (actual-key)
+	(setq tail (notmuch-describe-key actual-key binding prefix ua-keys tail)))
+      (where-is-internal command base-keymap)))
+   remap-keymap)
+  tail)
+
 (defun notmuch-describe-keymap (keymap ua-keys base-keymap &optional prefix tail)
   "Return a list of cons cells, each describing one binding in KEYMAP.
 
@@ -277,8 +292,11 @@ prefix argument.  PREFIX and TAIL are used internally."
      (cond ((mouse-event-p key) nil)
 	   ((keymapp binding)
 	    (setq tail
-		  (notmuch-describe-keymap
-		   binding ua-keys base-keymap (notmuch-prefix-key-description key) tail)))
+		  (if (eq key 'remap)
+		      (notmuch-describe-remaps
+		       binding ua-keys base-keymap prefix tail)
+		    (notmuch-describe-keymap
+		     binding ua-keys base-keymap (notmuch-prefix-key-description key) tail))))
 	   (binding
 	    (setq tail (notmuch-describe-key (vector key) binding prefix ua-keys tail)))))
    keymap)
-- 
1.7.9.1

[PATCH v3 6/7] emacs: tree: use remap for the over-ridden global bindings

[Mark Walters]

Following a suggestion by Austin in id:20130915153642.GY1426@mit.edu
we use remap for the over-riding bindings in pick. This means that if
the user modifies the global keymap these modifications will happen in
the tree-view versions of them too.

[tree-view overrides these to do things like close the message pane
before doing the action, so the functionality is very close to the
original common keymap function.]
---
 emacs/notmuch-tree.el |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/emacs/notmuch-tree.el b/emacs/notmuch-tree.el
index f13b41f..8d59e65 100644
--- a/emacs/notmuch-tree.el
+++ b/emacs/notmuch-tree.el
@@ -220,13 +220,13 @@ FUNC."
     (set-keymap-parent map notmuch-common-keymap)
     ;; The following override the global keymap.
     ;; Override because we want to close message pane first.
-    (define-key map "?" (notmuch-tree-close-message-pane-and #'notmuch-help))
+    (define-key map [remap notmuch-help] (notmuch-tree-close-message-pane-and #'notmuch-help))
     ;; Override because we first close message pane and then close tree buffer.
-    (define-key map "q" 'notmuch-tree-quit)
+    (define-key map [remap notmuch-kill-this-buffer] 'notmuch-tree-quit)
     ;; Override because we close message pane after the search query is entered.
-    (define-key map "s" 'notmuch-tree-to-search)
+    (define-key map [remap notmuch-search] 'notmuch-tree-to-search)
     ;; Override because we want to close message pane first.
-    (define-key map "m" (notmuch-tree-close-message-pane-and #'notmuch-mua-new-mail))
+    (define-key map [remap notmuch-mua-new-mail] (notmuch-tree-close-message-pane-and #'notmuch-mua-new-mail))
 
     ;; these use notmuch-show functions directly
     (define-key map "|" 'notmuch-show-pipe-message)
-- 
1.7.9.1

[PATCH v3 7/7] emacs: help: base-keymap-test-help

[Mark Walters]

Add an argument to notmuch-help for the mode to display help for.
This aids testing of the base-keymap case in remapping in emacs help.

It is only intended for testing, ie not for master (but it does no harm)
---
 emacs/notmuch-lib.el |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 4b3a86e..7b8acb3 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -316,7 +316,7 @@ prefix argument.  PREFIX and TAIL are used internally."
       (setq beg (match-end 0)))
     doc))
 
-(defun notmuch-help ()
+(defun notmuch-help (&optional mode)
   "Display help for the current notmuch mode.
 
 This is similar to `describe-function' for the current major
@@ -328,7 +328,7 @@ A command that supports a prefix argument can explicitly document
 its prefixed behavior by setting the 'notmuch-prefix-doc property
 of its command symbol."
   (interactive)
-  (let* ((mode major-mode)
+  (let* ((mode (or mode major-mode))
 	 (doc (substitute-command-keys (notmuch-substitute-command-keys (documentation mode t)))))
     (with-current-buffer (generate-new-buffer "*notmuch-help*")
       (insert doc)
-- 
1.7.9.1

Re: [PATCH v3 0/7] emacs: help: remap keybindings

[Austin Clements]

LGTM.

Quoth Mark Walters on Nov 08 at  5:40 pm:
> This is version 3 of the series. v2 is at
> id:1383870096-14627-1-git-send-email-markwalters1009@gmail.com
> 
> The changes are in response to Austin's review of v2. I have not
> changed notmuch-describe-key.
> 
> The other change is to add a base-keymap to look up the remapped
> commands in (since notmuch-substitute-command-keys could refer to
> something other than the current keymap). Currently this never happens
> but this makes the code more robust.
> 
> The final patch of the series is (probably) just for testing
> purposes. It lets you call notmuch-help and specify which mode you
> want to display help for. This means you can call it without having
> the mode as current-mode and hence test the base-keymap code.
> 
> Finally the diff from v2 below (with the testing patch omitted) is below.
> 
> Best wishes
> 
> Mark
> 
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index ef616d5..4b3a86e 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -261,7 +261,7 @@ It does not prepend if ACTUAL-KEY is already listed in TAIL."
>  	    tail)))
>      tail)
>  
> -(defun notmuch-describe-remaps (remap-keymap ua-keys prefix tail)
> +(defun notmuch-describe-remaps (remap-keymap ua-keys base-keymap prefix tail)
>    ;; Remappings are represented as a binding whose first "event" is
>    ;; 'remap.  Hence, if the keymap has any remappings, it will have a
>    ;; binding whose "key" is 'remap, and whose "binding" is itself a
> @@ -272,11 +272,11 @@ It does not prepend if ACTUAL-KEY is already listed in TAIL."
>       (mapc
>        (lambda (actual-key)
>  	(setq tail (notmuch-describe-key actual-key binding prefix ua-keys tail)))
> -      (where-is-internal command)))
> +      (where-is-internal command base-keymap)))
>     remap-keymap)
>    tail)
>  
> -(defun notmuch-describe-keymap (keymap ua-keys &optional prefix tail)
> +(defun notmuch-describe-keymap (keymap ua-keys base-keymap &optional prefix tail)
>    "Return a list of cons cells, each describing one binding in KEYMAP.
>  
>  Each cons cell consists of a string giving a human-readable
> @@ -291,13 +291,12 @@ prefix argument.  PREFIX and TAIL are used internally."
>     (lambda (key binding)
>       (cond ((mouse-event-p key) nil)
>  	   ((keymapp binding)
> -	    (if (eq key 'remap)
> -		(setq tail
> +	    (setq tail
> +		  (if (eq key 'remap)
>  		      (notmuch-describe-remaps
> -		       binding ua-keys prefix tail))
> -	      (setq tail
> +		       binding ua-keys base-keymap prefix tail)
>  		    (notmuch-describe-keymap
> -		     binding ua-keys (notmuch-prefix-key-description key) tail))))
> +		     binding ua-keys base-keymap (notmuch-prefix-key-description key) tail))))
>  	   (binding
>  	    (setq tail (notmuch-describe-key (vector key) binding prefix ua-keys tail)))))
>     keymap)
> @@ -310,7 +309,7 @@ prefix argument.  PREFIX and TAIL are used internally."
>        (let* ((keymap-name (substring doc (match-beginning 1) (match-end 1)))
>  	     (keymap (symbol-value (intern keymap-name)))
>  	     (ua-keys (where-is-internal 'universal-argument keymap t))
> -	     (desc-alist (notmuch-describe-keymap keymap ua-keys))
> +	     (desc-alist (notmuch-describe-keymap keymap ua-keys keymap))
>  	     (desc-list (mapcar (lambda (arg) (concat (car arg) "\t" (cdr arg))) desc-alist))
>  	     (desc (mapconcat #'identity desc-list "\n")))
>  	(setq doc (replace-match desc 1 1 doc)))
> 
> 
> 
> Mark Walters (7):
>   emacs: help: check for nil key binding
>   emacs: help: remove duplicate bindings
>   emacs: help: split out notmuch-describe-key as a function
>   emacs: help: add base-keymap
>   emacs: help: add a special function to deal with remaps
>   emacs: tree: use remap for the over-ridden global bindings
>   emacs: help: base-keymap-test-help
> 
>  emacs/notmuch-lib.el  |   79 ++++++++++++++++++++++++++++++++++--------------
>  emacs/notmuch-tree.el |    8 ++--
>  2 files changed, 60 insertions(+), 27 deletions(-)

[PATCH] emacs: help: bugfix

[Mark Walters]

Hi

David found a bug in the this remap/help series. He has a global
keybinding of "C-c s" for notmuch-search and this causes help in
tree-mode to hang.

I have mostly diagnosed this: the problem comes that all the construct
help routines are inside a string-match/replace-match pair. Somewhere
in these routines the match-data is being stomped on (but I have to
admit I am not sure where).

In any case putting the construct help routines inside a
save-match-data seems to fix it.

This version is a bit ugly: I am not sure of the best way to deal with
the save-match-data macro. (I think it is best to have it round
everything that happens between finding the match and replacing the
match to avoid anything similar in future).

This applies on top of the parent series.

Any comments gratefully received!

Best wishes

Mark

---
 emacs/notmuch-lib.el |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 7b8acb3..e98e073 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -305,13 +305,16 @@ prefix argument.  PREFIX and TAIL are used internally."
 (defun notmuch-substitute-command-keys (doc)
   "Like `substitute-command-keys' but with documentation, not function names."
   (let ((beg 0))
-    (while (string-match "\\\\{\\([^}[:space:]]*\\)}" doc beg)
-      (let* ((keymap-name (substring doc (match-beginning 1) (match-end 1)))
-	     (keymap (symbol-value (intern keymap-name)))
-	     (ua-keys (where-is-internal 'universal-argument keymap t))
-	     (desc-alist (notmuch-describe-keymap keymap ua-keys keymap))
-	     (desc-list (mapcar (lambda (arg) (concat (car arg) "\t" (cdr arg))) desc-alist))
-	     (desc (mapconcat #'identity desc-list "\n")))
+    (while (string-match "\\\\{\\([^}[:space:]]*\\)}" doc beg) ;; matches \{not-space}
+      (let ((desc
+	     (save-match-data
+	       (let* ((keymap-name (substring doc (match-beginning 1) (match-end 1)))
+		      (keymap (symbol-value (intern keymap-name)))
+		      (ua-keys (where-is-internal 'universal-argument keymap t))
+		      (desc-alist (notmuch-describe-keymap keymap ua-keys keymap))
+		      (desc-list (mapcar (lambda (arg) (concat (car arg) "\t" (cdr arg))) desc-alist))
+		      (desc (mapconcat #'identity desc-list "\n")))
+		 desc))))
 	(setq doc (replace-match desc 1 1 doc)))
       (setq beg (match-end 0)))
     doc))
-- 
1.7.9.1

Re: [PATCH] emacs: help: bugfix

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:
>
> This version is a bit ugly: I am not sure of the best way to deal with
> the save-match-data macro. (I think it is best to have it round
> everything that happens between finding the match and replacing the
> match to avoid anything similar in future).
>
> This applies on top of the parent series.

This seems to fix the problem for me.

d

Re: [PATCH] emacs: help: bugfix

[Austin Clements]

On Sun, 10 Nov 2013, Mark Walters <markwalters1009@gmail.com> wrote:
> Hi
>
> David found a bug in the this remap/help series. He has a global
> keybinding of "C-c s" for notmuch-search and this causes help in
> tree-mode to hang.
>
> I have mostly diagnosed this: the problem comes that all the construct
> help routines are inside a string-match/replace-match pair. Somewhere
> in these routines the match-data is being stomped on (but I have to
> admit I am not sure where).
>
> In any case putting the construct help routines inside a
> save-match-data seems to fix it.
>
> This version is a bit ugly: I am not sure of the best way to deal with
> the save-match-data macro. (I think it is best to have it round
> everything that happens between finding the match and replacing the
> match to avoid anything similar in future).
>
> This applies on top of the parent series.
>
> Any comments gratefully received!
>
> Best wishes
>
> Mark
>
> ---
>  emacs/notmuch-lib.el |   17 ++++++++++-------
>  1 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index 7b8acb3..e98e073 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -305,13 +305,16 @@ prefix argument.  PREFIX and TAIL are used internally."
>  (defun notmuch-substitute-command-keys (doc)
>    "Like `substitute-command-keys' but with documentation, not function names."
>    (let ((beg 0))
> -    (while (string-match "\\\\{\\([^}[:space:]]*\\)}" doc beg)
> -      (let* ((keymap-name (substring doc (match-beginning 1) (match-end 1)))
> -	     (keymap (symbol-value (intern keymap-name)))
> -	     (ua-keys (where-is-internal 'universal-argument keymap t))
> -	     (desc-alist (notmuch-describe-keymap keymap ua-keys keymap))
> -	     (desc-list (mapcar (lambda (arg) (concat (car arg) "\t" (cdr arg))) desc-alist))
> -	     (desc (mapconcat #'identity desc-list "\n")))
> +    (while (string-match "\\\\{\\([^}[:space:]]*\\)}" doc beg) ;; matches \{not-space}
> +      (let ((desc
> +	     (save-match-data
> +	       (let* ((keymap-name (substring doc (match-beginning 1) (match-end 1)))
> +		      (keymap (symbol-value (intern keymap-name)))
> +		      (ua-keys (where-is-internal 'universal-argument keymap t))
> +		      (desc-alist (notmuch-describe-keymap keymap ua-keys keymap))
> +		      (desc-list (mapcar (lambda (arg) (concat (car arg) "\t" (cdr arg))) desc-alist))
> +		      (desc (mapconcat #'identity desc-list "\n")))
> +		 desc))))

Oof, what an annoying place to lose the match data.  My one suggestion
would be to put (mapconcat #'identity desc-list "\n") as the body of the
inner let, rather than binding `desc' and then immediately evaluating to
`desc'.  That would remove some redundancy, and it's already clear from
the outer let that the result of the mapconcat is named `desc'.

>  	(setq doc (replace-match desc 1 1 doc)))
>        (setq beg (match-end 0)))
>      doc))
> -- 
> 1.7.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch