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

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

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

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.

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

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.

Best regards,
Stefan Kangas

[-- Attachment #2: 0001-Kill-display-time-world-buffer-on-exit.patch --]
[-- Type: text/x-patch, Size: 1730 bytes --]

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)
+    (define-key map "q" 'display-time-world-exit)
+    map)
+  "Keymap for `display-time-world-mode'.")
+
 (define-derived-mode display-time-world-mode special-mode "World clock"
   "Major mode for buffer that displays times in various time zones.
 See `display-time-world'."
@@ -537,11 +544,17 @@ display-time-world-display
     (delete-char -1))
   (goto-char (point-min)))
 
+(defun display-time-world-exit ()
+  "Quit the world time buffer."
+  (interactive)
+  (quit-window t))
+
 ;;;###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]'."
   (interactive)
   (when (and display-time-world-timer-enable
              (not (get-buffer display-time-world-buffer-name)))
-- 
2.26.2


[-- Attachment #3: 0002-New-face-for-display-time-world-label.patch --]
[-- Type: text/x-patch, Size: 1399 bytes --]

From 6680705f464dd6e582e70beaf64b027ac57d0d43 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sun, 26 Apr 2020 10:16:06 +0200
Subject: [PATCH 2/4] New face for display-time-world label

* lisp/time.el (display-time-world-label): New face.
(display-time-world-display): Use the new face.
---
 lisp/time.el | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lisp/time.el b/lisp/time.el
index a5e5b9b4a7..44885f74d1 100644
--- a/lisp/time.el
+++ b/lisp/time.el
@@ -516,6 +516,10 @@ display-time-world-mode-map
     map)
   "Keymap for `display-time-world-mode'.")
 
+(defface display-time-world-label
+  '((t :inherit font-lock-variable-name-face))
+  "Face for time zone label.")
+
 (define-derived-mode display-time-world-mode special-mode "World clock"
   "Major mode for buffer that displays times in various time zones.
 See `display-time-world'."
@@ -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))))
     (delete-char -1))
   (goto-char (point-min)))
 
-- 
2.26.2


[-- Attachment #4: 0003-Move-point-to-new-buffer-for-display-time-world.patch --]
[-- Type: text/x-patch, Size: 1389 bytes --]

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))))
+  (display-time-world-mode))
 
 (defun display-time-world-timer ()
   (if (get-buffer display-time-world-buffer-name)
-- 
2.26.2


[-- Attachment #5: 0004-Improve-the-display-time-world-UI.patch --]
[-- Type: text/x-patch, Size: 1554 bytes --]

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.
(world-clock): New defalias for 'display-time-world'.
---
 lisp/time.el | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/lisp/time.el b/lisp/time.el
index 84c0071b7d..18de3990c3 100644
--- a/lisp/time.el
+++ b/lisp/time.el
@@ -195,11 +195,11 @@ display-time-world-time-format
   :type 'string
   :version "23.1")
 
-(defcustom display-time-world-buffer-name "*wclock*"
+(defcustom display-time-world-buffer-name "*World Clock*"
   "Name of the world clock buffer."
   :group 'display-time
   :type 'string
-  :version "23.1")
+  :version "28.1")
 
 (defcustom display-time-world-timer-enable t
   "If non-nil, a timer will update the world clock."
@@ -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")))
+
+;; 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

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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Sun, 26 Apr 2020 10:56:03 +0200
> 
> -(defcustom display-time-world-buffer-name "*wclock*"
> +(defcustom display-time-world-buffer-name "*World Clock*"

Is this part really necessary? what if some application or user out
there expects the old name?

Thanks.

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

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> -(defcustom display-time-world-buffer-name "*wclock*"
>> +(defcustom display-time-world-buffer-name "*World Clock*"
>
> Is this part really necessary? what if some application or user out
> there expects the old name?

Good point, I'll leave that part out.

Thanks for reviewing.

Best regards,
Stefan Kangas

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

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Kangas <stefan@marxist.se>
>> Date: Sun, 26 Apr 2020 10:56:03 +0200
>> 
>> -(defcustom display-time-world-buffer-name "*wclock*"
>> +(defcustom display-time-world-buffer-name "*World Clock*"
>
> Is this part really necessary? what if some application or user out
> there expects the old name?

If an application expects a particular name, then it's already broken
because the user can change the defcustom at any time, right?  And if a
user expects the old name, they can customize it back to the old value.

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

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: Stefan Kangas <stefan@marxist.se>,  40863@debbugs.gnu.org
> Date: Mon, 27 Apr 2020 13:20:40 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Stefan Kangas <stefan@marxist.se>
> >> Date: Sun, 26 Apr 2020 10:56:03 +0200
> >> 
> >> -(defcustom display-time-world-buffer-name "*wclock*"
> >> +(defcustom display-time-world-buffer-name "*World Clock*"
> >
> > Is this part really necessary? what if some application or user out
> > there expects the old name?
> 
> If an application expects a particular name, then it's already broken
> because the user can change the defcustom at any time, right?  And if a
> user expects the old name, they can customize it back to the old value.

So you are saying that we should change these names from time to time
just to make a point?

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

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Noam Postavsky <npostavs@gmail.com>
>> Cc: Stefan Kangas <stefan@marxist.se>,  40863@debbugs.gnu.org
>> Date: Mon, 27 Apr 2020 13:20:40 -0400
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> From: Stefan Kangas <stefan@marxist.se>
>> >> Date: Sun, 26 Apr 2020 10:56:03 +0200
>> >> 
>> >> -(defcustom display-time-world-buffer-name "*wclock*"
>> >> +(defcustom display-time-world-buffer-name "*World Clock*"
>> >
>> > Is this part really necessary? what if some application or user out
>> > there expects the old name?
>> 
>> If an application expects a particular name, then it's already broken
>> because the user can change the defcustom at any time, right?  And if a
>> user expects the old name, they can customize it back to the old value.
>
> So you are saying that we should change these names from time to time
> just to make a point?

No, I think changing the name here makes it more readable (i.e., it's
not a random change, it's an improvement).  And I think this benefit
outweighs the possible existence of broken applications, or users
possibly preferring the current value (I will admit this weighting also
somewhat depends on the probability of these hypotheticals actually
turning out to be real, which I've no real basis for; you might judge
them to be more likely than I do).

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

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: 40863@debbugs.gnu.org,  stefan@marxist.se
> Date: Mon, 27 Apr 2020 15:00:37 -0400
> 
> No, I think changing the name here makes it more readable (i.e., it's
> not a random change, it's an improvement).  And I think this benefit
> outweighs the possible existence of broken applications, or users
> possibly preferring the current value (I will admit this weighting also
> somewhat depends on the probability of these hypotheticals actually
> turning out to be real, which I've no real basis for; you might judge
> them to be more likely than I do).

I guess it's a matter of personal preferences, then.  E.g., we have
*scratch* and *xref*.

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

[Dmitry Gutov]

On 27.04.2020 22:35, Eli Zaretskii wrote:
> I guess it's a matter of personal preferences, then.  E.g., we have
> *scratch*  and*xref*.

Scratch is scratch. No longer name needed (I wouldn't know what to use).

Regarding xref, someday we'll use longer names. E.g. ones including the 
search input.

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

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

[Noam Postavsky]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 27.04.2020 22:35, Eli Zaretskii wrote:
>> I guess it's a matter of personal preferences, then.  E.g., we have
>> *scratch*  and*xref*.
>
> Scratch is scratch. No longer name needed (I wouldn't know what to use).

I guess "*Scratch*" vs "*scratch*", but I don't think that's a
significant enough difference to even look at.

> Regarding xref, someday we'll use longer names. E.g. ones including
> the search input.

Since the term "xref" is used all over in package name, command names,
documentation, etc; users already need to be familiar with the short
form.  That's not the case for "wclock".  Anyway, none of this is
especially important.  Leaving the name as it is won't do any harm.

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

[Stefan Kangas]

Noam Postavsky <npostavs@gmail.com> writes:

>>> If an application expects a particular name, then it's already broken
>>> because the user can change the defcustom at any time, right?  And if a
>>> user expects the old name, they can customize it back to the old value.
>>
>> So you are saying that we should change these names from time to time
>> just to make a point?
>
> No, I think changing the name here makes it more readable (i.e., it's
> not a random change, it's an improvement).  And I think this benefit
> outweighs the possible existence of broken applications, or users
> possibly preferring the current value

I think Noam presents a compelling argument here.

It might be somewhat minor, and it might to some degree be a matter of
preference.  But even minor improvements can add up and contribute to
the overall user experience.

Best regards,
Stefan Kangas

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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Cc: Eli Zaretskii <eliz@gnu.org>,  40863@debbugs.gnu.org
> Date: Fri, 01 May 2020 17:03:10 +0200
> 
> > No, I think changing the name here makes it more readable (i.e., it's
> > not a random change, it's an improvement).  And I think this benefit
> > outweighs the possible existence of broken applications, or users
> > possibly preferring the current value
> 
> I think Noam presents a compelling argument here.

Noam said several things about this, not just what you cite above.

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

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> > No, I think changing the name here makes it more readable (i.e., it's
>> > not a random change, it's an improvement).  And I think this benefit
>> > outweighs the possible existence of broken applications, or users
>> > possibly preferring the current value
>> 
>> I think Noam presents a compelling argument here.
>
> Noam said several things about this, not just what you cite above.

Well, I quoted the part I agree with, and tried not to quote unfairly.
Maybe I failed to include something important, but I'm not sure what
that is.

I take it that you aren't convinced?

Best regards,
Stefan Kangas

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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Cc: 40863@debbugs.gnu.org,  npostavs@gmail.com
> Date: Fri, 01 May 2020 18:26:28 +0200
> 
> >> I think Noam presents a compelling argument here.
> >
> > Noam said several things about this, not just what you cite above.
> 
> Well, I quoted the part I agree with, and tried not to quote unfairly.
> Maybe I failed to include something important, but I'm not sure what
> that is.
> 
> I take it that you aren't convinced?

My bias is towards not breaking backward compatibility unless
necessary.  Noam in effect said that he tends towards your proposal,
but the issue is not important enough.  Where does that leave us?

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

[Dmitry Gutov]

On 01.05.2020 20:57, Eli Zaretskii wrote:
> My bias is towards not breaking backward compatibility unless
> necessary.  Noam in effect said that he tends towards your proposal,
> but the issue is not important enough.  Where does that leave us?

Do we vote?

I like "World Clock".

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

[Eli Zaretskii]

> Cc: 40863@debbugs.gnu.org, npostavs@gmail.com
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 1 May 2020 21:07:38 +0300
> 
> On 01.05.2020 20:57, Eli Zaretskii wrote:
> > My bias is towards not breaking backward compatibility unless
> > necessary.  Noam in effect said that he tends towards your proposal,
> > but the issue is not important enough.  Where does that leave us?
> 
> Do we vote?

Do we have to?

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

[Stefan Kangas]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Thanks for working on this, see my comments below.

Thank you for taking the time to make a thorough review.

I agree with your suggestions and will send an updated set of patches.

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

I think this is explained in (info "(elisp) Special Properties").

Best regards,
Stefan Kangas

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

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 1679 bytes --]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

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

So here's a revised set of patches:

1. Fix all Basil's comments, and remove the buffer rename that Eli
   didn't approve of.

2. Rename 'display-time-world' to 'world-clock', and add an alias for
   the old name.

Then comes the more ambitious part:

3. Rename 'display-time-world-*' functions and variables into
   'world-clock-*' equivalents and make the old names into obsolete
   aliases.

   Move the world-clock options from the defgroup display-time
   (sorting under the mode-line and mail), into a new defgroup
   world-clock (sorting under applications).

4. Rearrange the defcustoms time.el to clearly separate world-clock
   from display-time-mode.  Cleanup of :group args.

I have seen no to the proposals in patches 1-2 so far, and I hope that
we can agree to make the changes also in patches 3-4. I think the
result is much more logical from pretty much any point of view.

Comments are obviously very welcome.

Best regards,
Stefan Kangas


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-display-time-world-UI-Bug-40863.patch --]
[-- Type: text/x-diff, Size: 2978 bytes --]

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)

* lisp/time.el (display-time-world-label): New face.
(display-time-world-display): Use the new face.  Move point to new
buffer on creation.
(display-time-world-mode-map): New variable.  Bind 'g' to update the
display-time-world buffer.
(display-time-world-timer): Add interactive spec.
---
 lisp/time.el | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/lisp/time.el b/lisp/time.el
index 44fd1a7e33..7e26c131c9 100644
--- a/lisp/time.el
+++ b/lisp/time.el
@@ -509,6 +509,16 @@ display-time-mode
 		 'display-time-event-handler)))
 
 
+(defface display-time-world-label
+  '((t :inherit font-lock-variable-name-face))
+  "Face for time zone label.")
+
+(defvar display-time-world-mode-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map "g" #'display-time-world-timer)
+    map)
+  "Keymap for `display-time-world-mode'.")
+
 (define-derived-mode display-time-world-mode special-mode "World clock"
   "Major mode for buffer that displays times in various time zones.
 See `display-time-world'."
@@ -533,7 +543,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))))
     (delete-char -1))
   (goto-char (point-min)))
 
@@ -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))
 
 (defun display-time-world-timer ()
+  (interactive)
   (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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Make-display-time-world-into-an-alias-for-world-cloc.patch --]
[-- Type: text/x-diff, Size: 5301 bytes --]

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.
---
 etc/NEWS     |  7 +++++++
 lisp/time.el | 26 ++++++++++++++------------
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 025d5c14a7..d2b745c579 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -288,6 +288,13 @@ prefix on the Subject line in various languages.
 These new navigation commands are bound to 'n' and 'p' in
 'apropos-mode'.
 
+** Time
+
+---
+*** 'display-time-world' is now an alias for '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.
+
 \f
 * New Modes and Packages in Emacs 28.1
 
diff --git a/lisp/time.el b/lisp/time.el
index 7e26c131c9..fe290ff71f 100644
--- a/lisp/time.el
+++ b/lisp/time.el
@@ -25,8 +25,7 @@
 ;; Facilities to display current time/date and a new-mail indicator
 ;; in the Emacs mode line.  The entry point is `display-time'.
 
-;; Display time world in a buffer, the entry point is
-;; `display-time-world'.
+;; Use `world-clock' to display world clock in a buffer.
 
 ;;; Code:
 
@@ -130,7 +129,7 @@ zoneinfo-style-world-list
     ("Europe/Paris" "Paris")
     ("Asia/Calcutta" "Bangalore")
     ("Asia/Tokyo" "Tokyo"))
-  "Alist of zoneinfo-style time zones and places for `display-time-world'.
+  "Alist of zoneinfo-style time zones and places for `world-clock'.
 Each element has the form (TIMEZONE LABEL).
 TIMEZONE should be a string of the form AREA/LOCATION, where AREA is
 the name of a region -- a continent or ocean, and LOCATION is the name
@@ -147,7 +146,7 @@ legacy-style-world-list
     ("CET-1CDT" "Paris")
     ("IST-5:30" "Bangalore")
     ("JST-9" "Tokyo"))
-  "Alist of traditional-style time zones and places for `display-time-world'.
+  "Alist of traditional-style time zones and places for `world-clock'.
 Each element has the form (TIMEZONE LABEL).
 TIMEZONE should be a string of the form:
 
@@ -161,7 +160,7 @@ legacy-style-world-list
   :version "23.1")
 
 (defcustom display-time-world-list t
-  "Alist of time zones and places for `display-time-world' to display.
+  "Alist of time zones and places for `world-clock' to display.
 Each element has the form (TIMEZONE LABEL).
 TIMEZONE should be in a format supported by your system.  See the
 documentation of `zoneinfo-style-world-list' and
@@ -190,25 +189,25 @@ time--display-world-list
 	zoneinfo-style-world-list))))
 
 (defcustom display-time-world-time-format "%A %d %B %R %Z"
-  "Format of the time displayed, see `format-time-string'."
+  "Time format for `world-clock', see `format-time-string'."
   :group 'display-time
   :type 'string
   :version "23.1")
 
 (defcustom display-time-world-buffer-name "*wclock*"
-  "Name of the world clock buffer."
+  "Name of the `world-clock' buffer."
   :group 'display-time
   :type 'string
   :version "23.1")
 
 (defcustom display-time-world-timer-enable t
-  "If non-nil, a timer will update the world clock."
+  "If non-nil, a timer will update the `world-clock' buffer."
   :group 'display-time
   :type 'boolean
   :version "23.1")
 
 (defcustom display-time-world-timer-second 60
-  "Interval in seconds for updating the world clock."
+  "Interval in seconds for updating the `world-clock' buffer."
   :group 'display-time
   :type 'integer
   :version "23.1")
