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

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

[Stefan Monnier]

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`.
And you probably want to put it into `opam-switch-set-switch` rather
than duplicating it after every call:

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.

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

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/

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

[Stefan Monnier]

>> 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?

No, `(force-mode-line-update t)` should apply to all frames.

> 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.

IIRC that's because the use of the minibuffer forces a more complete
redisplay which happens to recompute those modelines, tho I'd be
surprised if it recomputes the modelines of all frames.

>> 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.

No, I reset it to nil at the same time as I call
`force-mode-line-update`, which should cause it to be recomputed at the
next redisplay.

> 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…

It's not too bad, admittedly.

> https://github.com/ocaml/merlin/blob/4f6c7cfee2344dd75e9568f25c0c1576521ec049/emacs/merlin.el#L2046
> Are you OK with that point?

It's up to you.


        Stefan

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]

Salut Stefan !

On Wednesday, July 12, 2023 00:09 CEST, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> > 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?
> 
> No, `(force-mode-line-update t)` should apply to all frames.

OK!

> >  given no subexpression of the current (opam-switch-mode-lighter)
> > involves any external system call, I'd say it is not that inefficient/costly…
> 
> It's not too bad, admittedly.
> 
> > https://github.com/ocaml/merlin/blob/4f6c7cfee2344dd75e9568f25c0c1576521ec049/emacs/merlin.el#L2046
> > Are you OK with that point?
> 
> It's up to you.

OK ;) so, given your optimization works very well and stays simple,
I've applied it just now, and pushed a new release:
https://github.com/ProofGeneral/opam-switch-mode/compare/1.4...1.5

Thanks again,
Erik

-- 
É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/