Address timing PR review feedback

Author: waywardmonkeys

masonry/examples/layers.rs

@@ -77,10 +77,10 @@ impl Widget for OverlayBox {
     ) {
         if let PointerEvent::Move(PointerUpdate { current, .. }) = event {
             self.last_cursor_pos = current.logical_point();
-            if let Some(timer) = self.hover_timer.take() {
-                ctx.cancel_timer(timer);
+            if self.layer_root_id.take().is_some() && ctx.is_hovered() && self.hover_timer.is_none()
+            {
+                self.hover_timer = Some(ctx.request_timer(Duration::from_millis(300)));
             }
-            self.hover_timer = Some(ctx.request_timer(Duration::from_millis(300)));
         }
     }
 
@@ -90,13 +90,24 @@ impl Widget for OverlayBox {
 
     fn update(&mut self, ctx: &mut UpdateCtx<'_>, _props: &mut PropertiesMut<'_>, event: &Update) {
         match event {
+            Update::HoveredChanged(true)
+                if self.layer_root_id.is_none() && self.hover_timer.is_none() =>
+            {
+                self.hover_timer = Some(ctx.request_timer(Duration::from_millis(300)));
+            }
             Update::HoveredChanged(false) => {
                 if let Some(timer) = self.hover_timer.take() {
                     ctx.cancel_timer(timer);
                 }
+                if let Some(layer) = self.layer_root_id.take() {
+                    ctx.remove_layer(layer);
+                }
             }
-            Update::Timer(token) if Some(*token) == self.hover_timer => {
+            Update::TimerExpired(token) if Some(*token) == self.hover_timer => {
                 self.hover_timer = None;
+                if !ctx.is_hovered() {
+                    return;
+                }
                 let (overlay, layer_type) = (self.overlayer)();
                 self.layer_root_id = Some(overlay.id());
                 let layer_pos = self.last_cursor_pos + Vec2::new(5., -25.);

masonry/src/doc/implementing_widget.md

@@ -48,7 +48,7 @@ In the course of a frame, Masonry will run a series of passes over the widget tr
 - `on_pointer_event`, `on_text_event` and `on_access_event` are called once after a user-initiated event (like a mouse click or keyboard input).
 - `on_anim_frame` is called once per frame for animated widgets.
 - `update` is called many times during a frame, with various events reflecting changes in the widget's state (for instance, it gets or loses text focus).
-- `update` is also where timers requested by the widget are delivered as `Update::Timer`.
+- `update` is also where timers requested by the widget are delivered as `Update::TimerExpired`.
 - `measure` and `layout` are called during Masonry's layout pass.
   `measure` computes the preferred size of the widget on a single axis.
   `layout` receives a chosen size and lays out its children accordingly.
@@ -165,8 +165,8 @@ impl Widget for ColorRectangle {
 
 Use `on_anim_frame` for visual state that should advance with frame cadence, such as continuous motion or interpolation.
 For delayed one-shot behavior, use a timer instead.
-For example, a widget can store an `Option<TimerToken>`, set it with `ctx.request_timer(delay)`, then compare it against `Update::Timer(token)` in `update`.
-If the delayed behavior should continue, request a new one-shot timer from that `Update::Timer` branch.
+For example, a widget can store an `Option<TimerToken>`, set it with `ctx.request_timer(delay)`, then compare it against `Update::TimerExpired(token)` in `update`.
+If the delayed behavior should continue, request a new one-shot timer from that `Update::TimerExpired` branch.
 Cancel the stored token with `ctx.cancel_timer(token)` when the state that made the timer relevant no longer applies.
 
 ### Layout

masonry/src/tests/update.rs

@@ -12,8 +12,8 @@ use crate::core::{
 };
 use crate::layout::{AsUnit, Length};
 use crate::testing::{
-    DebugName, ModularWidget, PRIMARY_MOUSE, Record, TestHarness, TestWidgetExt, assert_any,
-    assert_debug_panics,
+    DebugName, ModularWidget, PRIMARY_MOUSE, Record, TestHarness, TestWidgetExt, assert_all,
+    assert_any, assert_debug_panics,
 };
 use crate::theme::test_property_set;
 use crate::util::Duration;
@@ -81,22 +81,18 @@ fn timer_update_targets_widget() {
     harness.flush_records_of(target_tag);
     harness.flush_records_of(parent_tag);
 
-    harness.set_timer_time(Duration::from_millis(10));
-    assert_eq!(harness.handle_timers(), 1);
+    harness.handle_timers(Duration::from_millis(10));
 
     let records = harness.take_records_of(target_tag);
     assert_any(
         records,
-        |record| matches!(record, Record::Update(Update::Timer(seen)) if seen == token),
+        |record| matches!(record, Record::Update(Update::TimerExpired(seen)) if seen == token),
     );
 
     let records = harness.take_records_of(parent_tag);
-    assert!(
-        !records
-            .iter()
-            .any(|record| matches!(record, Record::Update(Update::Timer(_)))),
-        "timer update should not bubble to the parent"
-    );
+    assert_all(records, |record| {
+        !matches!(record, Record::Update(Update::TimerExpired(_)))
+    });
 }
 
 #[test]
@@ -113,16 +109,12 @@ fn cancelled_timer_does_not_fire() {
     });
     harness.flush_records_of(target_tag);
 
-    harness.set_timer_time(Duration::from_millis(10));
-    assert_eq!(harness.handle_timers(), 0);
+    harness.handle_timers(Duration::from_millis(10));
 
     let records = harness.take_records_of(target_tag);
-    assert!(
-        !records
-            .iter()
-            .any(|record| matches!(record, Record::Update(Update::Timer(_)))),
-        "cancelled timer should not be delivered"
-    );
+    assert_all(records, |record| {
+        !matches!(record, Record::Update(Update::TimerExpired(_)))
+    });
 }
 
 #[test]

masonry/src/widgets/text_area.rs

@@ -30,6 +30,8 @@ const CURSOR_BLINK_INTERVAL: u64 = 500;
 const CURSOR_BLINK_TIME: u64 = CURSOR_BLINK_INTERVAL * 2;
 /// The timeout after which the cursor will stop blinking and stay solid.
 const CURSOR_BLINK_TIMEOUT: u64 = 10_000;
+// TODO: These should be read from system settings, but we currently
+// aren't aware of a robust way to read that cross-platform.
 
 /// `TextArea` implements the core of interactive text.
 ///
@@ -284,8 +286,7 @@ impl<const EDITABLE: bool> TextArea<EDITABLE> {
             self.anim_prev_interval = self.anim_prev_interval.rem_euclid(CURSOR_BLINK_TIME);
         }
 
-        let should_show = self.anim_elapsed >= CURSOR_BLINK_TIMEOUT
-            || self.anim_prev_interval < CURSOR_BLINK_INTERVAL;
+        let should_show = self.anim_elapsed >= CURSOR_BLINK_TIMEOUT || self.anim_prev_interval == 0;
         if self.anim_cursor_visible == should_show {
             false
         } else {
@@ -885,7 +886,7 @@ impl<const EDITABLE: bool> Widget for TextArea<EDITABLE> {
                 // We might need to use the disabled brush, and stop displaying the selection.
                 ctx.request_render();
             }
-            Update::Timer(token) if Some(*token) == self.cursor_blink_timer => {
+            Update::TimerExpired(token) if Some(*token) == self.cursor_blink_timer => {
                 self.cursor_blink_timer = None;
                 if ctx.is_window_focused() && ctx.is_focus_target() {
                     if self.advance_cursor_blink() {

masonry/src/widgets/text_input.rs

@@ -414,8 +414,7 @@ mod tests {
         assert_render_snapshot!(harness, "text_input_selection_unfocused");
 
         harness.process_text_event(TextEvent::WindowFocusChange(true));
-        harness.set_timer_time(Duration::from_millis(500));
-        harness.handle_timers();
+        harness.handle_timers(Duration::from_millis(500));
 
         assert_render_snapshot!(harness, "text_input_cursor_blink");
     }

masonry_core/src/app/render_root.rs

@@ -48,10 +48,6 @@ use crate::util::Duration;
 /// IME area as the `last_sent_ime_area`.
 const INVALID_IME_AREA: Rect = Rect::new(f64::NAN, f64::NAN, f64::NAN, f64::NAN);
 
-fn duration_to_timer_ticks(duration: Duration) -> TimerInstant {
-    u64::try_from(duration.as_nanos()).unwrap_or(u64::MAX)
-}
-
 // --- MARK: STRUCTS
 
 /// The composition root of Masonry.
@@ -980,6 +976,10 @@ impl RenderRoot {
     /// The time is measured from an origin chosen by the host. All calls to
     /// this method and [`next_timer_deadline`](Self::next_timer_deadline) must
     /// use the same origin.
+    ///
+    /// This is separate from [`handle_timers`](Self::handle_timers) because timer
+    /// requests made while handling normal events also need to be scheduled
+    /// relative to the current host time.
     pub fn set_timer_time(&mut self, time: Duration) {
         self.global_state.timer_now = duration_to_timer_ticks(time);
     }
@@ -994,9 +994,8 @@ impl RenderRoot {
 
     /// Delivers all timers due at or before the current timer time.
     ///
-    /// Returns the number of timers delivered. Timers targeting removed widgets
-    /// are discarded.
-    pub fn handle_timers(&mut self) -> usize {
+    /// Timers targeting removed widgets are discarded.
+    pub fn handle_timers(&mut self) {
         let now = self.global_state.timer_now;
         let mut fired = Vec::new();
         // Pop the due set before dispatch. Widget updates may request or cancel
@@ -1007,18 +1006,16 @@ impl RenderRoot {
             fired.push((timer.into_target(), token));
         }
 
-        let mut delivered = 0;
+        let had_due_timer = !fired.is_empty();
         for (target, token) in fired {
             if !self.widget_arena.has(target) {
                 continue;
             }
             run_update_timer_pass(self, target, token);
-            delivered += 1;
         }
-        if delivered != 0 {
+        if had_due_timer {
             self.run_rewrite_passes();
         }
-        delivered
     }
 
     /// Returns true if the accessibility tree needs to be rebuilt.
@@ -1091,6 +1088,10 @@ impl RenderRootState {
     }
 }
 
+fn duration_to_timer_ticks(duration: Duration) -> TimerInstant {
+    u64::try_from(duration.as_nanos()).unwrap_or(u64::MAX)
+}
+
 impl RenderRootSignal {
     pub(crate) fn new_ime_moved_signal(area: Rect) -> Self {
         Self::ImeMoved(

masonry_core/src/core/contexts.rs

@@ -441,11 +441,11 @@ impl_context_method!(
     {
         /// Requests a timer for the current widget.
         ///
-        /// When the timer expires, this widget receives [`Update::Timer`] with
+        /// When the timer expires, this widget receives [`Update::TimerExpired`] with
         /// the returned token. Timer delivery is best-effort: timers targeting
         /// removed widgets are ignored by the host.
         ///
-        /// [`Update::Timer`]: crate::core::Update::Timer
+        /// [`Update::TimerExpired`]: crate::core::Update::TimerExpired
         pub fn request_timer(&mut self, delay: Duration) -> TimerToken {
             self.global_state.request_timer(self.widget_id(), delay)
         }

masonry_core/src/core/events.rs

@@ -14,7 +14,7 @@ use crate::util::Duration;
 ///
 /// Timer tokens are assigned by a [`RenderRoot`](crate::app::RenderRoot)'s
 /// timer queue. A widget can keep the token to recognize a later
-/// [`Update::Timer`] or cancel the timer before it fires.
+/// [`Update::TimerExpired`] or cancel the timer before it fires.
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 pub struct TimerToken(pub(crate) TimerId);
 
@@ -137,7 +137,7 @@ pub enum Update {
     ChildHoveredChanged(bool),
 
     /// Called when a timer requested by this widget expires.
-    Timer(TimerToken),
+    TimerExpired(TimerToken),
 
     /// Called when the [active] status of the current widget changes.
     ///
@@ -303,7 +303,7 @@ impl Update {
             Self::FocusChanged(true) => "FocusChanged(true)",
             Self::ChildFocusChanged(true) => "ChildFocusChanged(true)",
             Self::RequestPanToChild(_) => "RequestPanToChild(_)",
-            Self::Timer(_) => "Timer(_)",
+            Self::TimerExpired(_) => "TimerExpired(_)",
             Self::FontsChanged => "FontsChanged",
         }
     }

masonry_core/src/doc/pass_system.md

@@ -51,7 +51,7 @@ The animation pass may be considered as a special event pass: it's not triggered
 ### Timer delivery
 
 Widget timers are delayed callbacks requested with context methods such as [`UpdateCtx::request_timer`].
-When a timer expires, Masonry delivers [`Update::Timer`] to the widget that requested it.
+When a timer expires, Masonry delivers [`Update::TimerExpired`] to the widget that requested it.
 
 Timer delivery does not bubble to ancestors.
 Like the animation pass, it is triggered externally by the host event loop and sets off the rewrite passes after delivery.
@@ -265,7 +265,7 @@ They can access the layout of children if they have already been laid out.
 [`RegisterCtx`]: crate::core::RegisterCtx
 [`QueryCtx`]: crate::core::QueryCtx
 [`WidgetAdded`]: crate::core::Update::WidgetAdded
-[`Update::Timer`]: crate::core::Update::Timer
+[`Update::TimerExpired`]: crate::core::Update::TimerExpired
 [`UpdateCtx::request_timer`]: crate::core::UpdateCtx::request_timer
 [`Ime::Disabled`]: crate::core::Ime::Disabled
 [`FocusChanged`]: crate::core::Update::FocusChanged

masonry_core/src/passes/update.rs

@@ -1248,11 +1248,14 @@ pub(crate) fn run_update_fonts_pass(root: &mut RenderRoot) {
     );
 }
 
+// ----------------
+
+// --- MARK: TIMERS
 pub(crate) fn run_update_timer_pass(root: &mut RenderRoot, target: WidgetId, token: TimerToken) {
     let _span = info_span!("update_timer").entered();
 
     run_single_update_pass(root, Some(target), |widget, ctx, props| {
-        widget.update(ctx, props, &Update::Timer(token));
+        widget.update(ctx, props, &Update::TimerExpired(token));
     });
 }