@@ -511,7 +510,7 @@ display-time-mode
 
 (defface display-time-world-label
   '((t :inherit font-lock-variable-name-face))
-  "Face for time zone label.")
+  "Face for time zone label in `world-clock' buffer.")
 
 (defvar display-time-world-mode-map
   (let ((map (make-sparse-keymap)))
@@ -551,8 +550,8 @@ display-time-world-display
   (goto-char (point-min)))
 
 ;;;###autoload
-(defun display-time-world ()
-  "Enable updating display of times in various time zones.
+(defun world-clock ()
+  "Show world clock buffer with 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 `\\[quit-window]'."
   (interactive)
@@ -564,6 +563,7 @@ display-time-world
   (display-time-world-mode))
 
 (defun display-time-world-timer ()
+  "Update the `world-clock' buffer."
   (interactive)
   (if (get-buffer display-time-world-buffer-name)
       (with-current-buffer (get-buffer display-time-world-buffer-name)
@@ -576,6 +576,8 @@ display-time-world-timer
 		       "display-time-world-timer")
             (cancel-timer elt)))))))
 
+(defalias 'display-time-world #'world-clock)
+
 ;;;###autoload
 (defun emacs-uptime (&optional format)
   "Return a string giving the uptime of this instance of Emacs.
-- 
2.26.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Rename-display-time-world-to-world-clock-Bug-40863.patch --]
[-- Type: text/x-diff, Size: 10583 bytes --]

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)

* lisp/time.el (world-clock): New defgroup.
(zoneinfo-style-world-list, legacy-style-world-list): Use :group
'world-clock'.
(world-clock-list, world-clock-time-format)
(world-clock-buffer-name, world-clock-timer-enable)
(world-clock-timer-second): Rename from 'display-time-world-*'.
Use :group 'world-clock'.
(world-clock-label, world-clock-mode-map, world-clock-mode)
(world-clock-display, world-clock): Rename from
'display-time-world-*'.  Use the new names.
(time--display-world-list): Use the above new names.

(display-time-world-list, display-time-world-time-format)
(display-time-world-buffer-name)
(display-time-world-timer-enable)
(display-time-world-timer-second): Redefine as obsolete variable
aliases for the new 'world-clock-*' names.

(display-time-world-mode, display-time-world-display)
(display-time-world, display-time-world-timer): Redefine as
obsolete function aliases for the new 'world-clock-*' names.

* etc/NEWS: Announce the above changes.
---
 etc/NEWS     |  21 +++++++++-
 lisp/time.el | 114 +++++++++++++++++++++++++++++++--------------------
 2 files changed, 89 insertions(+), 46 deletions(-)

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.
+
+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 old names are now obsolete.
 
 \f
 * 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."
+  :group 'applications)
+
 (defcustom zoneinfo-style-world-list
   '(("America/Los_Angeles" "Seattle")
     ("America/New_York" "New York")
@@ -135,7 +139,7 @@ zoneinfo-style-world-list
 the name of a region -- a continent or ocean, and LOCATION is the name
 of a specific location, e.g., a city, within that region.
 LABEL is a string to display as the label of that TIMEZONE's time."
-  :group 'display-time
+  :group 'world-clock
   :type '(repeat (list string string))
   :version "23.1")
 
@@ -155,11 +159,11 @@ legacy-style-world-list
 See the documentation of the TZ environment variable on your system,
 for more details about the format of TIMEZONE.
 LABEL is a string to display as the label of that TIMEZONE's time."
-  :group 'display-time
+  :group 'world-clock
   :type '(repeat (list string string))
   :version "23.1")
 
