Re: [nongnu] elpa/opam-switch-mode 8b8670ca8b 1/3: feat(opam-switch-mode-lighter): Show the switch name in the mode-bar

[Erik Martin-Dorel]

Hi Stefan! thanks for your helpful feedback.

On Tuesday, July 11, 2023 22:21 CEST, Stefan Monnier <monnier@iro.umontreal.ca> 
wrote:
> Salut Erik¸
> 
> > +        (progn (opam-switch-set-switch ,switch)
> > +               (redraw-display))
> >          :active t
> >          :help ,(concat "Select opam switch \"" switch "\"")])
> >      (opam-switch--get-switches))
> >     ;; now reset as last element
> >     '(
> > -     ["reset" (opam-switch-set-switch "")
> > +     ["reset"
> > +      (progn (opam-switch-set-switch "")
> > +             (redraw-display))
> 
> Instead of `redraw-display` you want `force-mode-line-update`.

OK thanks. But we specifically need to update the mode-line for *all* frames.
While `force-mode-line-update` only deals with the current buffer,
and `(force-mode-line-update t)` just seems to deal with all buffers from the 
current frame.
Do you confirm?

> And you probably want to put it into `opam-switch-set-switch` rather
> than duplicating it after every call:

Sure: I had done this slight duplication outside of `opam-switch-set-switch`
because the need for `redraw-display` only showed up when using the menu
— direct calls to `M-x opam-switch-set-switch …` were fine.
But indeed, your suggestion to get rid of the `progn` looks better!
(I'll be able to commit it after you reply to this e-mail :)

> Also, in some circumstances the mode lines can be updated fairly often
> (e.g. after every key press, and/or every process output received by
> a filter), so it might be worthwhile to memoize/precompute the
> lighter rather than recompute it every time the modeline is updated.

Thanks for the suggestion, but IINM your patch only precomputes the lighter 
once,
while it would need to be recomputed each time we change the opam switch,
otherwise the ligther becomes out-of-sync.

So I'm afraid that this optimization leads to over-complicated code…
and given no subexpression of the current (opam-switch-mode-lighter)
involves any external system call, I'd say it is not that inefficient/costly…
so maybe we can just keep it as is;
just like the merlin.el lighter is implemented BTW:
https://github.com/ocaml/merlin/blob/4f6c7cfee2344dd75e9568f25c0c1576521ec049/emacs/merlin.el#L2046
Are you OK with that point?

Kind regards,
Erik

> So maybe the patch below?
> 
> 
>         Stefan
> 
> 
> diff --git a/opam-switch-mode.el b/opam-switch-mode.el
> index 0ec42aa123..b13cbb75d7 100644
> --- a/opam-switch-mode.el
> +++ b/opam-switch-mode.el
> @@ -93,6 +93,8 @@ background process before the opam switch changes."
>  
>  ;;; Code:
>  
> +(defvar opam-switch--mode-lighter nil)
> +
>  (defun opam-switch--run-command-without-stderr (sub-cmd
>                                                  &optional switch sexp
>                                                  &rest args)
> @@ -321,6 +323,8 @@ not any other shells outside Emacs."
>        (unless opam-switch--saved-env
>          (opam-switch--save-current-env opam-env))
>        (opam-switch--set-env opam-env prefix)))
> +  (setq opam-switch--mode-lighter nil)
> +  (force-mode-line-update t)
>    (run-hooks 'opam-switch-change-opam-switch-hook))
>  
>  (define-obsolete-function-alias 'opam-switch--set-switch
> @@ -343,17 +347,13 @@ not any other shells outside Emacs."
>     ;; then the list with all the real opam switches
>     (mapcar
>      (lambda (switch)
> -      `[,switch
> -        (progn (opam-switch-set-switch ,switch)
> -               (redraw-display))
> +      `[,switch (opam-switch-set-switch ,switch)
>          :active t
>          :help ,(concat "Select opam switch \"" switch "\"")])
>      (opam-switch--get-switches))
>     ;; now reset as last element
>     '(
> -     ["reset"
> -      (progn (opam-switch-set-switch "")
> -             (redraw-display))
> +     ["reset" (opam-switch-set-switch "")
>        :active opam-switch--saved-env
>        :help "Reset to state when Emacs was started"])))
>  
> @@ -376,10 +376,12 @@ is automatically created by `define-minor-mode'."
>  
>  (defun opam-switch-mode-lighter ()
>    "Return the lighter for opam-switch-mode which indicates the current 
> switch."
> -  (let* ((current-switch (opam-switch--get-current-switch))
> -         ;; handle the case of local switches for better UX
> -         (shortened (replace-regexp-in-string ".*/" "…/" current-switch t 
> t)))
> -    (format " OPSW-%s" shortened)))
> +  (or opam-switch--mode-lighter
> +      (let* ((current-switch (opam-switch--get-current-switch))
> +             ;; handle the case of local switches for better UX
> +             (shortened (replace-regexp-in-string ".*/" "…/"
> +                                                  current-switch t t)))
> +        (setq opam-switch--mode-lighter (format " OPSW-%s" shortened)))))
>  
>  ;;;###autoload
>  (define-minor-mode opam-switch-mode

-- 
Érik Martin-Dorel, PhD
Maître de Conférences, Lab. IRIT, Univ. Toulouse 3
erik.martin-dorel@irit.fr
erik.martin-dorel@master-developpement-logiciel.fr
https://www.irit.fr/~Erik.Martin-Dorel/