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

[Basil L. Contovounesios]

Thanks for the ping and sorry about not getting back sooner.

I'm generally fine with the proposed changes, except for some comments
below.

> From e4e5012abdece7ce334900fd45f7e5ba06185ecc Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sun, 26 Apr 2020 10:16:06 +0200
> Subject: [PATCH 1/4] Improve display-time-world UI (Bug#40863)

[...]

> +(defvar display-time-world-mode-map
> +  (let ((map (make-sparse-keymap)))
> +    (define-key map "g" #'display-time-world-timer)

I think it's better to keep the standard default binding of
revert-buffer inherited from special-mode-map and set
revert-buffer-function in display-time-world-mode instead, as I
suggested in my last review.

Then display-time-world-timer won't need an interactive spec, which
doesn't sit too well with the function's current name and purpose.

> +    map)
> +  "Keymap for `display-time-world-mode'.")

[...]

> @@ -541,18 +554,17 @@ display-time-world-display
>  (defun display-time-world ()
>    "Enable updating display of times in various time zones.
>  `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 
> `\\[quit-window]'."
>    (interactive)
>    (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)))
> +  (pop-to-buffer display-time-world-buffer-name)
> +  (display-time-world-display (time--display-world-list))
> +  (display-time-world-mode))

You should call fit-window-to-buffer after populating the buffer to
preserve the old display-buffer action semantics.  (pop-to-buffer takes
the same action argument, but buffer might still be empty then.)

