* Re: master d378615: Cancel timer when world-clock buffer is killed [not found] ` <20200903105450.8CD2D20A15@vcs0.savannah.gnu.org> @ 2020-09-03 13:46 ` Basil L. Contovounesios 2020-09-03 17:22 ` Stefan Kangas 0 siblings, 1 reply; 5+ messages in thread From: Basil L. Contovounesios @ 2020-09-03 13:46 UTC (permalink / raw) To: Stefan Kangas; +Cc: emacs-devel stefankangas@gmail.com (Stefan Kangas) writes: > branch: master > commit d37861535dfd452f7c2255ae5edcf7686b75fe5a > Author: Stefan Kangas <stefankangas@gmail.com> > Commit: Stefan Kangas <stefankangas@gmail.com> > > Cancel timer when world-clock buffer is killed > > * lisp/time.el (world-clock-timer): New variable. > (world-clock-cancel-timer): New defun. > (world-clock): Add 'world-clock-cancel-timer' to 'kill-buffer-hook'. > --- > lisp/time.el | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/lisp/time.el b/lisp/time.el > index 1ab992a..5ced920 100644 > --- a/lisp/time.el > +++ b/lisp/time.el > @@ -523,6 +523,8 @@ See `world-clock'." > (setq-local revert-buffer-function #'world-clock-update) > (setq show-trailing-whitespace nil)) > > +(defvar world-clock-timer nil) Should this be given an internal name (and docstring)? > (defun world-clock-display (alist) > "Replace current buffer text with times in various zones, based on ALIST." > (let ((inhibit-read-only t) > @@ -561,12 +563,20 @@ To turn off the world time display, go to the window and type `\\[quit-window]'. > (interactive) > (when (and world-clock-timer-enable > (not (get-buffer world-clock-buffer-name))) > - (run-at-time t world-clock-timer-second #'world-clock-update)) > + (setq world-clock-timer > + (run-at-time t world-clock-timer-second #'world-clock-update)) > + (add-hook 'kill-buffer-hook #'world-clock-cancel-timer)) Should this be hooked buffer-locally? > (pop-to-buffer world-clock-buffer-name) > (world-clock-display (time--display-world-list)) > (world-clock-mode) > (fit-window-to-buffer)) > > +(defun world-clock-cancel-timer () > + "Cancel the world clock timer." > + (when world-clock-timer > + (cancel-timer world-clock-timer) > + (setq world-clock-timer nil))) > + > (defun world-clock-update (&optional _arg _noconfirm) > "Update the `world-clock' buffer." > (if (get-buffer world-clock-buffer-name) Actually, doesn't world-clock-update already try to cancel itself when world-clock-buffer-name no longer names an existing buffer? It seems to me that world-clock-cancel-timer and world-clock-update are duplicating each other. Or am I missing something? Thanks, -- Basil ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: master d378615: Cancel timer when world-clock buffer is killed 2020-09-03 13:46 ` master d378615: Cancel timer when world-clock buffer is killed Basil L. Contovounesios @ 2020-09-03 17:22 ` Stefan Kangas 2020-09-04 9:58 ` Basil L. Contovounesios 0 siblings, 1 reply; 5+ messages in thread From: Stefan Kangas @ 2020-09-03 17:22 UTC (permalink / raw) To: Basil L. Contovounesios; +Cc: emacs-devel "Basil L. Contovounesios" <contovob@tcd.ie> writes: >> + (add-hook 'kill-buffer-hook #'world-clock-cancel-timer)) > > Should this be hooked buffer-locally? Yup. >> +(defvar world-clock-timer nil) > > Should this be given an internal name (and docstring)? > >> +(defun world-clock-cancel-timer () >> + "Cancel the world clock timer." >> + (when world-clock-timer >> + (cancel-timer world-clock-timer) >> + (setq world-clock-timer nil))) >> + > > It seems to me that world-clock-cancel-timer and world-clock-update are > duplicating each other. Or am I missing something? Oh, right... I've pushed a fixed version where I removed the above new variable and moved the code to cancel the timer from `world-clock-update' to `world-clock-cancel-timer'. (I couldn't just add `world-clock-cancel-timer' to `kill-buffer-hook' because the buffer still exists when the hook is run.) Thanks for the review! ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: master d378615: Cancel timer when world-clock buffer is killed 2020-09-03 17:22 ` Stefan Kangas @ 2020-09-04 9:58 ` Basil L. Contovounesios 2020-09-06 15:38 ` Stefan Kangas 0 siblings, 1 reply; 5+ messages in thread From: Basil L. Contovounesios @ 2020-09-04 9:58 UTC (permalink / raw) To: Stefan Kangas; +Cc: emacs-devel Stefan Kangas <stefankangas@gmail.com> writes: > "Basil L. Contovounesios" <contovob@tcd.ie> writes: > >>> + (add-hook 'kill-buffer-hook #'world-clock-cancel-timer)) >> >> Should this be hooked buffer-locally? > > Yup. > >>> +(defvar world-clock-timer nil) >> >> Should this be given an internal name (and docstring)? >> >>> +(defun world-clock-cancel-timer () >>> + "Cancel the world clock timer." >>> + (when world-clock-timer >>> + (cancel-timer world-clock-timer) >>> + (setq world-clock-timer nil))) >>> + >> >> It seems to me that world-clock-cancel-timer and world-clock-update are >> duplicating each other. Or am I missing something? > > Oh, right... I've pushed a fixed version where I removed the above new > variable and moved the code to cancel the timer from > `world-clock-update' to `world-clock-cancel-timer'. Why not keep the (internal) variable and pass it to cancel-timer directly instead of searching through timer-list for a particular entry? > (I couldn't just add `world-clock-cancel-timer' to `kill-buffer-hook' > because the buffer still exists when the hook is run.) > > Thanks for the review! Thanks for improving world-clock! :) -- Basil ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: master d378615: Cancel timer when world-clock buffer is killed 2020-09-04 9:58 ` Basil L. Contovounesios @ 2020-09-06 15:38 ` Stefan Kangas 2020-09-06 15:55 ` Basil L. Contovounesios 0 siblings, 1 reply; 5+ messages in thread From: Stefan Kangas @ 2020-09-06 15:38 UTC (permalink / raw) To: Basil L. Contovounesios; +Cc: emacs-devel "Basil L. Contovounesios" <contovob@tcd.ie> writes: > Why not keep the (internal) variable and pass it to cancel-timer > directly instead of searching through timer-list for a particular entry? Thanks for your attention to detail. It's a matter of taste, I suppose. While mucking around with this, I saw two timers at one point, so I just kept that code as is. (Although the particular issue causing two timers should be fixed now.) Perhaps this should just use `cancel-function-timers' though... Are there any strong reasons why keeping it in a variable is better? Maybe we just prefer that style? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: master d378615: Cancel timer when world-clock buffer is killed 2020-09-06 15:38 ` Stefan Kangas @ 2020-09-06 15:55 ` Basil L. Contovounesios 0 siblings, 0 replies; 5+ messages in thread From: Basil L. Contovounesios @ 2020-09-06 15:55 UTC (permalink / raw) To: Stefan Kangas; +Cc: emacs-devel Stefan Kangas <stefankangas@gmail.com> writes: > "Basil L. Contovounesios" <contovob@tcd.ie> writes: > >> Why not keep the (internal) variable and pass it to cancel-timer >> directly instead of searching through timer-list for a particular entry? > > Thanks for your attention to detail. > > It's a matter of taste, I suppose. While mucking around with this, I > saw two timers at one point, so I just kept that code as is. (Although > the particular issue causing two timers should be fixed now.) > > Perhaps this should just use `cancel-function-timers' though... > > Are there any strong reasons why keeping it in a variable is better? > Maybe we just prefer that style? I think it's the cleanest and least surprising approach. Matching based on function symbol (via cancel-function-timers) or name (as in the current world-clock-cancel-timer) runs the risk of cancelling something we didn't add. If we want to cancel a timer that we added, we should keep track of what we added, and explicitly cancel only that; at least that's what my idea of common sense dictates. Unless there's some benefit to the other approaches I'm not seeing? Thanks, -- Basil ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-06 15:55 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- [not found] <20200903105449.22858.36474@vcs0.savannah.gnu.org> [not found] ` <20200903105450.8CD2D20A15@vcs0.savannah.gnu.org> 2020-09-03 13:46 ` master d378615: Cancel timer when world-clock buffer is killed Basil L. Contovounesios 2020-09-03 17:22 ` Stefan Kangas 2020-09-04 9:58 ` Basil L. Contovounesios 2020-09-06 15:38 ` Stefan Kangas 2020-09-06 15:55 ` Basil L. Contovounesios
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).