all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* Re: [nongnu] elpa/opam-switch-mode 8b8670ca8b 1/3: feat(opam-switch-mode-lighter): Show the switch name in the mode-bar
       [not found] ` <20230711200015.24AC2C06C6C@vcs2.savannah.gnu.org>
@ 2023-07-11 20:21   ` Stefan Monnier
  2023-07-11 21:09     ` Erik Martin-Dorel
  0 siblings, 1 reply; 4+ messages in thread
From: Stefan Monnier @ 2023-07-11 20:21 UTC (permalink / raw)
  To: Erik Martin-Dorel; +Cc: emacs-devel

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




^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [nongnu] elpa/opam-switch-mode 8b8670ca8b  1/3: feat(opam-switch-mode-lighter): Show the  switch name in the mode-bar
  2023-07-11 20:21   ` [nongnu] elpa/opam-switch-mode 8b8670ca8b 1/3: feat(opam-switch-mode-lighter): Show the switch name in the mode-bar Stefan Monnier
@ 2023-07-11 21:09     ` Erik Martin-Dorel
  2023-07-11 22:09       ` Stefan Monnier
  0 siblings, 1 reply; 4+ messages in thread
From: Erik Martin-Dorel @ 2023-07-11 21:09 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

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/



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [nongnu] elpa/opam-switch-mode 8b8670ca8b  1/3: feat(opam-switch-mode-lighter): Show the  switch name in the mode-bar
  2023-07-11 21:09     ` Erik Martin-Dorel
@ 2023-07-11 22:09       ` Stefan Monnier
  2023-07-12 12:13         ` Erik Martin-Dorel
  0 siblings, 1 reply; 4+ messages in thread
From: Stefan Monnier @ 2023-07-11 22:09 UTC (permalink / raw)
  To: Erik Martin-Dorel; +Cc: emacs-devel

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




^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [nongnu] elpa/opam-switch-mode 8b8670ca8b   1/3: feat(opam-switch-mode-lighter): Show the   switch name in the mode-bar
  2023-07-11 22:09       ` Stefan Monnier
@ 2023-07-12 12:13         ` Erik Martin-Dorel
  0 siblings, 0 replies; 4+ messages in thread
From: Erik Martin-Dorel @ 2023-07-12 12:13 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

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/



^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2023-07-12 12:13 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <168910561415.26377.10324105754705665342@vcs2.savannah.gnu.org>
     [not found] ` <20230711200015.24AC2C06C6C@vcs2.savannah.gnu.org>
2023-07-11 20:21   ` [nongnu] elpa/opam-switch-mode 8b8670ca8b 1/3: feat(opam-switch-mode-lighter): Show the switch name in the mode-bar Stefan Monnier
2023-07-11 21:09     ` Erik Martin-Dorel
2023-07-11 22:09       ` Stefan Monnier
2023-07-12 12:13         ` Erik Martin-Dorel

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.