>  (defun display-time-world-timer ()
> +  (interactive)

This shouldn't be necessary, as mentioned above.

>    (if (get-buffer display-time-world-buffer-name)
>        (with-current-buffer (get-buffer display-time-world-buffer-name)
>          (display-time-world-display (time--display-world-list)))
> -- 
> 2.26.2
>
>
> From cd4c79a5758f0b6b8f5cc8acde62f806a1452b64 Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sat, 2 May 2020 16:08:33 +0200
> Subject: [PATCH 2/4] Make 'display-time-world' into an alias for 'world-clock'
>  (Bug#40863)
>
> * lisp/time.el (world-clock): Rename from 'display-time-world'.
> (display-time-world): Redefine as alias for 'world-clock'.
> * etc/NEWS: Announce the above change.
>
> (top-level, zoneinfo-style-world-list, legacy-style-world-list)
> (display-time-world-list, display-time-world-time-format)
> (display-time-world-buffer-name)
> (display-time-world-timer-enable)
> (display-time-world-timer-second, display-time-world-label)
> (display-time-world-timer): Update documentation to match the above
> rename.

These last entries should appear under lisp/time.el rather than
etc/NEWS, and you can either leave out top-level (which is the name of
an unrelated symbol) or write it as follows:

* lisp/time.el: Lorem ipsum...
(foo, bar): Curabitur pretium...

Yet another way you could probably get away with (others may correct
me), is:

* lisp/time.el (world-clock): Rename from 'display-time-world'.  Update
all documentation to mention the new name.
(display-time-world): Redefine as alias for 'world-clock'.
* etc/NEWS: Announce the above change.

[...]

> From 043650acccd2528c5459c3c854cbd886acae9642 Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sat, 2 May 2020 17:26:14 +0200
> Subject: [PATCH 3/4] Rename 'display-time-world' to 'world-clock' (Bug#40863)

[...]

> diff --git a/etc/NEWS b/etc/NEWS
> index d2b745c579..d97b148cc8 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -291,9 +291,26 @@ These new navigation commands are bound to 'n' and 'p' in
>  ** Time
>  
>  ---
> -*** 'display-time-world' is now an alias for 'world-clock'.
> +*** 'display-time-world' has been renamed to 'world-clock'.
>  'world-clock' creates a buffer with an updating time display using
> -several time zones.  The new name is hoped to be more discoverable.
> +several time zones.

I think you can keep "it is hoped that the new names are more
discoverable" as a justification for this mass rename.

> +The following functions have been renamed:
> +
> +  'display-time-world'         to 'world-clock'
> +  'display-time-world-mode'    to 'world-clock-mode'
> +  'display-time-world-display' to 'world-clock-display'
> +  'display-time-world-timer'   to 'world-clock-update'
> +
> +The following options have been renamed:
> +
> +  'display-time-world-list'         to 'world-clock-list'
> +  'display-time-world-time-format'  to 'world-clock-time-format'
> +  'display-time-world-buffer-name'  to 'world-clock-buffer-namt'
> +  'display-time-world-timer-enable' to 'world-clock-timer-enablt'
> +  'display-time-world-timer-second' to 'world-clock-timer-secont'

The last three world-clock-* vars are misspelt.

> +The old names are now obsolete.
>  
>  
>  * New Modes and Packages in Emacs 28.1
> diff --git a/lisp/time.el b/lisp/time.el
> index fe290ff71f..3f1880b708 100644
> --- a/lisp/time.el
> +++ b/lisp/time.el
> @@ -122,6 +122,10 @@ display-time-server-down-time
>     "Time when mail file's file system was recorded to be down.
>  If that file system seems to be up, the value is nil.")
>  
> +(defgroup world-clock nil
> +  "Show a world clock."

Nit: s/show/display/

Rings better in my ears and is consistent with the description of the
display-time group.

> +  :group 'applications)

It should continue to be under :group 'display-time as well.

[...]

> +;;; Obsolete names
> +
> +(define-obsolete-variable-alias 'display-time-world-list
> +  'world-clock-list "28.1")
> +(define-obsolete-variable-alias 'display-time-world-time-format
> +  'world-clock-time-format "28.1")
> +(define-obsolete-variable-alias 'display-time-world-buffer-name
> +  'world-clock-buffer-name "28.1")
> +(define-obsolete-variable-alias 'display-time-world-timer-enable
> +  'world-clock-timer-enable "28.1")
> +(define-obsolete-variable-alias 'display-time-world-timer-second
> +  'world-clock-timer-second "28.1")

These varaliases need to be declared before the variables they refer to.

> +(define-obsolete-function-alias 'display-time-world-mode
> +  #'world-clock-mode "28.1")
> +(define-obsolete-function-alias 'display-time-world-display
> +  #'world-clock-display "28.1")
> +(define-obsolete-function-alias 'display-time-world
> +  #'world-clock "28.1")
> +(define-obsolete-function-alias 'display-time-world-timer
> +  #'world-clock-update "28.1")
> +
> +
> +;;; World clock
> +
> +(defface world-clock-label
>    '((t :inherit font-lock-variable-name-face))
>    "Face for time zone label in `world-clock' buffer.")
>  
> -(defvar display-time-world-mode-map
> +(defvar world-clock-mode-map
>    (let ((map (make-sparse-keymap)))
> -    (define-key map "g" #'display-time-world-timer)
> +    (define-key map "g" #'world-clock-update)

Ditto re: revert-buffer-function.

>      map)
> -  "Keymap for `display-time-world-mode'.")
> +  "Keymap for `world-clock-mode'.")

[...]

> @@ -552,32 +580,30 @@ display-time-world-display
>  ;;;###autoload
>  (defun world-clock ()
>    "Show world clock buffer with times in various time zones.
> -`display-time-world-list' specifies the zones.
> +`world-clock-list' specifies the zones.
>  To turn off the world time display, go to that window and type 
> `\\[quit-window]'."
>    (interactive)
> -  (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))
> -  (pop-to-buffer display-time-world-buffer-name)
> -  (display-time-world-display (time--display-world-list))
> -  (display-time-world-mode))
> -
> -(defun display-time-world-timer ()
> +  (when (and world-clock-timer-enable
> +             (not (get-buffer world-clock-buffer-name)))
> +    (run-at-time t world-clock-timer-second 'world-clock-update))
                                               ^
Nit: Quote with #'.

> +  (pop-to-buffer world-clock-buffer-name)
> +  (world-clock-display (time--display-world-list))
> +  (world-clock-mode))

[...]

> From 137c920f5b1ab34120060af0d4e3adada0f3a8a3 Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sat, 2 May 2020 17:37:06 +0200
> Subject: [PATCH 4/4] Rearrange and cleanup code in time.el (Bug#40863)

Nit: The verb is to 'clean up'.

> * lisp/time.el (world-clock, zoneinfo-style-world-list)
> (legacy-style-world-list, world-clock-list)
> (time--display-world-list, world-clock-time-format)
> (world-clock-timer-enable, world-clock-timer-second): Move definitions
> closer to 'world-clock' code.  Remove redundant :group args.
>
> (display-time-mail-file, display-time-mail-directory)
> (display-time-mail-function)
> (display-time-default-load-average)
> (display-time-load-average-threshold, display-time-day-and-date)
> (display-time-interval, display-time-24hr-format)
> (display-time-hook, display-time-mail-face)
> (display-time-use-mail-icon, display-time-mail-string)
> (display-time-format, display-time-string-forms): Remove redundant
> :group args.

[...]

Thanks,

-- 
Basil