-(defcustom display-time-world-list t
+(defcustom world-clock-list t
   "Alist of time zones and places for `world-clock' to display.
 Each element has the form (TIMEZONE LABEL).
 TIMEZONE should be in a format supported by your system.  See the
@@ -170,16 +174,15 @@ display-time-world-list
 If the value is t instead of an alist, use the value of
 `zoneinfo-style-world-list' if it works on this platform, and of
 `legacy-style-world-list' otherwise."
-
-  :group 'display-time
+  :group 'world-clock
   :type '(choice (const :tag "Default" t)
                  (repeat :tag "List of zones and labels"
                          (list (string :tag "Zone") (string :tag "Label"))))
-  :version "23.1")
+  :version "28.1")
 
 (defun time--display-world-list ()
-  (if (listp display-time-world-list)
-      display-time-world-list
+  (if (listp world-clock-list)
+      world-clock-list
     ;; Determine if zoneinfo style timezones are supported by testing that
     ;; America/New York and Europe/London return different timezones.
     (let ((nyt (format-time-string "%z" nil "America/New_York"))
@@ -188,29 +191,29 @@ time--display-world-list
 	  legacy-style-world-list
 	zoneinfo-style-world-list))))
 
-(defcustom display-time-world-time-format "%A %d %B %R %Z"
+(defcustom world-clock-time-format "%A %d %B %R %Z"
   "Time format for `world-clock', see `format-time-string'."
-  :group 'display-time
+  :group 'world-clock
   :type 'string
-  :version "23.1")
+  :version "28.1")
 
-(defcustom display-time-world-buffer-name "*wclock*"
+(defcustom world-clock-buffer-name "*wclock*"
   "Name of the `world-clock' buffer."
-  :group 'display-time
+  :group 'world-clock
   :type 'string
-  :version "23.1")
+  :version "28.1")
 
-(defcustom display-time-world-timer-enable t
+(defcustom world-clock-timer-enable t
   "If non-nil, a timer will update the `world-clock' buffer."
-  :group 'display-time
+  :group 'world-clock
   :type 'boolean
-  :version "23.1")
+  :version "28.1")
 
-(defcustom display-time-world-timer-second 60
+(defcustom world-clock-timer-second 60
   "Interval in seconds for updating the `world-clock' buffer."
-  :group 'display-time
+  :group 'world-clock
   :type 'integer
-  :version "23.1")
+  :version "28.1")
 
 ;;;###autoload
 (defun display-time ()
@@ -507,23 +510,48 @@ display-time-mode
     (remove-hook 'rmail-after-get-new-mail-hook
 		 'display-time-event-handler)))
 
-
-(defface display-time-world-label
+\f
+;;; 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")
+
+(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")
+
+\f
+;;; 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)
     map)
-  "Keymap for `display-time-world-mode'.")
+  "Keymap for `world-clock-mode'.")
 
-(define-derived-mode display-time-world-mode special-mode "World clock"
+(define-derived-mode world-clock-mode special-mode "World clock"
   "Major mode for buffer that displays times in various time zones.
 See `display-time-world'."
   (setq show-trailing-whitespace nil))
 
-(defun display-time-world-display (alist)
+(defun world-clock-display (alist)
   "Replace current buffer text with times in various zones, based on ALIST."
   (let ((inhibit-read-only t)
 	(buffer-undo-list t)
@@ -535,7 +563,7 @@ display-time-world-display
       (let* ((label (cadr zone))
 	     (width (string-width label)))
 	(push (cons label
-		    (format-time-string display-time-world-time-format
+		    (format-time-string world-clock-time-format
 					now (car zone)))
 	      result)
 	(when (> width max-width)
@@ -544,7 +572,7 @@ display-time-world-display
     (dolist (timedata (nreverse result))
       (insert (format fmt
                       (propertize (car timedata)
-                                  'face 'display-time-world-label)
+                                  'face 'world-clock-label)
                       (cdr timedata))))
     (delete-char -1))
   (goto-char (point-min)))
@@ -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))
+  (pop-to-buffer world-clock-buffer-name)
+  (world-clock-display (time--display-world-list))
+  (world-clock-mode))
+
+(defun world-clock-update ()
   "Update the `world-clock' buffer."
   (interactive)
-  (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)))
+  (if (get-buffer world-clock-buffer-name)
+      (with-current-buffer (get-buffer world-clock-buffer-name)
+        (world-clock-display (time--display-world-list)))
     ;; cancel timer
     (let ((list timer-list))
       (while list
         (let ((elt (pop list)))
           (when (equal (symbol-name (timer--function elt))
-		       "display-time-world-timer")
+		       "world-clock-update")
             (cancel-timer elt)))))))
 
-(defalias 'display-time-world #'world-clock)
-
 ;;;###autoload
 (defun emacs-uptime (&optional format)
   "Return a string giving the uptime of this instance of Emacs.
-- 
2.26.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Rearrange-and-cleanup-code-in-time.el-Bug-40863.patch --]
[-- Type: text/x-diff, Size: 12746 bytes --]

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)

* 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.
---
 lisp/time.el | 216 +++++++++++++++++++++++----------------------------
 1 file changed, 97 insertions(+), 119 deletions(-)

diff --git a/lisp/time.el b/lisp/time.el
index 3f1880b708..e678254b54 100644
--- a/lisp/time.el
+++ b/lisp/time.el
@@ -34,23 +34,20 @@ display-time
   :group 'mode-line
   :group 'mail)
 
