Re: [elpa] externals/notmuch-indicator f563869b6a 2/4: Tweak how the keymap is implemented

Re: [elpa] externals/notmuch-indicator f563869b6a 2/4: Tweak how the keymap is implemented

[Stefan Monnier]

> branch: externals/notmuch-indicator
> commit f563869b6a5e5e12fd791bc109232ed45064f7a7
> Author: Protesilaos Stavrou <info@protesilaos.com>
> Commit: Protesilaos Stavrou <info@protesilaos.com>
>
>     Tweak how the keymap is implemented
>     
>     This continues the work done by Henrik Kjerringvåg in commit f5c9b2e.
>     Henrik does not require to assign copyright to the Free Software
>     Foundation for that commit, as it is within the ~15 line threshold.
> ---
>  notmuch-indicator.el | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/notmuch-indicator.el b/notmuch-indicator.el
> index 89e93054ee..9b598910e2 100644
> --- a/notmuch-indicator.el
> +++ b/notmuch-indicator.el
> @@ -166,17 +166,21 @@ option `notmuch-indicator-refresh-count'."
>     (shell-command-to-string
>      (format "notmuch count %s" terms))))
>  
> +(defvar notmuch-indicator-mode-map
> +  (let ((map (make-sparse-keymap)))
> +    map)
> +  "Keymap for the Notmuch indicator.")
> +
>  (defun notmuch-indicator--format-label (label count face terms)
>    "Format LABEL, COUNT, FACE and TERMS of `notmuch-indicator-args'."
> -  (let ((map (make-sparse-keymap)))
> -    (define-key map [mode-line mouse-1]
> -                (lambda () (interactive) (notmuch-search terms)))
> -    (propertize
> -     (if (and face label)
> -         (format "%s%s " (propertize label 'face face) count)
> -       (format "%s%s " (or label "") count))
> -     'help-echo "mouse-1: Open notmuch search"
> -     'local-map map)))
> +  (define-key notmuch-indicator-mode-map [mode-line mouse-1]
> +              (lambda () (interactive) (notmuch-search terms)))
> +  (propertize
> +   (if (and face label)
> +       (format "%s%s " (propertize label 'face face) count)
> +     (format "%s%s " (or label "") count))
> +   'help-echo (format "mouse-1: Open notmuch search for `%s'" terms)
> +   'local-map notmuch-indicator-mode-map))

Hmm... really?  The new `define-key` will overwrite the binding added in
the previous call to `notmuch-indicator--format-label`, so if you have
several elements in `notmuch-indicator-args`, they'll all use the last
element's `terms` when you click `mouse-1` on them.

While I'm here:

> +   (if (and face label)
> +       (format "%s%s " (propertize label 'face face) count)
> +     (format "%s%s " (or label "") count))

can be rewritten to

    
        (format "%s%s "
                (if (and face label) (propertize label 'face face)
                  (or label "")
                count)

Oh, and I see elsewhere that you call `mapconcat` with only 2 args,
which is a new feature in Emacs-29 whereas the package claims to want to
support Emacs-27.


        Stefan

Re: [elpa] externals/notmuch-indicator f563869b6a 2/4: Tweak how the keymap is implemented

[Protesilaos Stavrou]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Wed, 28 Sep 2022 14:30:01 -0400
>
>> branch: externals/notmuch-indicator
>> commit f563869b6a5e5e12fd791bc109232ed45064f7a7
>> Author: Protesilaos Stavrou <info@protesilaos.com>
>> Commit: Protesilaos Stavrou <info@protesilaos.com>
>>
>>     Tweak how the keymap is implemented
>>     
>>     This continues the work done by Henrik Kjerringvåg in commit f5c9b2e.
>>     Henrik does not require to assign copyright to the Free Software
>>     Foundation for that commit, as it is within the ~15 line threshold.

> [... 37 lines elided]

> Hmm... really?  The new `define-key` will overwrite the binding added in
> the previous call to `notmuch-indicator--format-label`, so if you have
> several elements in `notmuch-indicator-args`, they'll all use the last
> element's `terms` when you click `mouse-1` on them.

Thanks for pointing it out!  I reverted to the previous approach.

> While I'm here:
>
>> +   (if (and face label)
>> +       (format "%s%s " (propertize label 'face face) count)
>> +     (format "%s%s " (or label "") count))
>
> can be rewritten to
>
>     
>         (format "%s%s "
>                 (if (and face label) (propertize label 'face face)
>                   (or label "")
>                 count)

Done.  Thank you!

> Oh, and I see elsewhere that you call `mapconcat` with only 2 args,
> which is a new feature in Emacs-29 whereas the package claims to want to
> support Emacs-27.

Thanks!  On a more general note, how can we catch cases like this one?
The compiler did not warn about it and the doc string of mapconcat does
not explain as much.  I know there is an entry in the NEWS, but having
to check that manually is not easy.

-- 
Protesilaos Stavrou
https://protesilaos.com

Re: [elpa] externals/notmuch-indicator f563869b6a 2/4: Tweak how the keymap is implemented

[Stefan Monnier]

>> Oh, and I see elsewhere that you call `mapconcat` with only 2 args,
>> which is a new feature in Emacs-29 whereas the package claims to want to
>> support Emacs-27.
>
> Thanks!  On a more general note, how can we catch cases like this one?

Byte-compile with the oldest Emacs you want to support and check
its warnings?


        Stefan


PS: We make efforts to provide warnings that help people adjust old code
to work better in current and future Emacsen, but we don't provide any
such thing in the other direction.

The closest we have is that `C-h o` will sometimes give you a guess
about the Emacs version in which this var/fun was introduced.  And IIUC
this functionality is not considered as very important.

Re: [elpa] externals/notmuch-indicator f563869b6a 2/4: Tweak how the keymap is implemented

[Stefan Monnier]

Here's another trivial simplification (more specifically an η-reduction).


        Stefan


diff --git a/notmuch-indicator.el b/notmuch-indicator.el
index 07ada7aec8..7680ccdc3d 100644
--- a/notmuch-indicator.el
+++ b/notmuch-indicator.el
@@ -199,8 +199,7 @@ option `notmuch-indicator-refresh-count'."
 (defun notmuch-indicator--return-count ()
   "Parse `notmuch-indicator-args' and format them as single string."
   (mapconcat
-   (lambda (props)
-     (notmuch-indicator--format-output props))
+   #'notmuch-indicator--format-output
    notmuch-indicator-args ""))
 
 (defvar notmuch-indicator-string ""

Re: [elpa] externals/notmuch-indicator f563869b6a 2/4: Tweak how the keymap is implemented

[Protesilaos Stavrou]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Wed, 28 Sep 2022 16:38:38 -0400
>
> Here's another trivial simplification (more specifically an η-reduction).
>
>
>         Stefan
>
>
> diff --git a/notmuch-indicator.el b/notmuch-indicator.el
> index 07ada7aec8..7680ccdc3d 100644
> --- a/notmuch-indicator.el
> +++ b/notmuch-indicator.el
> @@ -199,8 +199,7 @@ option `notmuch-indicator-refresh-count'."
>  (defun notmuch-indicator--return-count ()
>    "Parse `notmuch-indicator-args' and format them as single string."
>    (mapconcat
> -   (lambda (props)
> -     (notmuch-indicator--format-output props))
> +   #'notmuch-indicator--format-output
>     notmuch-indicator-args ""))
>  
>  (defvar notmuch-indicator-string ""
>

Installed it.  Thank you!

-- 
Protesilaos Stavrou
https://protesilaos.com