* 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 external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.