-
 (defcustom display-time-mail-file nil
   "File name of mail inbox file, for indicating existence of new mail.
 Non-nil and not a string means don't check for mail; nil means use
 default, which is system-dependent, and is the same as used by Rmail."
   :type '(choice (const :tag "None" none)
 		 (const :tag "Default" nil)
-		 (file :format "%v"))
-  :group 'display-time)
+		 (file :format "%v")))
 
 (defcustom display-time-mail-directory nil
   "Name of mail inbox directory, for indicating existence of new mail.
 Any nonempty regular file in the directory is regarded as newly arrived mail.
 If nil, do not check a directory for arriving mail."
   :type '(choice (const :tag "None" nil)
-		 (directory :format "%v"))
-  :group 'display-time)
+		 (directory :format "%v")))
 
 (defcustom display-time-mail-function nil
   "Function to call, for indicating existence of new mail.
@@ -58,8 +55,7 @@ display-time-mail-function
 specified by `display-time-mail-file' is nonempty or that the
 directory `display-time-mail-directory' contains nonempty files."
   :type '(choice (const :tag "Default" nil)
-		 (function))
-  :group 'display-time)
+		 (function)))
 
 (defcustom display-time-default-load-average 0
   "Which load average value will be shown in the mode line.
@@ -74,8 +70,7 @@ display-time-default-load-average
   :type '(choice (const :tag "1 minute load" 0)
 		 (const :tag "5 minutes load" 1)
 		 (const :tag "15 minutes load" 2)
-		 (const :tag "None" nil))
-  :group 'display-time)
+		 (const :tag "None" nil)))
 
 (defvar display-time-load-average nil
   "Value of the system's load average currently shown on the mode line.
@@ -85,27 +80,23 @@ display-time-load-average
 
 (defcustom display-time-load-average-threshold 0.1
   "Load-average values below this value won't be shown in the mode line."
-  :type 'number
-  :group 'display-time)
+  :type 'number)
 
 ;;;###autoload
 (defcustom display-time-day-and-date nil "\
 Non-nil means \\[display-time] should display day and date as well as time."
-  :type 'boolean
-  :group 'display-time)
+  :type 'boolean)
 
 (defvar display-time-timer nil)
 
 (defcustom display-time-interval 60
   "Seconds between updates of time in the mode line."
-  :type 'integer
-  :group 'display-time)
+  :type 'integer)
 
 (defcustom display-time-24hr-format nil
   "Non-nil indicates time should be displayed as hh:mm, 0 <= hh <= 23.
 A value of nil means 1 <= hh <= 12, and an AM/PM suffix is used."
-  :type 'boolean
-  :group 'display-time)
+  :type 'boolean)
 
 (defvar display-time-string nil
   "String used in mode lines to display a time string.
@@ -115,106 +106,12 @@ display-time-string
 
 (defcustom display-time-hook nil
   "List of functions to be called when the time is updated on the mode line."
-  :type 'hook
-  :group 'display-time)
+  :type 'hook)
 
 (defvar display-time-server-down-time nil
    "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."
-  :group 'applications)
-
-(defcustom zoneinfo-style-world-list
-  '(("America/Los_Angeles" "Seattle")
-    ("America/New_York" "New York")
-    ("Europe/London" "London")
-    ("Europe/Paris" "Paris")
-    ("Asia/Calcutta" "Bangalore")
-    ("Asia/Tokyo" "Tokyo"))
-  "Alist of zoneinfo-style time zones and places for `world-clock'.
-Each element has the form (TIMEZONE LABEL).
-TIMEZONE should be a string of the form AREA/LOCATION, where AREA is
-the name of a region -- a continent or ocean, and LOCATION is the name
-of a specific location, e.g., a city, within that region.
-LABEL is a string to display as the label of that TIMEZONE's time."
-  :group 'world-clock
-  :type '(repeat (list string string))
-  :version "23.1")
-
-(defcustom legacy-style-world-list
-  '(("PST8PDT" "Seattle")
-    ("EST5EDT" "New York")
-    ("GMT0BST" "London")
-    ("CET-1CDT" "Paris")
-    ("IST-5:30" "Bangalore")
-    ("JST-9" "Tokyo"))
-  "Alist of traditional-style time zones and places for `world-clock'.
-Each element has the form (TIMEZONE LABEL).
-TIMEZONE should be a string of the form:
-
-     std[+|-]offset[dst[offset][,date[/time],date[/time]]]
-
-See the documentation of the TZ environment variable on your system,
-for more details about the format of TIMEZONE.
-LABEL is a string to display as the label of that TIMEZONE's time."
-  :group 'world-clock
-  :type '(repeat (list string string))
-  :version "23.1")
-
-(defcustom world-clock-list t
-  "Alist of time zones and places for `world-clock' to display.
-Each element has the form (TIMEZONE LABEL).
-TIMEZONE should be in a format supported by your system.  See the
-documentation of `zoneinfo-style-world-list' and
-`legacy-style-world-list' for two widely used formats.  LABEL is
-a string to display as the label of that TIMEZONE's time.
-
-If the value is t instead of an alist, use the value of
-`zoneinfo-style-world-list' if it works on this platform, and of
-`legacy-style-world-list' otherwise."
-  :group 'world-clock
-  :type '(choice (const :tag "Default" t)
-                 (repeat :tag "List of zones and labels"
-                         (list (string :tag "Zone") (string :tag "Label"))))
-  :version "28.1")
-
-(defun time--display-world-list ()
-  (if (listp world-clock-list)
-      world-clock-list
-    ;; Determine if zoneinfo style timezones are supported by testing that
-    ;; America/New York and Europe/London return different timezones.
-    (let ((nyt (format-time-string "%z" nil "America/New_York"))
-	  (gmt (format-time-string "%z" nil "Europe/London")))
-      (if (string-equal nyt gmt)
-	  legacy-style-world-list
-	zoneinfo-style-world-list))))
-
-(defcustom world-clock-time-format "%A %d %B %R %Z"
-  "Time format for `world-clock', see `format-time-string'."
-  :group 'world-clock
-  :type 'string
-  :version "28.1")
-
-(defcustom world-clock-buffer-name "*wclock*"
-  "Name of the `world-clock' buffer."
-  :group 'world-clock
-  :type 'string
-  :version "28.1")
-
-(defcustom world-clock-timer-enable t
-  "If non-nil, a timer will update the `world-clock' buffer."
-  :group 'world-clock
-  :type 'boolean
-  :version "28.1")
-
-(defcustom world-clock-timer-second 60
-  "Interval in seconds for updating the `world-clock' buffer."
-  :group 'world-clock
-  :type 'integer
-  :version "28.1")
-
 ;;;###autoload
 (defun display-time ()
   "Enable display of time, load level, and mail flag in mode lines.
@@ -236,7 +133,6 @@ display-time-mail-face
 background color is the background of this face.  Set this to
 make the mail indicator stand out on a color display."
   :group 'mode-line-faces
-  :group 'display-time
   :version "22.1"
   :type '(choice (const :tag "None" nil) face))
 
@@ -251,14 +147,12 @@ display-time-use-mail-icon
   "Non-nil means use an icon as mail indicator on a graphic display.
 Otherwise use `display-time-mail-string'.  The icon may consume less
 of the mode line.  It is specified by `display-time-mail-icon'."
-  :group 'display-time
   :type 'boolean)
 
 ;; Fixme: maybe default to the character if we can display Unicode.
 (defcustom display-time-mail-string "Mail"
   "String to use as the mail indicator in `display-time-string-forms'.
 This can use the Unicode letter character if you can display it."
-  :group 'display-time
   :version "22.1"
   :type '(choice (const "Mail")
 		 ;; Use :tag here because the Lucid menu won't display
@@ -272,8 +166,7 @@ display-time-format
 how to write this string.  If this is nil, the defaults
 depend on `display-time-day-and-date' and `display-time-24hr-format'."
   :type '(choice (const :tag "Default" nil)
-		 string)
-  :group 'display-time)
+		 string))
 
 (defcustom display-time-string-forms
   '((if (and (not display-time-format) display-time-day-and-date)
@@ -327,8 +220,7 @@ display-time-string-forms
     (if mail \" Mail\" \"\"))
 
 would give mode line times like `94/12/30 21:07:48 (UTC)'."
-  :type '(repeat sexp)
-  :group 'display-time)
+  :type '(repeat sexp))
 
 (defun display-time-event-handler ()
   (display-time-update)
@@ -536,6 +428,92 @@ 'display-time-world-timer
 \f
 ;;; World clock
 
+(defgroup world-clock nil
+  "Show a world clock."
+  :group 'applications)
+
+(defcustom zoneinfo-style-world-list
+  '(("America/Los_Angeles" "Seattle")
+    ("America/New_York" "New York")
+    ("Europe/London" "London")
+    ("Europe/Paris" "Paris")
+    ("Asia/Calcutta" "Bangalore")
+    ("Asia/Tokyo" "Tokyo"))
+  "Alist of zoneinfo-style time zones and places for `world-clock'.
+Each element has the form (TIMEZONE LABEL).
+TIMEZONE should be a string of the form AREA/LOCATION, where AREA is
+the name of a region -- a continent or ocean, and LOCATION is the name
+of a specific location, e.g., a city, within that region.
+LABEL is a string to display as the label of that TIMEZONE's time."
+  :type '(repeat (list string string))
+  :version "23.1")
+
+(defcustom legacy-style-world-list
+  '(("PST8PDT" "Seattle")
+    ("EST5EDT" "New York")
+    ("GMT0BST" "London")
+    ("CET-1CDT" "Paris")
+    ("IST-5:30" "Bangalore")
+    ("JST-9" "Tokyo"))
+  "Alist of traditional-style time zones and places for `world-clock'.
+Each element has the form (TIMEZONE LABEL).
+TIMEZONE should be a string of the form:
+
+     std[+|-]offset[dst[offset][,date[/time],date[/time]]]
+
+See the documentation of the TZ environment variable on your system,
+for more details about the format of TIMEZONE.
+LABEL is a string to display as the label of that TIMEZONE's time."
+  :type '(repeat (list string string))
+  :version "23.1")
+
+(defcustom world-clock-list t
+  "Alist of time zones and places for `world-clock' to display.
+Each element has the form (TIMEZONE LABEL).
+TIMEZONE should be in a format supported by your system.  See the
+documentation of `zoneinfo-style-world-list' and
+`legacy-style-world-list' for two widely used formats.  LABEL is
+a string to display as the label of that TIMEZONE's time.
+
+If the value is t instead of an alist, use the value of
+`zoneinfo-style-world-list' if it works on this platform, and of
+`legacy-style-world-list' otherwise."
+  :type '(choice (const :tag "Default" t)
+                 (repeat :tag "List of zones and labels"
+                         (list (string :tag "Zone") (string :tag "Label"))))
+  :version "28.1")
+
+(defun time--display-world-list ()
+  (if (listp world-clock-list)
+      world-clock-list
+    ;; Determine if zoneinfo style timezones are supported by testing that
+    ;; America/New York and Europe/London return different timezones.
+    (let ((nyt (format-time-string "%z" nil "America/New_York"))
+	  (gmt (format-time-string "%z" nil "Europe/London")))
+      (if (string-equal nyt gmt)
+	  legacy-style-world-list
+	zoneinfo-style-world-list))))
+
+(defcustom world-clock-time-format "%A %d %B %R %Z"
+  "Time format for `world-clock', see `format-time-string'."
+  :type 'string
+  :version "28.1")
+
+(defcustom world-clock-buffer-name "*wclock*"
+  "Name of the `world-clock' buffer."
+  :type 'string
+  :version "28.1")
+
+(defcustom world-clock-timer-enable t
+  "If non-nil, a timer will update the `world-clock' buffer."
+  :type 'boolean
+  :version "28.1")
+
+(defcustom world-clock-timer-second 60
+  "Interval in seconds for updating the `world-clock' buffer."
+  :type 'integer
+  :version "28.1")
+
 (defface world-clock-label
   '((t :inherit font-lock-variable-name-face))
   "Face for time zone label in `world-clock' buffer.")
-- 
2.26.2

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

[Stefan Kangas]

Stefan Kangas <stefan@marxist.se> writes:

> 4. Rearrange the defcustoms time.el to clearly separate world-clock
                             ^ in
>    from display-time-mode.  Cleanup of :group args.
>
> I have seen no to the proposals in patches 1-2 so far, and I hope that
                ^ objections
> we can agree to make the changes also in patches 3-4. I think the
> result is much more logical from pretty much any point of view.

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.
>  
>  \f
>  * 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")
> +
> +\f
> +;;; 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

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

[Basil L. Contovounesios]

Stefan Kangas <stefan@marxist.se> writes:

>> Just curious: when do we use 'face' and when 'font-lock-face'?
>
> I think this is explained in (info "(elisp) Special Properties").

I'm aware of the difference between the two properties.  My question is,
when do we prefer one or the other?  My impression is that
font-lock-face should be preferred wherever it makes sense, as its
display can be toggled.

Not that I'm arguing for switching to font-lock-face in this case; that
could be done another time.  I was just wondering out loud.

-- 
Basil

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

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 357 bytes --]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

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

Thank you for the detailed review!  I believe I have fixed all your
comments in the attached patches.  (Note that I squashed patch 2 and 3
into one patch.)

Please let me know if there are any further comments.

Best regards,
Stefan Kangas

[-- Attachment #2: 0001-Improve-display-time-world-UI-Bug-40863.patch --]
[-- Type: text/x-diff, Size: 2758 bytes --]

From 853b9359a91efec0c10b22d49645f5d0ff94b17c 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/3] Improve display-time-world UI (Bug#40863)

* lisp/time.el (display-time-world-mode): Set 'revert-buffer-function'
to 'display-time-world-timer'.

(display-time-world-label): New face.
(display-time-world-display): Use the new face.  Move point to new
buffer on creation and resize.
---
 lisp/time.el | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/lisp/time.el b/lisp/time.el
index 44fd1a7e33..75526a22e0 100644
--- a/lisp/time.el
+++ b/lisp/time.el
@@ -509,9 +509,14 @@ display-time-mode
 		 'display-time-event-handler)))
 
 
+(defface display-time-world-label
+  '((t :inherit font-lock-variable-name-face))
+  "Face for time zone label.")
+
 (define-derived-mode display-time-world-mode special-mode "World clock"
   "Major mode for buffer that displays times in various time zones.
 See `display-time-world'."
+  (setq revert-buffer-function 'display-time-world-timer)
   (setq show-trailing-whitespace nil))
 
 (defun display-time-world-display (alist)
@@ -533,7 +538,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))))
     (delete-char -1))
   (goto-char (point-min)))
 
@@ -541,16 +549,15 @@ 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)
+  (fit-window-to-buffer))
 
 (defun display-time-world-timer ()
   (if (get-buffer display-time-world-buffer-name)
-- 
2.27.0


[-- Attachment #3: 0002-Rename-display-time-world-to-world-clock-Bug-40863.patch --]
[-- Type: text/x-diff, Size: 11632 bytes --]

From 63c9882139e433359805debeedc35fd9be75f704 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/3] Rename 'display-time-world' to 'world-clock' (Bug#40863)

* lisp/time.el (world-clock-list, world-clock-time-format)
(world-clock-buffer-name, world-clock-timer-enable)
(world-clock-timer-second, world-clock-label, world-clock-mode)
(world-clock-display, world-clock, world-clock-update): Rename
from 'display-time-world-*' and update all uses.

(world-clock): New defgroup.
(zoneinfo-style-world-list, legacy-style-world-list): Use :group
'world-clock'.

(display-time-world-list, display-time-world-time-format)
(display-time-world-buffer-name)
(display-time-world-timer-enable)
(display-time-world-timer-second): Make into obsolete variable
aliases for the new names.

* lisp/time.el (display-time-world-mode)
(display-time-world-display, display-time-world)
(display-time-world-timer): Make into obsolete function aliases
for the new names.

* etc/NEWS: Announce the above changes.
---
 etc/NEWS     |  25 ++++++++++
 lisp/time.el | 130 +++++++++++++++++++++++++++++++--------------------
 2 files changed, 104 insertions(+), 51 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 850b166069..630b9ecf19 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -697,6 +697,31 @@ custom rules, see the variables 'bug-reference-setup-from-vc-alist',
 It's bound to the 'C-c C-c f' keystroke, and prompts for a local file
 name.
 
+** Time
+
+---
+*** 'display-time-world' has been renamed to 'world-clock'.
+'world-clock' creates a buffer with an updating time display using
+several time zones.  It is hoped that the new names are more
+discoverable.
+
+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-name'
+  'display-time-world-timer-enable' to 'world-clock-timer-enable'
+  'display-time-world-timer-second' to 'world-clock-timer-second'
+
+The old names are now obsolete.
+
 \f
 * New Modes and Packages in Emacs 28.1
 
diff --git a/lisp/time.el b/lisp/time.el
index 75526a22e0..7cff1e6b45 100644
--- a/lisp/time.el
+++ b/lisp/time.el
@@ -25,8 +25,7 @@
 ;; Facilities to display current time/date and a new-mail indicator
 ;; in the Emacs mode line.  The entry point is `display-time'.
 
-;; Display time world in a buffer, the entry point is
-;; `display-time-world'.
+;; Use `world-clock' to display world clock in a buffer.
 
 ;;; Code:
 
@@ -123,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
+  "Display a world clock."
+  :group 'display-time)
+
 (defcustom zoneinfo-style-world-list
   '(("America/Los_Angeles" "Seattle")
     ("America/New_York" "New York")
@@ -130,13 +133,13 @@ zoneinfo-style-world-list
     ("Europe/Paris" "Paris")
     ("Asia/Calcutta" "Bangalore")
     ("Asia/Tokyo" "Tokyo"))
-  "Alist of zoneinfo-style time zones and places for `display-time-world'.
+  "Alist of zoneinfo-style time zones and places for `world-clock'.
 Each element has the form (TIMEZONE LABEL).
 TIMEZONE should be a string of the form AREA/LOCATION, where AREA is
 the name of a region -- a continent or ocean, and LOCATION is the name
 of a specific location, e.g., a city, within that region.
 LABEL is a string to display as the label of that TIMEZONE's time."
-  :group 'display-time
+  :group 'world-clock
   :type '(repeat (list string string))
   :version "23.1")
 
@@ -147,7 +150,7 @@ legacy-style-world-list
     ("CET-1CDT" "Paris")
     ("IST-5:30" "Bangalore")
     ("JST-9" "Tokyo"))
-  "Alist of traditional-style time zones and places for `display-time-world'.
+  "Alist of traditional-style time zones and places for `world-clock'.
 Each element has the form (TIMEZONE LABEL).
 TIMEZONE should be a string of the form:
 
@@ -156,12 +159,12 @@ legacy-style-world-list
 See the documentation of the TZ environment variable on your system,
 for more details about the format of TIMEZONE.
 LABEL is a string to display as the label of that TIMEZONE's time."
-  :group 'display-time
+  :group 'world-clock
   :type '(repeat (list string string))
   :version "23.1")
 
-(defcustom display-time-world-list t
-  "Alist of time zones and places for `display-time-world' to display.
+(defcustom world-clock-list t
+  "Alist of time zones and places for `world-clock' to display.
 Each element has the form (TIMEZONE LABEL).
 TIMEZONE should be in a format supported by your system.  See the
 documentation of `zoneinfo-style-world-list' and
@@ -171,16 +174,15 @@ display-time-world-list
 If the value is t instead of an alist, use the value of
 `zoneinfo-style-world-list' if it works on this platform, and of
 `legacy-style-world-list' otherwise."
-
-  :group 'display-time
+  :group 'world-clock
   :type '(choice (const :tag "Default" t)
                  (repeat :tag "List of zones and labels"
                          (list (string :tag "Zone") (string :tag "Label"))))
-  :version "23.1")
+  :version "28.1")
 
 (defun time--display-world-list ()
-  (if (listp display-time-world-list)
-      display-time-world-list
+  (if (listp world-clock-list)
+      world-clock-list
     ;; Determine if zoneinfo style timezones are supported by testing that
     ;; America/New York and Europe/London return different timezones.
     (let ((nyt (format-time-string "%z" nil "America/New_York"))
@@ -189,29 +191,29 @@ time--display-world-list
 	  legacy-style-world-list
 	zoneinfo-style-world-list))))
 
-(defcustom display-time-world-time-format "%A %d %B %R %Z"
-  "Format of the time displayed, see `format-time-string'."
-  :group 'display-time
+(defcustom world-clock-time-format "%A %d %B %R %Z"
+  "Time format for `world-clock', see `format-time-string'."
+  :group 'world-clock
   :type 'string
-  :version "23.1")
+  :version "28.1")
 
-(defcustom display-time-world-buffer-name "*wclock*"
-  "Name of the world clock buffer."
-  :group 'display-time
+(defcustom world-clock-buffer-name "*wclock*"
+  "Name of the `world-clock' buffer."
+  :group 'world-clock
   :type 'string
-  :version "23.1")
+  :version "28.1")
 
-(defcustom display-time-world-timer-enable t
-  "If non-nil, a timer will update the world clock."
-  :group 'display-time
+(defcustom world-clock-timer-enable t
+  "If non-nil, a timer will update the `world-clock' buffer."
+  :group 'world-clock
   :type 'boolean
-  :version "23.1")
+  :version "28.1")
 
-(defcustom display-time-world-timer-second 60
-  "Interval in seconds for updating the world clock."
-  :group 'display-time
+(defcustom world-clock-timer-second 60
+  "Interval in seconds for updating the `world-clock' buffer."
+  :group 'world-clock
   :type 'integer
-  :version "23.1")
+  :version "28.1")
 
 ;;;###autoload
 (defun display-time ()
@@ -508,18 +510,43 @@ display-time-mode
     (remove-hook 'rmail-after-get-new-mail-hook
 		 'display-time-event-handler)))
 
-
-(defface display-time-world-label
+\f
+;;; 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")
+
+(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")
+
+\f
+;;; World clock
+
+(defface world-clock-label
   '((t :inherit font-lock-variable-name-face))
-  "Face for time zone label.")
+  "Face for time zone label in `world-clock' buffer.")
 
-(define-derived-mode display-time-world-mode special-mode "World clock"
+(define-derived-mode world-clock-mode special-mode "World clock"
   "Major mode for buffer that displays times in various time zones.
 See `display-time-world'."
   (setq revert-buffer-function 'display-time-world-timer)
   (setq show-trailing-whitespace nil))
 
-(defun display-time-world-display (alist)
+(defun world-clock-display (alist)
   "Replace current buffer text with times in various zones, based on ALIST."
   (let ((inhibit-read-only t)
 	(buffer-undo-list t)
@@ -531,7 +558,7 @@ display-time-world-display
       (let* ((label (cadr zone))
 	     (width (string-width label)))
 	(push (cons label
-		    (format-time-string display-time-world-time-format
+		    (format-time-string world-clock-time-format
 					now (car zone)))
 	      result)
 	(when (> width max-width)
@@ -540,35 +567,36 @@ display-time-world-display
     (dolist (timedata (nreverse result))
       (insert (format fmt
                       (propertize (car timedata)
-                                  'face 'display-time-world-label)
+                                  'face 'world-clock-label)
                       (cdr timedata))))
     (delete-char -1))
   (goto-char (point-min)))
 
 ;;;###autoload
-(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 `\\[quit-window]'."
+(defun world-clock ()
+  "Display a world clock buffer with times in various time zones.
+The variable `world-clock-list' specifies which time zones to use.
+To turn off the world time display, go to the 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)
+  (when (and world-clock-timer-enable
+             (not (get-buffer world-clock-buffer-name)))
+    (run-at-time t world-clock-timer-second #'world-clock-update))
+  (pop-to-buffer world-clock-buffer-name)
+  (world-clock-display (time--display-world-list))
+  (world-clock-mode)
   (fit-window-to-buffer))
 
-(defun display-time-world-timer ()
-  (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)))
+(defun world-clock-update ()
+  "Update the `world-clock' buffer."
+  (if (get-buffer world-clock-buffer-name)
+      (with-current-buffer (get-buffer world-clock-buffer-name)
+        (world-clock-display (time--display-world-list)))
     ;; cancel timer
     (let ((list timer-list))
       (while list
         (let ((elt (pop list)))
           (when (equal (symbol-name (timer--function elt))
-		       "display-time-world-timer")
+		       "world-clock-update")
             (cancel-timer elt)))))))
 
 ;;;###autoload
-- 
2.27.0


[-- Attachment #4: 0003-Rearrange-and-clean-up-code-in-time.el-Bug-40863.patch --]
[-- Type: text/x-diff, Size: 9212 bytes --]

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

* 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.
---
 lisp/time.el | 123 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 97 insertions(+), 26 deletions(-)

diff --git a/lisp/time.el b/lisp/time.el
index 7cff1e6b45..7bedd2ad9a 100644
--- a/lisp/time.el
+++ b/lisp/time.el
@@ -34,23 +34,20 @@ display-time
   :group 'mode-line
   :group 'mail)
 
-
 (defcustom display-time-mail-file nil
   "File name of mail inbox file, for indicating existence of new mail.
 Non-nil and not a string means don't check for mail; nil means use
 default, which is system-dependent, and is the same as used by Rmail."
   :type '(choice (const :tag "None" none)
 		 (const :tag "Default" nil)
-		 (file :format "%v"))
-  :group 'display-time)
+		 (file :format "%v")))
 
 (defcustom display-time-mail-directory nil
   "Name of mail inbox directory, for indicating existence of new mail.
 Any nonempty regular file in the directory is regarded as newly arrived mail.
 If nil, do not check a directory for arriving mail."
   :type '(choice (const :tag "None" nil)
-		 (directory :format "%v"))
-  :group 'display-time)
+		 (directory :format "%v")))
 
 (defcustom display-time-mail-function nil
   "Function to call, for indicating existence of new mail.
@@ -58,8 +55,7 @@ display-time-mail-function
 specified by `display-time-mail-file' is nonempty or that the
 directory `display-time-mail-directory' contains nonempty files."
   :type '(choice (const :tag "Default" nil)
-		 (function))
-  :group 'display-time)
+		 (function)))
 
 (defcustom display-time-default-load-average 0
   "Which load average value will be shown in the mode line.
@@ -74,8 +70,7 @@ display-time-default-load-average
   :type '(choice (const :tag "1 minute load" 0)
 		 (const :tag "5 minutes load" 1)
 		 (const :tag "15 minutes load" 2)
-		 (const :tag "None" nil))
-  :group 'display-time)
+		 (const :tag "None" nil)))
 
 (defvar display-time-load-average nil
   "Value of the system's load average currently shown on the mode line.
@@ -85,27 +80,23 @@ display-time-load-average
 
 (defcustom display-time-load-average-threshold 0.1
   "Load-average values below this value won't be shown in the mode line."
-  :type 'number
-  :group 'display-time)
+  :type 'number)
 
 ;;;###autoload
 (defcustom display-time-day-and-date nil "\
 Non-nil means \\[display-time] should display day and date as well as time."
-  :type 'boolean
-  :group 'display-time)
+  :type 'boolean)
 
 (defvar display-time-timer nil)
 
 (defcustom display-time-interval 60
   "Seconds between updates of time in the mode line."
-  :type 'integer
-  :group 'display-time)
+  :type 'integer)
 
 (defcustom display-time-24hr-format nil
   "Non-nil indicates time should be displayed as hh:mm, 0 <= hh <= 23.
 A value of nil means 1 <= hh <= 12, and an AM/PM suffix is used."
-  :type 'boolean
-  :group 'display-time)
+  :type 'boolean)
 
 (defvar display-time-string nil
   "String used in mode lines to display a time string.
@@ -115,8 +106,7 @@ display-time-string
 
 (defcustom display-time-hook nil
   "List of functions to be called when the time is updated on the mode line."
-  :type 'hook
-  :group 'display-time)
+  :type 'hook)
 
 (defvar display-time-server-down-time nil
    "Time when mail file's file system was recorded to be down.
@@ -236,7 +226,6 @@ display-time-mail-face
 background color is the background of this face.  Set this to
 make the mail indicator stand out on a color display."
   :group 'mode-line-faces
-  :group 'display-time
   :version "22.1"
   :type '(choice (const :tag "None" nil) face))
 
@@ -251,14 +240,12 @@ display-time-use-mail-icon
   "Non-nil means use an icon as mail indicator on a graphic display.
 Otherwise use `display-time-mail-string'.  The icon may consume less
 of the mode line.  It is specified by `display-time-mail-icon'."
-  :group 'display-time
   :type 'boolean)
 
 ;; Fixme: maybe default to the character if we can display Unicode.
 (defcustom display-time-mail-string "Mail"
   "String to use as the mail indicator in `display-time-string-forms'.
 This can use the Unicode letter character if you can display it."
-  :group 'display-time
   :version "22.1"
   :type '(choice (const "Mail")
 		 ;; Use :tag here because the Lucid menu won't display
@@ -272,8 +259,7 @@ display-time-format
 how to write this string.  If this is nil, the defaults
 depend on `display-time-day-and-date' and `display-time-24hr-format'."
   :type '(choice (const :tag "Default" nil)
-		 string)
-  :group 'display-time)
+		 string))
 
 (defcustom display-time-string-forms
   '((if (and (not display-time-format) display-time-day-and-date)
@@ -327,8 +313,7 @@ display-time-string-forms
     (if mail \" Mail\" \"\"))
 
 would give mode line times like `94/12/30 21:07:48 (UTC)'."
-  :type '(repeat sexp)
-  :group 'display-time)
+  :type '(repeat sexp))
 
 (defun display-time-event-handler ()
   (display-time-update)
@@ -536,6 +521,92 @@ 'display-time-world-timer
 \f
 ;;; World clock
 
+(defgroup world-clock nil
+  "Display a world clock."
+  :group 'display-time)
+
+(defcustom zoneinfo-style-world-list
+  '(("America/Los_Angeles" "Seattle")
+    ("America/New_York" "New York")
+    ("Europe/London" "London")
+    ("Europe/Paris" "Paris")
+    ("Asia/Calcutta" "Bangalore")
+    ("Asia/Tokyo" "Tokyo"))
+  "Alist of zoneinfo-style time zones and places for `world-clock'.
+Each element has the form (TIMEZONE LABEL).
+TIMEZONE should be a string of the form AREA/LOCATION, where AREA is
+the name of a region -- a continent or ocean, and LOCATION is the name
+of a specific location, e.g., a city, within that region.
+LABEL is a string to display as the label of that TIMEZONE's time."
+  :type '(repeat (list string string))
+  :version "23.1")
+
+(defcustom legacy-style-world-list
+  '(("PST8PDT" "Seattle")
+    ("EST5EDT" "New York")
+    ("GMT0BST" "London")
+    ("CET-1CDT" "Paris")
+    ("IST-5:30" "Bangalore")
+    ("JST-9" "Tokyo"))
+  "Alist of traditional-style time zones and places for `world-clock'.
+Each element has the form (TIMEZONE LABEL).
+TIMEZONE should be a string of the form:
+
+     std[+|-]offset[dst[offset][,date[/time],date[/time]]]
+
+See the documentation of the TZ environment variable on your system,
+for more details about the format of TIMEZONE.
+LABEL is a string to display as the label of that TIMEZONE's time."
+  :type '(repeat (list string string))
+  :version "23.1")
+
+(defcustom world-clock-list t
+  "Alist of time zones and places for `world-clock' to display.
+Each element has the form (TIMEZONE LABEL).
+TIMEZONE should be in a format supported by your system.  See the
+documentation of `zoneinfo-style-world-list' and
+`legacy-style-world-list' for two widely used formats.  LABEL is
+a string to display as the label of that TIMEZONE's time.
+
+If the value is t instead of an alist, use the value of
+`zoneinfo-style-world-list' if it works on this platform, and of
+`legacy-style-world-list' otherwise."
+  :type '(choice (const :tag "Default" t)
+                 (repeat :tag "List of zones and labels"
+                         (list (string :tag "Zone") (string :tag "Label"))))
+  :version "28.1")
+
+(defun time--display-world-list ()
+  (if (listp world-clock-list)
+      world-clock-list
+    ;; Determine if zoneinfo style timezones are supported by testing that
+    ;; America/New York and Europe/London return different timezones.
+    (let ((nyt (format-time-string "%z" nil "America/New_York"))
+	  (gmt (format-time-string "%z" nil "Europe/London")))
+      (if (string-equal nyt gmt)
+	  legacy-style-world-list
+	zoneinfo-style-world-list))))
+
+(defcustom world-clock-time-format "%A %d %B %R %Z"
+  "Time format for `world-clock', see `format-time-string'."
+  :type 'string
+  :version "28.1")
+
+(defcustom world-clock-buffer-name "*wclock*"
+  "Name of the `world-clock' buffer."
+  :type 'string
+  :version "28.1")
+
+(defcustom world-clock-timer-enable t
+  "If non-nil, a timer will update the `world-clock' buffer."
+  :type 'boolean
+  :version "28.1")
+
+(defcustom world-clock-timer-second 60
+  "Interval in seconds for updating the `world-clock' buffer."
+  :type 'integer
+  :version "28.1")
+
 (defface world-clock-label
   '((t :inherit font-lock-variable-name-face))
   "Face for time zone label in `world-clock' buffer.")
-- 
2.27.0

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

[Basil L. Contovounesios]

Stefan Kangas <stefan@marxist.se> writes:

> Thank you for the detailed review!  I believe I have fixed all your
> comments in the attached patches.  (Note that I squashed patch 2 and 3
> into one patch.)
>
> Please let me know if there are any further comments.

Thanks, we're close. ;)  See below.

> Subject: [PATCH 1/3] Improve display-time-world UI (Bug#40863)

[...]

>  (define-derived-mode display-time-world-mode special-mode "World clock"
>    "Major mode for buffer that displays times in various time zones.
>  See `display-time-world'."
> +  (setq revert-buffer-function 'display-time-world-timer)
                                  ^
                            Nit: #'

More importantly, doesn't revert-buffer-function need two parameters?

[...]

> Subject: [PATCH 3/3] Rearrange and clean up code in time.el (Bug#40863)
>
> * 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.

Unless I'm misreading it, the patch you attached seems to be adding
duplicate definitions rather than moving them.

-- 
Basil

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

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

>>  (define-derived-mode display-time-world-mode special-mode "World clock"
>>    "Major mode for buffer that displays times in various time zones.
>>  See `display-time-world'."
>> +  (setq revert-buffer-function 'display-time-world-timer)
>                                   ^
>                             Nit: #'
>
> More importantly, doesn't revert-buffer-function need two parameters?

Fixed.

>> Subject: [PATCH 3/3] Rearrange and clean up code in time.el (Bug#40863)
>>
>> * 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.
>
> Unless I'm misreading it, the patch you attached seems to be adding
> duplicate definitions rather than moving them.

Uhm, right... Fixed in the attached.

Thanks again for the review.

Best regards,
Stefan Kangas

[-- Attachment #2: 0001-Improve-display-time-world-UI-Bug-40863.patch --]
[-- Type: text/x-diff, Size: 2960 bytes --]

From c7bb644998bed7262d491dd7c73af2ed34b5ca43 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/3] Improve display-time-world UI (Bug#40863)

* lisp/time.el (display-time-world-mode): Set 'revert-buffer-function'
to 'display-time-world-timer'.

(display-time-world-label): New face.
(display-time-world-display): Use the new face.  Move point to new
buffer on creation and resize.
---
 lisp/time.el | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/lisp/time.el b/lisp/time.el
index 44fd1a7e33..3625940334 100644
--- a/lisp/time.el
+++ b/lisp/time.el
@@ -509,9 +509,14 @@ display-time-mode
 		 'display-time-event-handler)))
 
 
+(defface display-time-world-label
+  '((t :inherit font-lock-variable-name-face))
+  "Face for time zone label.")
+
 (define-derived-mode display-time-world-mode special-mode "World clock"
   "Major mode for buffer that displays times in various time zones.
 See `display-time-world'."
+  (setq revert-buffer-function #'display-time-world-timer)
   (setq show-trailing-whitespace nil))
 
 (defun display-time-world-display (alist)
@@ -533,7 +538,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))))
     (delete-char -1))
   (goto-char (point-min)))
 
@@ -541,18 +549,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)
+  (fit-window-to-buffer))
 
-(defun display-time-world-timer ()
+(defun display-time-world-timer (&optional _arg _noconfirm)
   (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.28.0


[-- Attachment #3: 0002-Rename-display-time-world-to-world-clock-Bug-40863.patch --]
[-- Type: text/x-diff, Size: 11789 bytes --]

From 52a02e6d159d93f9d707ea9e5d40a05f290ac1f9 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/3] Rename 'display-time-world' to 'world-clock' (Bug#40863)

* lisp/time.el (world-clock-list, world-clock-time-format)
(world-clock-buffer-name, world-clock-timer-enable)
(world-clock-timer-second, world-clock-label, world-clock-mode)
(world-clock-display, world-clock, world-clock-update): Rename
from 'display-time-world-*' and update all uses.

(world-clock): New defgroup.
(zoneinfo-style-world-list, legacy-style-world-list): Use :group
'world-clock'.

(display-time-world-list, display-time-world-time-format)
(display-time-world-buffer-name)
(display-time-world-timer-enable)
(display-time-world-timer-second): Make into obsolete variable
aliases for the new names.

* lisp/time.el (display-time-world-mode)
(display-time-world-display, display-time-world)
(display-time-world-timer): Make into obsolete function aliases
for the new names.

* etc/NEWS: Announce the above changes.
---
 etc/NEWS     |  25 ++++++++++
 lisp/time.el | 134 +++++++++++++++++++++++++++++++--------------------
 2 files changed, 106 insertions(+), 53 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index e3d7ff0bef..9078cd3a7a 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -754,6 +754,31 @@ never be narrower than 19 characters.
 When the 'bookmark.el' library is loaded, a customize choice is added
 to 'tab-bar-new-tab-choice' for new tabs to show the bookmark list.
 
+** Time
+
+---
+*** 'display-time-world' has been renamed to 'world-clock'.
+'world-clock' creates a buffer with an updating time display using
+several time zones.  It is hoped that the new names are more
+discoverable.
+
+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-name'
+  'display-time-world-timer-enable' to 'world-clock-timer-enable'
+  'display-time-world-timer-second' to 'world-clock-timer-second'
+
+The old names are now obsolete.
+
 \f
 * New Modes and Packages in Emacs 28.1
 
diff --git a/lisp/time.el b/lisp/time.el
index 3625940334..fe918c5400 100644
--- a/lisp/time.el
+++ b/lisp/time.el
@@ -25,8 +25,7 @@
 ;; Facilities to display current time/date and a new-mail indicator
 ;; in the Emacs mode line.  The entry point is `display-time'.
 
-;; Display time world in a buffer, the entry point is
-;; `display-time-world'.
+;; Use `world-clock' to display world clock in a buffer.
 
 ;;; Code:
 
@@ -123,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
+  "Display a world clock."
+  :group 'display-time)
+
 (defcustom zoneinfo-style-world-list
   '(("America/Los_Angeles" "Seattle")
     ("America/New_York" "New York")
@@ -130,13 +133,13 @@ zoneinfo-style-world-list
     ("Europe/Paris" "Paris")
     ("Asia/Calcutta" "Bangalore")
     ("Asia/Tokyo" "Tokyo"))
-  "Alist of zoneinfo-style time zones and places for `display-time-world'.
+  "Alist of zoneinfo-style time zones and places for `world-clock'.
 Each element has the form (TIMEZONE LABEL).
 TIMEZONE should be a string of the form AREA/LOCATION, where AREA is
 the name of a region -- a continent or ocean, and LOCATION is the name
 of a specific location, e.g., a city, within that region.
 LABEL is a string to display as the label of that TIMEZONE's time."
-  :group 'display-time
+  :group 'world-clock
   :type '(repeat (list string string))
   :version "23.1")
 
@@ -147,7 +150,7 @@ legacy-style-world-list
     ("CET-1CDT" "Paris")
     ("IST-5:30" "Bangalore")
     ("JST-9" "Tokyo"))
-  "Alist of traditional-style time zones and places for `display-time-world'.
+  "Alist of traditional-style time zones and places for `world-clock'.
 Each element has the form (TIMEZONE LABEL).
 TIMEZONE should be a string of the form:
 
@@ -156,12 +159,12 @@ legacy-style-world-list
 See the documentation of the TZ environment variable on your system,
 for more details about the format of TIMEZONE.
 LABEL is a string to display as the label of that TIMEZONE's time."
-  :group 'display-time
+  :group 'world-clock
   :type '(repeat (list string string))
   :version "23.1")
 
-(defcustom display-time-world-list t
-  "Alist of time zones and places for `display-time-world' to display.
+(defcustom world-clock-list t
+  "Alist of time zones and places for `world-clock' to display.
 Each element has the form (TIMEZONE LABEL).
 TIMEZONE should be in a format supported by your system.  See the
 documentation of `zoneinfo-style-world-list' and
@@ -171,16 +174,15 @@ display-time-world-list
 If the value is t instead of an alist, use the value of
 `zoneinfo-style-world-list' if it works on this platform, and of
 `legacy-style-world-list' otherwise."
-
-  :group 'display-time
+  :group 'world-clock
   :type '(choice (const :tag "Default" t)
                  (repeat :tag "List of zones and labels"
                          (list (string :tag "Zone") (string :tag "Label"))))
-  :version "23.1")
+  :version "28.1")
 
 (defun time--display-world-list ()
-  (if (listp display-time-world-list)
-      display-time-world-list
+  (if (listp world-clock-list)
+      world-clock-list
     ;; Determine if zoneinfo style timezones are supported by testing that
     ;; America/New York and Europe/London return different timezones.
     (let ((nyt (format-time-string "%z" nil "America/New_York"))
@@ -189,29 +191,29 @@ time--display-world-list
 	  legacy-style-world-list
 	zoneinfo-style-world-list))))
 
-(defcustom display-time-world-time-format "%A %d %B %R %Z"
-  "Format of the time displayed, see `format-time-string'."
-  :group 'display-time
+(defcustom world-clock-time-format "%A %d %B %R %Z"
+  "Time format for `world-clock', see `format-time-string'."
+  :group 'world-clock
   :type 'string
-  :version "23.1")
+  :version "28.1")
 
-(defcustom display-time-world-buffer-name "*wclock*"
-  "Name of the world clock buffer."
-  :group 'display-time
+(defcustom world-clock-buffer-name "*wclock*"
+  "Name of the `world-clock' buffer."
+  :group 'world-clock
   :type 'string
-  :version "23.1")
+  :version "28.1")
 
-(defcustom display-time-world-timer-enable t
-  "If non-nil, a timer will update the world clock."
-  :group 'display-time
+(defcustom world-clock-timer-enable t
+  "If non-nil, a timer will update the `world-clock' buffer."
+  :group 'world-clock
   :type 'boolean
-  :version "23.1")
+  :version "28.1")
 
-(defcustom display-time-world-timer-second 60
-  "Interval in seconds for updating the world clock."
-  :group 'display-time
+(defcustom world-clock-timer-second 60
+  "Interval in seconds for updating the `world-clock' buffer."
+  :group 'world-clock
   :type 'integer
-  :version "23.1")
+  :version "28.1")
 
 ;;;###autoload
 (defun display-time ()
@@ -508,18 +510,43 @@ display-time-mode
     (remove-hook 'rmail-after-get-new-mail-hook
 		 'display-time-event-handler)))
 
-
-(defface display-time-world-label
+\f
+;;; 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")
+
+(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")
+
+\f
+;;; World clock
+
+(defface world-clock-label
   '((t :inherit font-lock-variable-name-face))
-  "Face for time zone label.")
+  "Face for time zone label in `world-clock' buffer.")
 
-(define-derived-mode display-time-world-mode special-mode "World clock"
+(define-derived-mode world-clock-mode special-mode "World clock"
   "Major mode for buffer that displays times in various time zones.
-See `display-time-world'."
-  (setq revert-buffer-function #'display-time-world-timer)
+See `world-clock'."
+  (setq revert-buffer-function #'world-clock-update)
   (setq show-trailing-whitespace nil))
 
-(defun display-time-world-display (alist)
+(defun world-clock-display (alist)
   "Replace current buffer text with times in various zones, based on ALIST."
   (let ((inhibit-read-only t)
 	(buffer-undo-list t)
@@ -531,7 +558,7 @@ display-time-world-display
       (let* ((label (cadr zone))
 	     (width (string-width label)))
 	(push (cons label
-		    (format-time-string display-time-world-time-format
+		    (format-time-string world-clock-time-format
 					now (car zone)))
 	      result)
 	(when (> width max-width)
@@ -540,35 +567,36 @@ display-time-world-display
     (dolist (timedata (nreverse result))
       (insert (format fmt
                       (propertize (car timedata)
-                                  'face 'display-time-world-label)
+                                  'face 'world-clock-label)
                       (cdr timedata))))
     (delete-char -1))
   (goto-char (point-min)))
 
 ;;;###autoload
-(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 `\\[quit-window]'."
+(defun world-clock ()
+  "Display a world clock buffer with times in various time zones.
+The variable `world-clock-list' specifies which time zones to use.
+To turn off the world time display, go to the 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)
+  (when (and world-clock-timer-enable
+             (not (get-buffer world-clock-buffer-name)))
+    (run-at-time t world-clock-timer-second #'world-clock-update))
+  (pop-to-buffer world-clock-buffer-name)
+  (world-clock-display (time--display-world-list))
+  (world-clock-mode)
   (fit-window-to-buffer))
 
-(defun display-time-world-timer (&optional _arg _noconfirm)
-  (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)))
+(defun world-clock-update (&optional _arg _noconfirm)
+  "Update the `world-clock' buffer."
+  (if (get-buffer world-clock-buffer-name)
+      (with-current-buffer (get-buffer world-clock-buffer-name)
+        (world-clock-display (time--display-world-list)))
     ;; cancel timer
     (let ((list timer-list))
       (while list
         (let ((elt (pop list)))
           (when (equal (symbol-name (timer--function elt))
-		       "display-time-world-timer")
+		       "world-clock-update")
             (cancel-timer elt)))))))
 
 ;;;###autoload
-- 
2.28.0


[-- Attachment #4: 0003-Rearrange-and-clean-up-code-in-time.el-Bug-40863.patch --]
[-- Type: text/x-diff, Size: 12470 bytes --]

From df1d9b2470a4a39c909bea69c9b8dcc6427c1566 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sun, 9 Aug 2020 23:42:06 +0200
Subject: [PATCH 3/3] Rearrange and clean up code in time.el (Bug#40863)

* 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.
---
 lisp/time.el | 215 +++++++++++++++++++++++----------------------------
 1 file changed, 97 insertions(+), 118 deletions(-)

diff --git a/lisp/time.el b/lisp/time.el
index fe918c5400..96b49ddabd 100644
--- a/lisp/time.el
+++ b/lisp/time.el
@@ -34,23 +34,20 @@ display-time
   :group 'mode-line
   :group 'mail)
 
-
 (defcustom display-time-mail-file nil
   "File name of mail inbox file, for indicating existence of new mail.
 Non-nil and not a string means don't check for mail; nil means use
 default, which is system-dependent, and is the same as used by Rmail."
   :type '(choice (const :tag "None" none)
 		 (const :tag "Default" nil)
-		 (file :format "%v"))
-  :group 'display-time)
+		 (file :format "%v")))
 
 (defcustom display-time-mail-directory nil
   "Name of mail inbox directory, for indicating existence of new mail.
 Any nonempty regular file in the directory is regarded as newly arrived mail.
 If nil, do not check a directory for arriving mail."
   :type '(choice (const :tag "None" nil)
-		 (directory :format "%v"))
-  :group 'display-time)
+		 (directory :format "%v")))
 
 (defcustom display-time-mail-function nil
   "Function to call, for indicating existence of new mail.
@@ -58,8 +55,7 @@ display-time-mail-function
 specified by `display-time-mail-file' is nonempty or that the
 directory `display-time-mail-directory' contains nonempty files."
   :type '(choice (const :tag "Default" nil)
-		 (function))
-  :group 'display-time)
+		 (function)))
 
 (defcustom display-time-default-load-average 0
   "Which load average value will be shown in the mode line.
@@ -74,8 +70,7 @@ display-time-default-load-average
   :type '(choice (const :tag "1 minute load" 0)
 		 (const :tag "5 minutes load" 1)
 		 (const :tag "15 minutes load" 2)
-		 (const :tag "None" nil))
-  :group 'display-time)
+		 (const :tag "None" nil)))
 
 (defvar display-time-load-average nil
   "Value of the system's load average currently shown on the mode line.
@@ -85,27 +80,23 @@ display-time-load-average
 
 (defcustom display-time-load-average-threshold 0.1
   "Load-average values below this value won't be shown in the mode line."
-  :type 'number
-  :group 'display-time)
+  :type 'number)
 
 ;;;###autoload
 (defcustom display-time-day-and-date nil "\
 Non-nil means \\[display-time] should display day and date as well as time."
-  :type 'boolean
-  :group 'display-time)
+  :type 'boolean)
 
 (defvar display-time-timer nil)
 
 (defcustom display-time-interval 60
   "Seconds between updates of time in the mode line."
-  :type 'integer
-  :group 'display-time)
+  :type 'integer)
 
 (defcustom display-time-24hr-format nil
   "Non-nil indicates time should be displayed as hh:mm, 0 <= hh <= 23.
 A value of nil means 1 <= hh <= 12, and an AM/PM suffix is used."
-  :type 'boolean
-  :group 'display-time)
+  :type 'boolean)
 
 (defvar display-time-string nil
   "String used in mode lines to display a time string.
@@ -115,106 +106,12 @@ display-time-string
 
 (defcustom display-time-hook nil
   "List of functions to be called when the time is updated on the mode line."
-  :type 'hook
-  :group 'display-time)
+  :type 'hook)
 
 (defvar display-time-server-down-time nil
    "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
-  "Display a world clock."
-  :group 'display-time)
-
-(defcustom zoneinfo-style-world-list
-  '(("America/Los_Angeles" "Seattle")
-    ("America/New_York" "New York")
-    ("Europe/London" "London")
-    ("Europe/Paris" "Paris")
-    ("Asia/Calcutta" "Bangalore")
-    ("Asia/Tokyo" "Tokyo"))
-  "Alist of zoneinfo-style time zones and places for `world-clock'.
-Each element has the form (TIMEZONE LABEL).
-TIMEZONE should be a string of the form AREA/LOCATION, where AREA is
-the name of a region -- a continent or ocean, and LOCATION is the name
-of a specific location, e.g., a city, within that region.
-LABEL is a string to display as the label of that TIMEZONE's time."
-  :group 'world-clock
-  :type '(repeat (list string string))
-  :version "23.1")
-
-(defcustom legacy-style-world-list
-  '(("PST8PDT" "Seattle")
-    ("EST5EDT" "New York")
-    ("GMT0BST" "London")
-    ("CET-1CDT" "Paris")
-    ("IST-5:30" "Bangalore")
-    ("JST-9" "Tokyo"))
-  "Alist of traditional-style time zones and places for `world-clock'.
-Each element has the form (TIMEZONE LABEL).
-TIMEZONE should be a string of the form:
-
-     std[+|-]offset[dst[offset][,date[/time],date[/time]]]
-
-See the documentation of the TZ environment variable on your system,
-for more details about the format of TIMEZONE.
-LABEL is a string to display as the label of that TIMEZONE's time."
-  :group 'world-clock
-  :type '(repeat (list string string))
-  :version "23.1")
-
-(defcustom world-clock-list t
-  "Alist of time zones and places for `world-clock' to display.
-Each element has the form (TIMEZONE LABEL).
-TIMEZONE should be in a format supported by your system.  See the
-documentation of `zoneinfo-style-world-list' and
-`legacy-style-world-list' for two widely used formats.  LABEL is
-a string to display as the label of that TIMEZONE's time.
-
-If the value is t instead of an alist, use the value of
-`zoneinfo-style-world-list' if it works on this platform, and of
-`legacy-style-world-list' otherwise."
-  :group 'world-clock
-  :type '(choice (const :tag "Default" t)
-                 (repeat :tag "List of zones and labels"
-                         (list (string :tag "Zone") (string :tag "Label"))))
-  :version "28.1")
-
-(defun time--display-world-list ()
-  (if (listp world-clock-list)
-      world-clock-list
-    ;; Determine if zoneinfo style timezones are supported by testing that
-    ;; America/New York and Europe/London return different timezones.
-    (let ((nyt (format-time-string "%z" nil "America/New_York"))
-	  (gmt (format-time-string "%z" nil "Europe/London")))
-      (if (string-equal nyt gmt)
-	  legacy-style-world-list
-	zoneinfo-style-world-list))))
-
-(defcustom world-clock-time-format "%A %d %B %R %Z"
-  "Time format for `world-clock', see `format-time-string'."
-  :group 'world-clock
-  :type 'string
-  :version "28.1")
-
-(defcustom world-clock-buffer-name "*wclock*"
-  "Name of the `world-clock' buffer."
-  :group 'world-clock
-  :type 'string
-  :version "28.1")
-
-(defcustom world-clock-timer-enable t
-  "If non-nil, a timer will update the `world-clock' buffer."
-  :group 'world-clock
-  :type 'boolean
-  :version "28.1")
-
-(defcustom world-clock-timer-second 60
-  "Interval in seconds for updating the `world-clock' buffer."
-  :group 'world-clock
-  :type 'integer
-  :version "28.1")
-
 ;;;###autoload
 (defun display-time ()
   "Enable display of time, load level, and mail flag in mode lines.
@@ -251,14 +148,12 @@ display-time-use-mail-icon
   "Non-nil means use an icon as mail indicator on a graphic display.
 Otherwise use `display-time-mail-string'.  The icon may consume less
 of the mode line.  It is specified by `display-time-mail-icon'."
-  :group 'display-time
   :type 'boolean)
 
 ;; Fixme: maybe default to the character if we can display Unicode.
 (defcustom display-time-mail-string "Mail"
   "String to use as the mail indicator in `display-time-string-forms'.
 This can use the Unicode letter character if you can display it."
-  :group 'display-time
   :version "22.1"
   :type '(choice (const "Mail")
 		 ;; Use :tag here because the Lucid menu won't display
@@ -272,8 +167,7 @@ display-time-format
 how to write this string.  If this is nil, the defaults
 depend on `display-time-day-and-date' and `display-time-24hr-format'."
   :type '(choice (const :tag "Default" nil)
-		 string)
-  :group 'display-time)
+		 string))
 
 (defcustom display-time-string-forms
   '((if (and (not display-time-format) display-time-day-and-date)
@@ -327,8 +221,7 @@ display-time-string-forms
     (if mail \" Mail\" \"\"))
 
 would give mode line times like `94/12/30 21:07:48 (UTC)'."
-  :type '(repeat sexp)
-  :group 'display-time)
+  :type '(repeat sexp))
 
 (defun display-time-event-handler ()
   (display-time-update)
@@ -536,6 +429,92 @@ 'display-time-world-timer
 \f
 ;;; World clock
 
+(defgroup world-clock nil
+  "Display a world clock."
+  :group 'display-time)
+
+(defcustom zoneinfo-style-world-list
+  '(("America/Los_Angeles" "Seattle")
+    ("America/New_York" "New York")
+    ("Europe/London" "London")
+    ("Europe/Paris" "Paris")
+    ("Asia/Calcutta" "Bangalore")
+    ("Asia/Tokyo" "Tokyo"))
+  "Alist of zoneinfo-style time zones and places for `world-clock'.
+Each element has the form (TIMEZONE LABEL).
+TIMEZONE should be a string of the form AREA/LOCATION, where AREA is
+the name of a region -- a continent or ocean, and LOCATION is the name
+of a specific location, e.g., a city, within that region.
+LABEL is a string to display as the label of that TIMEZONE's time."
+  :type '(repeat (list string string))
+  :version "23.1")
+
+(defcustom legacy-style-world-list
+  '(("PST8PDT" "Seattle")
+    ("EST5EDT" "New York")
+    ("GMT0BST" "London")
+    ("CET-1CDT" "Paris")
+    ("IST-5:30" "Bangalore")
+    ("JST-9" "Tokyo"))
+  "Alist of traditional-style time zones and places for `world-clock'.
+Each element has the form (TIMEZONE LABEL).
+TIMEZONE should be a string of the form:
+
+     std[+|-]offset[dst[offset][,date[/time],date[/time]]]
+
+See the documentation of the TZ environment variable on your system,
+for more details about the format of TIMEZONE.
+LABEL is a string to display as the label of that TIMEZONE's time."
+  :type '(repeat (list string string))
+  :version "23.1")
+
+(defcustom world-clock-list t
+  "Alist of time zones and places for `world-clock' to display.
+Each element has the form (TIMEZONE LABEL).
+TIMEZONE should be in a format supported by your system.  See the
+documentation of `zoneinfo-style-world-list' and
+`legacy-style-world-list' for two widely used formats.  LABEL is
+a string to display as the label of that TIMEZONE's time.
+
+If the value is t instead of an alist, use the value of
+`zoneinfo-style-world-list' if it works on this platform, and of
+`legacy-style-world-list' otherwise."
+  :type '(choice (const :tag "Default" t)
+                 (repeat :tag "List of zones and labels"
+                         (list (string :tag "Zone") (string :tag "Label"))))
+  :version "28.1")
+
+(defun time--display-world-list ()
+  (if (listp world-clock-list)
+      world-clock-list
+    ;; Determine if zoneinfo style timezones are supported by testing that
+    ;; America/New York and Europe/London return different timezones.
+    (let ((nyt (format-time-string "%z" nil "America/New_York"))
+	  (gmt (format-time-string "%z" nil "Europe/London")))
+      (if (string-equal nyt gmt)
+	  legacy-style-world-list
+	zoneinfo-style-world-list))))
+
+(defcustom world-clock-time-format "%A %d %B %R %Z"
+  "Time format for `world-clock', see `format-time-string'."
+  :type 'string
+  :version "28.1")
+
+(defcustom world-clock-buffer-name "*wclock*"
+  "Name of the `world-clock' buffer."
+  :type 'string
+  :version "28.1")
+
+(defcustom world-clock-timer-enable t
+  "If non-nil, a timer will update the `world-clock' buffer."
+  :type 'boolean
+  :version "28.1")
+
+(defcustom world-clock-timer-second 60
+  "Interval in seconds for updating the `world-clock' buffer."
+  :type 'integer
+  :version "28.1")
+
 (defface world-clock-label
   '((t :inherit font-lock-variable-name-face))
   "Face for time zone label in `world-clock' buffer.")
-- 
2.28.0

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

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> +*** 'display-time-world' has been renamed to 'world-clock'.
> +'world-clock' creates a buffer with an updating time display using
> +several time zones.  It is hoped that the new names are more
> +discoverable.

Looks like this was supposed to be applied, but it doesn't look like it
has been yet?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Looks like this was supposed to be applied, but it doesn't look like it
> has been yet?

I wanted to give Basil some time to comment, since he had some feedback
on the previous patches.  But I guess 9 days is plenty in this case, so
I'll look this all over one final time and push it when I find some
time.

Best regards,
Stefan Kangas

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

[Stefan Kangas]

close 40863 28.1
thanks

Stefan Kangas <stefan@marxist.se> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> Looks like this was supposed to be applied, but it doesn't look like it
>> has been yet?
>
> I wanted to give Basil some time to comment, since he had some feedback
> on the previous patches.  But I guess 9 days is plenty in this case, so
> I'll look this all over one final time and push it when I find some
> time.

Now pushed to master as commit c70b5e8ad0.  Closing.

Best regards,
Stefan Kangas