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

From: "Basil L. Contovounesios" <[email-masked]>
Date: Fri, 07 Aug 2020 18:51:35 +0100

Stefan Kangas <[email-masked]> 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