Add widget timers to Masonry and use them for delayed UI behavior. - #1779
Add widget timers to Masonry and use them for delayed UI behavior.#1779waywardmonkeys wants to merge 5 commits into
Conversation
|
This was done with LLM assistance to avoid typing out the boring stuff. Draft since I will publish |
8e792bf to
cfeb24c
Compare
xStrom
left a comment
There was a problem hiding this comment.
Timers are such a great feature to have, thanks for moving this forward.
The TextInput cursor is also much less buggy with this implementation compared to the old one. For example, losing focus when the cursor is not shown doesn't cause it disappear for good anymore. 😅
| // TODO: These should be reading from the system settings, but we currently | ||
| // aren't aware of a robust way to read that cross-platform. |
There was a problem hiding this comment.
Let's preserve this comment near the top area where these consts now live.
| let should_show = self.anim_elapsed >= CURSOR_BLINK_TIMEOUT | ||
| || self.anim_prev_interval < CURSOR_BLINK_INTERVAL; |
There was a problem hiding this comment.
The self.anim_prev_interval < CURSOR_BLINK_INTERVAL check seems needlessly misleading, the only scenario that satisfies this is self.anim_prev_interval == 0.
| if let Some(timer) = self.hover_timer.take() { | ||
| ctx.cancel_timer(timer); | ||
| } | ||
| self.hover_timer = Some(ctx.request_timer(Duration::from_millis(300))); |
There was a problem hiding this comment.
I know this replicated the old logic of showing the layer after the mouse stopped moving for 300ms, but I think this is a good opportunity to improve things.
Canceling and requesting a new timer for every mouse move position is kind of wasteful, including understory_timing always looping over the whole set of timers for the cancel. Also, usually tooltips are shown after hovering for a certain amount of time, not after stopping movement. This is certainly how Windows tooltips work.
Thus I think we should update the code. Perhaps just having Update::HoveredChanged(true) request the timer and HoveredChanged(false) cancel it.
There was a problem hiding this comment.
Perhaps just having Update::HoveredChanged(true) request the timer and HoveredChanged(false) cancel it.
I'm all for finding a better solution, but I'll note that this does not match how tooltips work in most frameworks.
PoignardAzur
left a comment
There was a problem hiding this comment.
Timers are long-overdue in Masonry. Glad to finally have an implementation!
I have doubts about the API, but the internals seem sound.
| pub fn set_timer_time(&mut self, time: Duration) { | ||
| self.global_state.timer_now = duration_to_timer_ticks(time); | ||
| } |
There was a problem hiding this comment.
Is there a benefit to splitting this from handle_timers? As far as I can tell, all call sites call them both at once.
This seems like a surprising pattern, we don't do this anywhere else (we don't call one method to set the mouse position and another to trigger all mouse events).
There was a problem hiding this comment.
I wrote up a whole thing ... and thought it was still complicated ... so going to revisit this code soon even further. I think there's a better / simpler solution (on the surface, it might need some other changes internally, we'll see).
| /// Returns the next timer deadline, measured from the host's timer origin. | ||
| pub fn next_timer_deadline(&self) -> Option<Duration> { |
There was a problem hiding this comment.
So as far as I can tell, the reason to use Duration instead of Instant is that Instant can't be mocked in tests? If so, this should probably be documented in either a doc comment or a code comment.
There was a problem hiding this comment.
Yes, that was the reason and there's a comment there. But I may revisit this in a moment due to some other simplifications.
| fired.push((timer.into_target(), token)); | ||
| } | ||
|
|
||
| let mut delivered = 0; |
There was a problem hiding this comment.
I don't think keeping track of delivered is very useful. It's used to short-circuit rewrite passes in RenderRoot and signal handling in EventLoop, but both of those already have short-circuits built in. I'd recommend dropping it.
There was a problem hiding this comment.
Done, I think.
| /// Sets the simulated timer time. | ||
| pub fn set_timer_time(&mut self, time: Duration) { | ||
| self.render_root.set_timer_time(time); | ||
| } |
There was a problem hiding this comment.
As above, I think this should be merged with handle_timers.
In fact, I'm thinking maybe this should be merged with the animate_ms method above into a move_time_forward method, because a situation where animations progress but timers don't is unrealistic.
|
Thanks for the feedback. I'm heading out for the evening shortly but will respond with updates within the next day or two. I didn't know about |
Add `understory_timing` as a `masonry_core` dependency and keep the timer queue inside `RenderRoot` so widget timer requests synchronously return the queue-assigned identity. Expose timer time/deadline/drain hooks for backends, and update `masonry_winit` to own only the host clock origin and platform wakeup scheduling. Add focused tests for targeted timer delivery and cancellation.
Replace `TextArea`'s animation-frame polling for caret blinking with one-shot widget timers. Focus and text activity reset the blink state and schedule a half-cycle timer; timer updates toggle the caret and reschedule while the text area remains focused. Update the text input cursor blink snapshot test to advance the simulated timer clock instead of sending an animation frame.
Replace the layers example's animation-frame polling loop with a one-shot hover timer. Pointer movement cancels and reschedules the delay, hover exit cancels it, and the timer update creates the tooltip layer once the cursor has settled.
Clarify that `request_anim_frame` and `Widget::on_anim_frame` are for frame-cadenced visual updates, while delayed one-shot UI behavior should use widget timers. Update the pass-system and widget-implementation docs to describe timer delivery through `Update::Timer`, the non-bubbling delivery model, and the usual `Option<TimerToken>` request/cancel pattern.
cfeb24c to
e55ee2d
Compare
|
This is now using the published version 0.1.2 of |
| use winit::event::StartCause; | ||
| use winit::event::{DeviceEvent as WinitDeviceEvent, DeviceId, WindowEvent as WinitWindowEvent}; |
There was a problem hiding this comment.
| use winit::event::StartCause; | |
| use winit::event::{DeviceEvent as WinitDeviceEvent, DeviceId, WindowEvent as WinitWindowEvent}; | |
| use winit::event::{ | |
| DeviceEvent as WinitDeviceEvent, DeviceId, StartCause, WindowEvent as WinitWindowEvent, | |
| }; |
PoignardAzur
left a comment
There was a problem hiding this comment.
Again, I have nitpicks about the API, but I'm fine merging now and fixing them later.
LGTM.
understory_timingas amasonry_coredependency.RenderRoot.TimerTokenand deliver expired timers throughUpdate::Timer.request_timer(delay) -> TimerTokencancel_timer(token)RenderRoot::set_timer_timeRenderRoot::next_timer_deadlineRenderRoot::handle_timersmasonry_winitto drive timer time and platform wakeups.TextAreacursor blinking to one-shot timers.Rationale
Masonry has several UI behaviors that are delayed but not frame-cadenced. Examples include cursor blinking, tooltip
delays, debounce, and long press recognition. These fit a timer model better than animation frames.
This PR adds a widget-local timer lifecycle: widgets request a timer, keep the returned
TimerToken, and receiveUpdate::Timerwhen it expires. Widgets can cancel pending timers when the relevant state changes.Animation frames remain the right tool for continuous visual updates, such as interpolation or motion that should advance with frame cadence.