Re: master d378615: Cancel timer when world-clock buffer is killed

Re: master d378615: Cancel timer when world-clock buffer is killed

[Basil L. Contovounesios]

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

Re: master d378615: Cancel timer when world-clock buffer is killed

[Stefan Kangas]

"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!

Re: master d378615: Cancel timer when world-clock buffer is killed

[Basil L. Contovounesios]

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

Re: master d378615: Cancel timer when world-clock buffer is killed

[Stefan Kangas]

"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?

Re: master d378615: Cancel timer when world-clock buffer is killed

[Basil L. Contovounesios]

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