bug#40863: [PATCH] Improve the display-time-world UI

[Basil L. Contovounesios]

Stefan Kangas <stefan@marxist.se> writes:

> I have made some improvements to the display-time-world UI.  I divided
> them up into four patches to ease review and merging of the individual
> features.  Please let me know what you think.

Thanks for working on this, see my comments below.

> (Of course I can squash the patches before pushing if that is preferable.)
>
> Patch 4 adds an alias 'world-clock'.  Ideally, I would like to rename
> the somewhat obscurely named 'display-world-time' to 'world-clock' and
> make the old names into obsolete aliases.  It would be good to hear
> any opinions on that too.

No strong feelings either way here.

> I'm also not sure if any of this should go into NEWS, so I didn't do
> that work for now.  Let me know if that's desirable.

A new entry-point command alias intended as a replacement for the
current command should definitely be announced.  Not sure about the
rest.

> Best regards,
> Stefan Kangas
>
> From 3d8c091a03c25826755b3735eca9da4b342826ba Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sun, 26 Apr 2020 09:38:03 +0200
> Subject: [PATCH 1/4] Kill display-time-world buffer on exit
>
> * lisp/time.el (display-time-world-mode-map): New defvar.
> (display-time-world-exit): New defun to kill buffer on exit.
> (display-time-world): Doc fix.
> ---
>  lisp/time.el | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/lisp/time.el b/lisp/time.el
> index 44fd1a7e33..a5e5b9b4a7 100644
> --- a/lisp/time.el
> +++ b/lisp/time.el
> @@ -509,6 +509,13 @@ display-time-mode
>                'display-time-event-handler)))
>  
>  
> +(defvar display-time-world-mode-map
> +  (let ((map (make-sparse-keymap)))
> +    (define-key map "g" nil)

Rather than remove the revert-buffer binding inherited from
special-mode-map, wouldn't it be better to implement a
revert-buffer-function that updates the world clocks displayed?

> +    (define-key map "q" 'display-time-world-exit)

[...]

> +(defun display-time-world-exit ()
> +  "Quit the world time buffer."
> +  (interactive)
> +  (quit-window t))

I don't think this is a change for the better.  The existing binding of
quit-window, inherited by and consistent across many special-mode
derivatives, already takes a prefix argument for killing the
corresponding buffer if a user prefers that to burying.  The proposed
change would make killing the only option by default.

>  ;;;###autoload
>  (defun display-time-world ()
>    "Enable updating display of times in various time zones.
> +\\<display-time-world-mode-map>
>  `display-time-world-list' specifies the zones.
> -To turn off the world time display, go to that window and type `q'."
> +To turn off the world time display, go to that window and type 
> `\\[display-time-world-exit]'."

If you agree with me then this can be changed to \\[quit-window].

[...]

> @@ -540,7 +544,10 @@ display-time-world-display
>         (setq max-width width))))
>      (setq fmt (concat "%-" (int-to-string max-width) "s %s\n"))
>      (dolist (timedata (nreverse result))
> -      (insert (format fmt (car timedata) (cdr timedata))))
> +      (insert (format fmt
> +                      (propertize (car timedata)
> +                                  'face 'display-time-world-label)
> +                      (cdr timedata))))

Just curious: when do we use 'face' and when 'font-lock-face'?

>      (delete-char -1))
>    (goto-char (point-min)))
>  
> -- 
> 2.26.2
>
>
> From 0f2ca954e03b857554071ae48a98d44ba7030d69 Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sun, 26 Apr 2020 10:44:00 +0200
> Subject: [PATCH 3/4] Move point to new buffer for display-time-world
>
> * lisp/time.el (display-time-world): Move point to created buffer.
> ---
>  lisp/time.el | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/lisp/time.el b/lisp/time.el
> index 44885f74d1..84c0071b7d 100644
> --- a/lisp/time.el
> +++ b/lisp/time.el
> @@ -566,11 +566,11 @@ display-time-world
>    (when (and display-time-world-timer-enable
>               (not (get-buffer display-time-world-buffer-name)))
>      (run-at-time t display-time-world-timer-second 
> 'display-time-world-timer))
> -  (with-current-buffer (get-buffer-create display-time-world-buffer-name)
> -    (display-time-world-display (time--display-world-list))
> -    (display-buffer display-time-world-buffer-name
> -                 (cons nil '((window-height . fit-window-to-buffer))))
> -    (display-time-world-mode)))
> +  (switch-to-buffer-other-window (get-buffer-create 
> display-time-world-buffer-name))
> +  (display-time-world-display (time--display-world-list))
> +  (display-buffer display-time-world-buffer-name
> +               (cons nil '((window-height . fit-window-to-buffer))))

switch-to-buffer and friends have historically not played nicely with
display-buffer-alist and switch-to-buffer-preserve-window-point when
used in Elisp libraries, with adverse user-visible effects.  Unless
there's a specific reason to use switch-to-buffer, pop-to-buffer and
friends should generally be preferred instead.

Note that using pop-to-buffer or similar here could remove the need for
calling both get-buffer-create and display-buffer, while remaining fully
customisable by the user.

> +  (display-time-world-mode))
>  
>  (defun display-time-world-timer ()
>    (if (get-buffer display-time-world-buffer-name)
> -- 
> 2.26.2
>
>
> From 47401a1e10213d89cbdb6eaa2b66b44c2be37dfe Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sun, 26 Apr 2020 10:47:17 +0200
> Subject: [PATCH 4/4] Improve the display-time-world UI
>
> * lisp/time.el (display-time-world-buffer-name): Change default
> buffer name.
> (display-time-world): Add usage instructions to modeline.

Are there many other modes that do this?  Is it really that helpful to
have a standard special-mode binding displayed prominently in the mode
line by default, with no easy way to remove it?

FWIW, on entry some modes display useful key bindings in the echo area,
which I find far less intrusive than a more permanent addition to the
mode line.

[...]

> @@ -570,7 +570,11 @@ display-time-world
>    (display-time-world-display (time--display-world-list))
>    (display-buffer display-time-world-buffer-name
>                 (cons nil '((window-height . fit-window-to-buffer))))
> -  (display-time-world-mode))
> +  (display-time-world-mode)
> +  (setq mode-line-format (format "%15s %13s" "World Clock" "q to quit")))

Doesn't this override the entire mode line, erasing all other default
constructs?

What's wrong with the status quo, which displays the mode name, "World
clock", like every other mode does?

> +;; This would be a more intuitive name.
> +(defalias 'world-clock 'display-time-world)
>  
>  (defun display-time-world-timer ()
>    (if (get-buffer display-time-world-buffer-name)
> -- 
> 2.26.2

Otherwise LGTM.

-- 
Basil