From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.ciao.gmane.io!not-for-mail From: "Basil L. Contovounesios" Newsgroups: gmane.emacs.bugs Subject: bug#40863: [PATCH] Improve the display-time-world UI Date: Sat, 23 May 2020 14:43:18 +0100 Message-ID: <87y2pircll.fsf@tcd.ie> References: <874kt4y1rc.fsf@tcd.ie> <87k11u48t0.fsf@stefankangas.se> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="ciao.gmane.io:159.69.161.202"; logging-data="28864"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) Cc: 40863@debbugs.gnu.org To: Stefan Kangas Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sat May 23 15:44:10 2020 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jcURd-0007NZ-P6 for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 23 May 2020 15:44:09 +0200 Original-Received: from localhost ([::1]:42910 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jcURc-00088B-Pp for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 23 May 2020 09:44:08 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:57842) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jcURW-000880-AC for bug-gnu-emacs@gnu.org; Sat, 23 May 2020 09:44:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:50421) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jcURW-00010A-1P for bug-gnu-emacs@gnu.org; Sat, 23 May 2020 09:44:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1jcURV-0006os-VU for bug-gnu-emacs@gnu.org; Sat, 23 May 2020 09:44:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: "Basil L. Contovounesios" Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 23 May 2020 13:44:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 40863 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 40863-submit@debbugs.gnu.org id=B40863.159024140926164 (code B ref 40863); Sat, 23 May 2020 13:44:01 +0000 Original-Received: (at 40863) by debbugs.gnu.org; 23 May 2020 13:43:29 +0000 Original-Received: from localhost ([127.0.0.1]:33734 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jcUQy-0006nw-V2 for submit@debbugs.gnu.org; Sat, 23 May 2020 09:43:29 -0400 Original-Received: from mail-wr1-f46.google.com ([209.85.221.46]:37201) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jcUQx-0006nh-0o for 40863@debbugs.gnu.org; Sat, 23 May 2020 09:43:27 -0400 Original-Received: by mail-wr1-f46.google.com with SMTP id l17so13019367wrr.4 for <40863@debbugs.gnu.org>; Sat, 23 May 2020 06:43:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tcd-ie.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version; bh=5nHm8ih1nYbMDj24WKMvh/BPha9ohhAhvy0NEfhQ/qw=; b=1u5J837dcmmjUYU6oFd2GyGxjeN5t2yfWRiew3wUkjns21uucbmCQZufP793/VByu4 lyWfsqsOUQUM52mVD+MGGcT9zBHh+cwt7Mxjkf/vOnK3M4jJYK+hZm0eLc8Zptq6njrA TuR7iGHFxPOJEyhmqoeTpnpWybBGSJtM8b8meNixdHLs0yoHW2+Vi1M2rrto1YLqVowF Y5v5Muw5Swt+sXnGB34Q4UA3qPm6BTBHX83oMoBZ+hVSvy2A3CL7vfQnhBCGl9FqiynC i5N9jMBEH8PuDKZIDDgZOHCZQSLLn+Z9bmpOAqTY8+0Df73hsQEdl+xVkmubXfeUlXPv ho7A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version; bh=5nHm8ih1nYbMDj24WKMvh/BPha9ohhAhvy0NEfhQ/qw=; b=ANPiN0ByWW56xfoDae4+VUFKBxGslj4/AXGf64lD/t2GYkmAPKxEOL5W2lfY2RIXO4 JrR9/G9XhA7sPACaT1kly5rGpSKMsdt0/eL4zgG5bfiY4dZ0ygYspvRpvZvZplCCrjg0 tXD6vPNbXj7jPMZxqyKhTO9925Md+9wmVTdW9w70tHrJiHvaeNBrSOXbaVYuLR706hn8 a6ebZE7NYQjGkelu9KjEfmCAuMLyS2E1BAeFf1kRAn2kzg9vP3OKP2dm/0tHk2ah+wPj 0XwsHG7rEWalIzd50101GrZyBDvHT6aCZIO4HuZwZBhz/ynDN7xLMjwR3oc4gKW9vRqg cuPA== X-Gm-Message-State: AOAM530m4oe2qz/xqlRVD3j+q0w8Z2RuuRnzwWHNT0zZ2Eu59KHGjZX/ QjSLAswBIBZiTczXUV6UepQe5yc46U4= X-Google-Smtp-Source: ABdhPJykTLiN8zHGZV5X+BQ4sOAKcwGANbJNzDiOfT5dvdYk1onY0BhQFEX3f0qBmjsehALPy45Mjg== X-Received: by 2002:a5d:5585:: with SMTP id i5mr4314302wrv.112.1590241400821; Sat, 23 May 2020 06:43:20 -0700 (PDT) Original-Received: from localhost ([2a02:8084:20e2:c380:1f68:7ff5:120d:64e]) by smtp.gmail.com with ESMTPSA id z12sm1560800wrg.9.2020.05.23.06.43.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 23 May 2020 06:43:19 -0700 (PDT) In-Reply-To: <87k11u48t0.fsf@stefankangas.se> (Stefan Kangas's message of "Sat, 02 May 2020 18:10:19 +0200") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:180803 Archived-At: 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 > 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 > 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 > Date: Sat, 2 May 2020 17:26:14 +0200 > Subject: [PATCH 3/4] Rename 'display-time-world' to 'world-clock' (Bug#40863) [...] > diff --git a/etc/NEWS b/etc/NEWS > index d2b745c579..d97b148cc8 100644 > --- a/etc/NEWS > +++ b/etc/NEWS > @@ -291,9 +291,26 @@ These new navigation commands are bound to 'n' and 'p' in > ** Time > > --- > -*** 'display-time-world' is now an alias for 'world-clock'. > +*** 'display-time-world' has been renamed to 'world-clock'. > 'world-clock' creates a buffer with an updating time display using > -several time zones. The new name is hoped to be more discoverable. > +several time zones. I think you can keep "it is hoped that the new names are more discoverable" as a justification for this mass rename. > +The following functions have been renamed: > + > + 'display-time-world' to 'world-clock' > + 'display-time-world-mode' to 'world-clock-mode' > + 'display-time-world-display' to 'world-clock-display' > + 'display-time-world-timer' to 'world-clock-update' > + > +The following options have been renamed: > + > + 'display-time-world-list' to 'world-clock-list' > + 'display-time-world-time-format' to 'world-clock-time-format' > + 'display-time-world-buffer-name' to 'world-clock-buffer-namt' > + 'display-time-world-timer-enable' to 'world-clock-timer-enablt' > + 'display-time-world-timer-second' to 'world-clock-timer-secont' The last three world-clock-* vars are misspelt. > +The old names are now obsolete. > > > * New Modes and Packages in Emacs 28.1 > diff --git a/lisp/time.el b/lisp/time.el > index fe290ff71f..3f1880b708 100644 > --- a/lisp/time.el > +++ b/lisp/time.el > @@ -122,6 +122,10 @@ display-time-server-down-time > "Time when mail file's file system was recorded to be down. > If that file system seems to be up, the value is nil.") > > +(defgroup world-clock nil > + "Show a world clock." Nit: s/show/display/ Rings better in my ears and is consistent with the description of the display-time group. > + :group 'applications) It should continue to be under :group 'display-time as well. [...] > +;;; Obsolete names > + > +(define-obsolete-variable-alias 'display-time-world-list > + 'world-clock-list "28.1") > +(define-obsolete-variable-alias 'display-time-world-time-format > + 'world-clock-time-format "28.1") > +(define-obsolete-variable-alias 'display-time-world-buffer-name > + 'world-clock-buffer-name "28.1") > +(define-obsolete-variable-alias 'display-time-world-timer-enable > + 'world-clock-timer-enable "28.1") > +(define-obsolete-variable-alias 'display-time-world-timer-second > + 'world-clock-timer-second "28.1") These varaliases need to be declared before the variables they refer to. > +(define-obsolete-function-alias 'display-time-world-mode > + #'world-clock-mode "28.1") > +(define-obsolete-function-alias 'display-time-world-display > + #'world-clock-display "28.1") > +(define-obsolete-function-alias 'display-time-world > + #'world-clock "28.1") > +(define-obsolete-function-alias 'display-time-world-timer > + #'world-clock-update "28.1") > + > + > +;;; World clock > + > +(defface world-clock-label > '((t :inherit font-lock-variable-name-face)) > "Face for time zone label in `world-clock' buffer.") > > -(defvar display-time-world-mode-map > +(defvar world-clock-mode-map > (let ((map (make-sparse-keymap))) > - (define-key map "g" #'display-time-world-timer) > + (define-key map "g" #'world-clock-update) Ditto re: revert-buffer-function. > map) > - "Keymap for `display-time-world-mode'.") > + "Keymap for `world-clock-mode'.") [...] > @@ -552,32 +580,30 @@ display-time-world-display > ;;;###autoload > (defun world-clock () > "Show world clock buffer with times in various time zones. > -`display-time-world-list' specifies the zones. > +`world-clock-list' specifies the zones. > To turn off the world time display, go to that window and type `\\[quit-window]'." > (interactive) > - (when (and display-time-world-timer-enable > - (not (get-buffer display-time-world-buffer-name))) > - (run-at-time t display-time-world-timer-second 'display-time-world-timer)) > - (pop-to-buffer display-time-world-buffer-name) > - (display-time-world-display (time--display-world-list)) > - (display-time-world-mode)) > - > -(defun display-time-world-timer () > + (when (and world-clock-timer-enable > + (not (get-buffer world-clock-buffer-name))) > + (run-at-time t world-clock-timer-second 'world-clock-update)) ^ Nit: Quote with #'. > + (pop-to-buffer world-clock-buffer-name) > + (world-clock-display (time--display-world-list)) > + (world-clock-mode)) [...] > From 137c920f5b1ab34120060af0d4e3adada0f3a8a3 Mon Sep 17 00:00:00 2001 > From: Stefan Kangas > 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