Add `auto_refresh` property to DOMNode/Widget

[abey79]

This PR adds an auto_refresh property to DOMNode (and thus Widget) to implement the common case of setting an interval timer on refresh(). The auto_refresh property can be set to a a float value (interval) or None to stop the auto-refresh behaviour. It can be read to check the current refresh value.

Side changes:

• fixed missing event parameter for Clock.on_mount()
• added a test on cancelled status in MessagePump.close_messages()

TODO/TBD:

• Timer doesn't have an API to access the currently set interval. I'm using Timer._interval for the time being, which is obviously bad. (Suggestion: rename _interval to interval to make it public.)
• Repeatedly setting/changing/cancelling the auto_refresh value likely leaks asyncio tasks (and possibly Timer objects). In particular, I had to add the test on cancelled status in the close_messages() function such as to avoid a CancelledError upon app close (with ctrl-C).
• Missing tests.

[willmcgugan]

It does look like there is a flaw in the Timer API. It doesn't mange timers that don't have the same lifetime as the widget.

I have a suspicion that we would have to address that issue, before we did auto_refresh. Any thoughts?

[abey79]

At first sight, my suggestion would be the following:

• add MessagePump.stop_timer(timer)
• for implementation, change MessagePump._child_tasks into MessagePump._timers

Client code would use Timer instance returned by set_timer()/set_interval() as C-style opaque reference to call stop_timer() (unless, of course, they use the convenience auto_refresh API), which is consistent with the fact that Timer is "private" (_timer.py).

The proposed implementation removes the generalisation to multiple types of child tasks, but that may be good if that generalisation was premature. Any insight on that?

While making changes here, I'd suggest merging set_timer() and set_interval() into a single start_timer(..., repeat: int | None). The API would be a cleaner start_timer()/stop_timer(), and the nearly duplicate implementation would be removed. The minor API complexity increase is justifiable as user code would most often use auto_refresh anyways.

Sounds ok to you? Happy to update the PR with the above changes.

[willmcgugan]

While making changes here, I'd suggest merging set_timer() and set_interval() into a single start_timer(..., repeat: int | None).

Those methods were borrowed from Javascript. I would agree that logically they are kind of the same thing and single method makes sense, but I think there is greater benefit in familiarity for JS devs.

add MessagePump.stop_timer(timer) for implementation, change MessagePump._child_tasks into MessagePump._timers

I'm not keen on the first point. We have something very similar in Rich with progress bars, which return an integer task id. Python devs don't like it because they still need to store a reference to the progress bar in addition to the task id. I've had so many issues requesting an interface like task_id.stop() or task_id.update()

There are child tasks that are not timers. But child tasks were intended for things that essentially have the same lifetime as a Widget. Timers may have a much shorter lifetime, so we probably need an adjacent mechanism to manage such tasks.

I'm starting to suspect that now we've hammered it out, we might have to close this as something we can't do without addressing the deficiencies in timers. What do you think?

[abey79]

I do agree that stop_timer() isn't very Pythonic and a better Timer API would be superior (esp. if it'd have other uses for client code). This can be closed as it can trivially be remade once the better timer is there. (The missing event parameter in Clock.on_mount() might be worth fixing though.)