[PATCH] Fix notmuch-describe-key

[PATCH] Fix notmuch-describe-key

[Yang Sheng]

Fix notmuch-describe-key crashing for the following two cases
1. format-kbd-macro cannot deal with keys like [(32 . 126)], switch to
use key-description instead.
2. if a function in the current keymap is not bounded, it will crash
the whole process. We check if it is bounded and silently skip it to
avoid crashing.
---
 emacs/notmuch-lib.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 8cf7261e..546ab6fd 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -298,7 +298,7 @@ This is basically just `format-kbd-macro' but we also convert ESC to M-."
   "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))))
+  (let ((key-string (concat prefix (key-description 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.
@@ -313,7 +313,7 @@ It does not prepend if ACTUAL-KEY is already listed in TAIL."
       ;; Documentation for command
       (push (cons key-string
 		  (or (and (symbolp binding) (get binding 'notmuch-doc))
-		      (notmuch-documentation-first-line binding)))
+		      (and (functionp binding) (notmuch-documentation-first-line binding))))
 	    tail)))
     tail)
 
-- 
2.20.1

Re: [PATCH] Fix notmuch-describe-key

[William Casarin]

Yang Sheng <yangsheng6810@gmail.com> writes:

> Fix notmuch-describe-key crashing for the following two cases
> 1. format-kbd-macro cannot deal with keys like [(32 . 126)], switch to
> use key-description instead.
> 2. if a function in the current keymap is not bounded, it will crash
> the whole process. We check if it is bounded and silently skip it to
> avoid crashing.
> ---
>  emacs/notmuch-lib.el | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index 8cf7261e..546ab6fd 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -298,7 +298,7 @@ This is basically just `format-kbd-macro' but we also convert ESC to M-."
>    "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))))
> +  (let ((key-string (concat prefix (key-description 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.
> @@ -313,7 +313,7 @@ It does not prepend if ACTUAL-KEY is already listed in TAIL."
>        ;; Documentation for command
>        (push (cons key-string
>  		  (or (and (symbolp binding) (get binding 'notmuch-doc))
> -		      (notmuch-documentation-first-line binding)))
> +		      (and (functionp binding) (notmuch-documentation-first-line binding))))
>  	    tail)))
>      tail)
>  

Thanks!

Some context: this fixes an issue in spacemacs where the help key is
broken:

  https://github.com/syl20bnr/spacemacs/issues/10123


-- 
https://jb55.com

Re: [PATCH] Fix notmuch-describe-key

[Matt Armstrong]

Yang Sheng <yangsheng6810@gmail.com> writes:

> Fix notmuch-describe-key crashing for the following two cases
> 1. format-kbd-macro cannot deal with keys like [(32 . 126)], switch to
> use key-description instead.
> 2. if a function in the current keymap is not bounded, it will crash
> the whole process. We check if it is bounded and silently skip it to
> avoid crashing.

Can you describe case 2 in more detail?  I am wondering what "crash the
whole process means" -- Emacs itself crashes?  If so, is there an Emacs
bug filed for it?  Also, perhaps the bug should be worked around with in
notmuch-documentation-first-line itself, rather than its caller?

 
> ---
>  emacs/notmuch-lib.el | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index 8cf7261e..546ab6fd 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -298,7 +298,7 @@ This is basically just `format-kbd-macro' but we also convert ESC to M-."
>    "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))))
> +  (let ((key-string (concat prefix (key-description 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.
> @@ -313,7 +313,7 @@ It does not prepend if ACTUAL-KEY is already listed in TAIL."
>        ;; Documentation for command
>        (push (cons key-string
>  		  (or (and (symbolp binding) (get binding 'notmuch-doc))
> -		      (notmuch-documentation-first-line binding)))
> +		      (and (functionp binding) (notmuch-documentation-first-line binding))))
>  	    tail)))
>      tail)
>  
> -- 
> 2.20.1

Re: [PATCH] Fix notmuch-describe-key

[Matt Armstrong]

Matt Armstrong <marmstrong@google.com> writes:

> Yang Sheng <yangsheng6810@gmail.com> writes:
>
>> Fix notmuch-describe-key crashing for the following two cases
>> 1. format-kbd-macro cannot deal with keys like [(32 . 126)], switch to
>> use key-description instead.
>> 2. if a function in the current keymap is not bounded, it will crash
>> the whole process. We check if it is bounded and silently skip it to
>> avoid crashing.
>
> Can you describe case 2 in more detail?  I am wondering what "crash the
> whole process means" -- Emacs itself crashes?  If so, is there an Emacs
> bug filed for it?  Also, perhaps the bug should be worked around with in
> notmuch-documentation-first-line itself, rather than its caller?

For example, this kind of fix makes it clearer that the problem is with
the `documentation' function.


modified   emacs/notmuch-lib.el
@@ -273,7 +273,7 @@ it, in which case it is killed."
 
 (defun notmuch-documentation-first-line (symbol)
   "Return the first line of the documentation string for SYMBOL."
-  (let ((doc (documentation symbol)))
+  (let ((doc (and (functionp symbol) (documentation symbol))))
     (if doc
 	(with-temp-buffer
 	  (insert (documentation symbol t))




>> ---
>>  emacs/notmuch-lib.el | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
>> index 8cf7261e..546ab6fd 100644
>> --- a/emacs/notmuch-lib.el
>> +++ b/emacs/notmuch-lib.el
>> @@ -298,7 +298,7 @@ This is basically just `format-kbd-macro' but we also convert ESC to M-."
>>    "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))))
>> +  (let ((key-string (concat prefix (key-description 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.
>> @@ -313,7 +313,7 @@ It does not prepend if ACTUAL-KEY is already listed in TAIL."
>>        ;; Documentation for command
>>        (push (cons key-string
>>  		  (or (and (symbolp binding) (get binding 'notmuch-doc))
>> -		      (notmuch-documentation-first-line binding)))
>> +		      (and (functionp binding) (notmuch-documentation-first-line binding))))
>>  	    tail)))
>>      tail)
>>  
>> -- 
>> 2.20.1

Re: [PATCH] Fix notmuch-describe-key

[Sheng Yang]

For the second case, I mean the call of notmuch-help fails with the
following error:

notmuch-documentation-first-line: Symbol’s function definition is void: aya-persist-snippet

Sorry for the confusion. Other void functions include the following for me

spacemacs/search-pt-grep-region-or-symbol
spacemacs/search-pt-grep
spacemacs/search-rg-grep-region-or-symbol
spacemacs/search-rg-grep
spacemacs/search-ack-grep-region-or-symbol
spacemacs/search-ack-grep


I think either is fine: notmuch-documentation-first-line is only called
from notmuch-describe-key, we can do it in either function. The
advantage of fixing it in notmuch-describe-key is that we can avoid some
lines that have key bindings without description.

Here are some known problems that are not fixed by this patch:
1. For me, 600+ lines of help are produced by notmuch-hello, which is a
lot. (Maybe not as much, as counsel-desbinds tells me there are 3000+
key bindings)
2. Key bindings using leader key (in spacemacs, it is ESC) are not
shown correctly: instead of showing "SPC q q", notmuch-help shows "q q"
for a key binding <SPC q q>. 
3. Some lines show key bindings with no explanation. Possibly because no
doc string is available for these functions.

If we do not need to stick to the current implementation of
notmuch-help, a simple way is to port counsel-desbinds or
counsel--desbinds-cands from counsel.el.


Matt Armstrong <marmstrong@google.com> writes:

> Can you describe case 2 in more detail?  I am wondering what "crash the
> whole process means" -- Emacs itself crashes?  If so, is there an Emacs
> bug filed for it?  Also, perhaps the bug should be worked around with in
> notmuch-documentation-first-line itself, rather than its caller?
>
>  

-- 
Sheng Yang(杨圣), PhD student
Computer Science Department
University of Maryland, College Park
E-mail: yangsheng6810@gmail.com

Re: [PATCH] Fix notmuch-describe-key

[Matt Armstrong]

Sheng Yang <yangsheng6810@gmail.com> writes:

> For the second case, I mean the call of notmuch-help fails with the
> following error:
>
> notmuch-documentation-first-line: Symbol’s function definition is void: aya-persist-snippet
>
> Sorry for the confusion. Other void functions include the following for me
>
> spacemacs/search-pt-grep-region-or-symbol
> spacemacs/search-pt-grep
> spacemacs/search-rg-grep-region-or-symbol
> spacemacs/search-rg-grep
> spacemacs/search-ack-grep-region-or-symbol
> spacemacs/search-ack-grep

Thank you, that makes sense to me now.


> I think either is fine: notmuch-documentation-first-line is only
> called from notmuch-describe-key, we can do it in either function. The
> advantage of fixing it in notmuch-describe-key is that we can avoid
> some lines that have key bindings without description.

I agree.  Given all this, I think your patch looks good.


> Here are some known problems that are not fixed by this patch:
> 1. For me, 600+ lines of help are produced by notmuch-hello, which is a
> lot. (Maybe not as much, as counsel-desbinds tells me there are 3000+
> key bindings)

It is interesting that your help buffer displays 600 lines.  The intent
of the notmuch help is to display help for the notmuch commands, not the
myriad of other key bindings available.

I use vanilla Emacs (not spacemacs, not evil mode, etc.), and the help
buffers are about a page long.  For example, in a search buffer the key
bindings display as this:

Complete list of currently available key bindings:

?	Display help for the current notmuch mode.
q	Undisplay the current buffer.
s	Search for messages.
z	Display threads matching QUERY in Tree View.
g	Refresh the current buffer.
=	Refresh the current buffer.
M-=	Invoke ‘notmuch-refresh-this-buffer’ on all notmuch major-mode buffers.
G	Invoke ‘notmuch-poll’ to import mail, then refresh the current buffer.
j	Jump to a saved search by shortcut key.
x	Undisplay the current buffer.
DEL	Move backward through the search results by one window’s worth.
b	Move backward through the search results by one window’s worth.
SPC	Move forward through search results by one window’s worth.
<	Select the first thread in the search results.
>	Select the last thread in the search results.
p	Select the previous thread in the search results.
n	Select the next thread in the search results.
r	Begin composing a reply to the entire current thread in a new buffer.
R	Begin composing a reply-all to the entire current thread in a new buffer.
o	Toggle the current search order.
c i	Copy thread ID of current thread to kill-ring.
c q	Copy current query to kill-ring.
c ?	Show help for a subkeymap.
t	Filter the current search results based on a single tag.
l	Filter or LIMIT the current search results based on an additional query string.
k	Create a jump menu for tagging operations.
*	Add/remove tags from all messages in current search buffer.
a	Archive the currently selected thread or region.
C-u a	Un-archive the currently selected thread.
-	Change tags for the current thread or region (defaulting to remove).
+	Change tags for the current thread or region (defaulting to add).
RET	Display the currently selected thread.
Z	Call notmuch tree with the current query
m	Mute the currently selected thread or region.


> 2. Key bindings using leader key (in spacemacs, it is ESC) are not
> shown correctly: instead of showing "SPC q q", notmuch-help shows "q q"
> for a key binding <SPC q q>. 
> 3. Some lines show key bindings with no explanation. Possibly because no
> doc string is available for these functions.
>
> If we do not need to stick to the current implementation of
> notmuch-help, a simple way is to port counsel-desbinds or
> counsel--desbinds-cands from counsel.el.

Yes, I can believe that spacemacs and/or evil mode do not display useful
help when using "vanilla" Emacs commands to display the help, since
vanilla Emacs doesn't understand the approach.

I think it would be great if someone worked with upstream Emacs to make
the `documentation' function do something useful for evil mode and
spacemacs.  If that happened, then all packages would benefit.  Porting
things like counsel-descbinds into packages like notmuch can work but
does not feel like the right long term solution to me.

Re: [PATCH] Fix notmuch-describe-key

[Sheng Yang]

Matt Armstrong <marmstrong@google.com> writes:

> I think it would be great if someone worked with upstream Emacs to make
> the `documentation' function do something useful for evil mode and
> spacemacs.  If that happened, then all packages would benefit.  Porting
> things like counsel-descbinds into packages like notmuch can work but
> does not feel like the right long term solution to me.

I guess the problem does not come from `documentation' function itself.
I will investigate a bit more.

What if we filter the key bindings and show only those that are binded
to notmuch functions? Maybe those functions that matches "^notmuch-"?
This only shows related key bindings related to notmuch, and the list
would not be long anyway.

-- 
Sheng Yang(杨圣), PhD student
Computer Science Department
University of Maryland, College Park
E-mail: yangsheng6810@gmail.com

Re: [PATCH] Fix notmuch-describe-key

[Matt Armstrong]

Sheng Yang <yangsheng6810@gmail.com> writes:

> Matt Armstrong <marmstrong@google.com> writes:
>
>> I think it would be great if someone worked with upstream Emacs to make
>> the `documentation' function do something useful for evil mode and
>> spacemacs.  If that happened, then all packages would benefit.  Porting
>> things like counsel-descbinds into packages like notmuch can work but
>> does not feel like the right long term solution to me.
>
> I guess the problem does not come from `documentation' function itself.
> I will investigate a bit more.
>
> What if we filter the key bindings and show only those that are binded
> to notmuch functions? Maybe those functions that matches "^notmuch-"?
> This only shows related key bindings related to notmuch, and the list
> would not be long anyway.

Yes, that could be a practical and pragmatic solution.  I do not oppose
it, but see my one note about this below.

I suggested improving core Emacs help functions earlier simply because I
think the core help system should be better, which would reduce the
reasons we see packages like notmuch and counsel trying to do a better
job.  For example, if core Emacs understood the approaches taken by
evil-mode to bind keys it could do a better job with help.  Emacs help
already does a "bad" job with other modal key maps (e.g. the help for
the ESC key is displayed as an almost useless "Prefix Command" by Emacs
today).

Improving all this is a long term solution -- any fix to Emacs itself
takes years to be available to everyone.

But take M-x describe-mode in a notmuch-search buffer.  It shows me the
list below.  Clearly the original intent of the notmuch help functions
were to make that more useful, but something is going wrong in the
spacemacs case.

I'm not sure why.  Emacs lisp is so flexible that anything could be
happening.  For example, spacemacs could be installing advice around the
`documentation' function that modifies the behavior in a way that makes
sense for spacemacs but confuses the notmuch code.  I am not sure why so
many spacemacs bindings see to be in the notmuch key maps.  Who knows.
:)

One problem with filtering the command list is shown below.  I have
bound a command of my own, my-notmuch-search-mute-thread, to the key
map.  I find it useful to have that included in the help output.


RET             notmuch-search-show-thread
ESC             Prefix Command
SPC             notmuch-search-scroll-up
*               notmuch-search-tag-all
+               notmuch-search-add-tag
-               notmuch-search-remove-tag
<               notmuch-search-first-thread
=               notmuch-refresh-this-buffer
>               notmuch-search-last-thread
?               notmuch-help
G               notmuch-poll-and-refresh-this-buffer
R               notmuch-search-reply-to-thread
Z               notmuch-tree-from-search-current-query
a               notmuch-search-archive-thread
b               notmuch-search-scroll-down
c               notmuch-search-stash-map
g               notmuch-refresh-this-buffer
j               notmuch-jump-search
k               notmuch-tag-jump
l               notmuch-search-filter
m               my-notmuch-search-mute-thread
n               notmuch-search-next-thread
o               notmuch-search-toggle-order
p               notmuch-search-previous-thread
q               notmuch-bury-or-kill-this-buffer
r               notmuch-search-reply-to-thread-sender
s               notmuch-search
t               notmuch-search-filter-by-tag
x               notmuch-bury-or-kill-this-buffer
z               notmuch-tree
DEL             notmuch-search-scroll-down
<mouse-1>       notmuch-search-show-thread

c ?             notmuch-subkeymap-help
c i             notmuch-search-stash-thread-id
c q             notmuch-stash-query

M-=             notmuch-refresh-all-buffers

Re: [PATCH] Fix notmuch-describe-key

[David Bremner]

Yang Sheng <yangsheng6810@gmail.com> writes:

> Fix notmuch-describe-key crashing for the following two cases
> 1. format-kbd-macro cannot deal with keys like [(32 . 126)], switch to
> use key-description instead.
> 2. if a function in the current keymap is not bounded, it will crash
> the whole process. We check if it is bounded and silently skip it to
> avoid crashing.

pushed,

d