Package: emacs;
Reported by: Stefan Kangas <stefan <at> marxist.se>
Date: Sun, 26 Apr 2020 08:57:01 UTC
Severity: wishlist
Tags: patch
Fixed in version 28.1
Done: Stefan Kangas <stefan <at> marxist.se>
Bug is archived. No further changes may be made.
Message #67 received at 40863 <at> debbugs.gnu.org (full text, mbox):
From: "Basil L. Contovounesios" <contovob <at> tcd.ie> To: Stefan Kangas <stefan <at> marxist.se> Cc: 40863 <at> debbugs.gnu.org Subject: Re: bug#40863: [PATCH] Improve the display-time-world UI Date: Sat, 23 May 2020 14:43:18 +0100
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 <at> 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 <at> 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 <at> 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 <at> 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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.