Fix timer requests in update and lifecycle. (#964)

CHANGELOG.md

@@ -117,6 +117,7 @@ This means that druid no longer requires cairo on macOS and uses Core Graphics i
 - Built-in widgets no longer stroke outside their `paint_rect`. ([#861] by [@jneem])
 - `Switch` toggles with animation when its data changes externally. ([#898] by [@finnerale])
 - Render progress bar correctly. ([#949] by [@scholtzan])
+- Scrollbars animate when the scroll container size changes. ([#964] by [@xStrom])
 
 ### Docs
 
@@ -222,6 +223,7 @@ This means that druid no longer requires cairo on macOS and uses Core Graphics i
 [#959]: https://github.com/xi-editor/druid/pull/959
 [#961]: https://github.com/xi-editor/druid/pull/961
 [#963]: https://github.com/xi-editor/druid/pull/963
+[#964]: https://github.com/xi-editor/druid/pull/964
 [#967]: https://github.com/xi-editor/druid/pull/967
 [#969]: https://github.com/xi-editor/druid/pull/969
 [#970]: https://github.com/xi-editor/druid/pull/970

druid/src/contexts.rs

@@ -896,7 +896,6 @@ impl<'a> ContextState<'a> {
     }
 
     fn request_timer(&self, widget_state: &mut WidgetState, deadline: Duration) -> TimerToken {
-        widget_state.request_timer = true;
         let timer_token = self.window.request_timer(deadline);
         widget_state.add_timer(timer_token);
         timer_token

druid/src/core.rs

@@ -15,14 +15,14 @@
 //! The fundamental druid types.
 
 use std::collections::{HashMap, VecDeque};
-use std::mem;
 
 use crate::bloom::Bloom;
 use crate::contexts::ContextState;
 use crate::kurbo::{Affine, Insets, Point, Rect, Shape, Size, Vec2};
 use crate::piet::{
     FontBuilder, PietTextLayout, RenderContext, Text, TextLayout, TextLayoutBuilder,
 };
+use crate::util::ExtendDrain;
 use crate::{
     BoxConstraints, Color, Command, Data, Env, Event, EventCtx, InternalEvent, InternalLifeCycle,
     LayoutCtx, LifeCycle, LifeCycleCtx, PaintCtx, Region, Target, TimerToken, UpdateCtx, Widget,
@@ -105,12 +105,6 @@ pub(crate) struct WidgetState {
     /// Any descendant has requested an animation frame.
     pub(crate) request_anim: bool,
 
-    /// Any descendant has requested a timer.
-    ///
-    /// Note: we don't have any way of clearing this request, as it's
-    /// likely not worth the complexity.
-    pub(crate) request_timer: bool,
-
     pub(crate) focus_chain: Vec<WidgetId>,
     pub(crate) request_focus: Option<FocusChange>,
     pub(crate) children: Bloom<WidgetId>,
@@ -902,7 +896,6 @@ impl WidgetState {
             has_active: false,
             has_focus: false,
             request_anim: false,
-            request_timer: false,
             request_focus: None,
             focus_chain: Vec::new(),
             children: Bloom::new(),
@@ -930,19 +923,11 @@ impl WidgetState {
 
         self.needs_layout |= child_state.needs_layout;
         self.request_anim |= child_state.request_anim;
-        self.request_timer |= child_state.request_timer;
         self.has_active |= child_state.has_active;
         self.has_focus |= child_state.has_focus;
         self.children_changed |= child_state.children_changed;
         self.request_focus = child_state.request_focus.take().or(self.request_focus);
-
-        if !child_state.timers.is_empty() {
-            if self.timers.is_empty() {
-                mem::swap(&mut self.timers, &mut child_state.timers);
-            } else {
-                self.timers.extend(&mut child_state.timers.drain());
-            }
-        }
+        self.timers.extend_drain(&mut child_state.timers);
     }
 
     #[inline]

druid/src/lib.rs

@@ -135,6 +135,7 @@ mod mouse;
 mod tests;
 pub mod text;
 pub mod theme;
+mod util;
 pub mod widget;
 mod win_handler;
 mod window;

druid/src/util.rs

@@ -0,0 +1,52 @@
+// Copyright 2020 The xi-editor Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! Miscellaneous utility functions.
+
+use std::collections::HashMap;
+use std::hash::Hash;
+use std::mem;
+
+/// Fast path for equal type extend + drain.
+pub trait ExtendDrain {
+    /// Extend the collection by draining the entries from `source`.
+    ///
+    /// This function may swap the underlying memory locations,
+    /// so keep that in mind if one of the collections has a large allocation
+    /// and it should keep that allocation.
+    fn extend_drain(&mut self, source: &mut Self);
+}
+
+impl<K, V> ExtendDrain for HashMap<K, V>
+where
+    K: Eq + Hash + Copy,
+    V: Copy,
+{
+    // Benchmarking this vs just extend+drain with a 10k entry map.
+    //
+    // running 2 tests
+    // test bench_extend       ... bench:       1,971 ns/iter (+/- 566)
+    // test bench_extend_drain ... bench:           0 ns/iter (+/- 0)
+    fn extend_drain(&mut self, source: &mut Self) {
+        if !source.is_empty() {
+            if self.is_empty() {
+                // If the target is empty we can just swap the pointers.
+                mem::swap(self, source);
+            } else {
+                // Otherwise we need to fall back to regular extend-drain.
+                self.extend(source.drain());
+            }
+        }
+    }
+}

druid/src/widget/scroll.rs

@@ -176,12 +176,15 @@ impl<T, W: Widget<T>> Scroll<T, W> {
     }
 
     /// Makes the scrollbars visible, and resets the fade timer.
-    pub fn reset_scrollbar_fade(&mut self, ctx: &mut EventCtx, env: &Env) {
+    pub fn reset_scrollbar_fade<F>(&mut self, request_timer: F, env: &Env)
+    where
+        F: FnOnce(Duration) -> TimerToken,
+    {
         // Display scroll bars and schedule their disappearance
         self.scrollbars.opacity = env.get(theme::SCROLLBAR_MAX_OPACITY);
         let fade_delay = env.get(theme::SCROLLBAR_FADE_DELAY);
         let deadline = Duration::from_millis(fade_delay);
-        self.scrollbars.timer_id = ctx.request_timer(deadline);
+        self.scrollbars.timer_id = request_timer(deadline);
     }
 
     /// Returns the current scroll offset.
@@ -344,7 +347,7 @@ impl<T: Data, W: Widget<T>> Widget<T> for Scroll<T, W> {
 
                     if !scrollbar_is_hovered {
                         self.scrollbars.hovered = BarHoveredState::None;
-                        self.reset_scrollbar_fade(ctx, env);
+                        self.reset_scrollbar_fade(|d| ctx.request_timer(d), env);
                     }
                 }
                 _ => (), // other events are a noop
@@ -395,7 +398,7 @@ impl<T: Data, W: Widget<T>> Widget<T> for Scroll<T, W> {
                     // if we have just stopped hovering
                     if self.scrollbars.hovered.is_hovered() && !scrollbar_is_hovered {
                         self.scrollbars.hovered = BarHoveredState::None;
-                        self.reset_scrollbar_fade(ctx, env);
+                        self.reset_scrollbar_fade(|d| ctx.request_timer(d), env);
                     }
                 }
                 Event::Timer(id) if *id == self.scrollbars.timer_id => {
@@ -412,24 +415,29 @@ impl<T: Data, W: Widget<T>> Widget<T> for Scroll<T, W> {
                 if self.scroll(mouse.wheel_delta, size) {
                     ctx.request_paint();
                     ctx.set_handled();
-                    self.reset_scrollbar_fade(ctx, env);
+                    self.reset_scrollbar_fade(|d| ctx.request_timer(d), env);
                 }
             }
         }
     }
 
     fn lifecycle(&mut self, ctx: &mut LifeCycleCtx, event: &LifeCycle, data: &T, env: &Env) {
-        // Guard by the timer id being invalid, otherwise the scroll bars would fade
-        // immediately if some other widgeet started animating.
-        if let LifeCycle::AnimFrame(interval) = event {
-            if self.scrollbars.timer_id == TimerToken::INVALID {
-                // Animate scroll bars opacity
-                let diff = 2.0 * (*interval as f64) * 1e-9;
-                self.scrollbars.opacity -= diff;
-                if self.scrollbars.opacity > 0.0 {
-                    ctx.request_anim_frame();
+        match event {
+            LifeCycle::AnimFrame(interval) => {
+                // Guard by the timer id being invalid, otherwise the scroll bars would fade
+                // immediately if some other widgeet started animating.
+                if self.scrollbars.timer_id == TimerToken::INVALID {
+                    // Animate scroll bars opacity
+                    let diff = 2.0 * (*interval as f64) * 1e-9;
+                    self.scrollbars.opacity -= diff;
+                    if self.scrollbars.opacity > 0.0 {
+                        ctx.request_anim_frame();
+                    }
                 }
             }
+            // Show the scrollbars any time our size changes
+            LifeCycle::Size(_) => self.reset_scrollbar_fade(|d| ctx.request_timer(d), &env),
+            _ => (),
         }
         self.child.lifecycle(ctx, event, data, env)
     }

druid/src/window.rs

@@ -26,6 +26,7 @@ use crate::shell::{Counter, Cursor, WindowHandle};
 
 use crate::contexts::ContextState;
 use crate::core::{CommandQueue, FocusChange, WidgetState};
+use crate::util::ExtendDrain;
 use crate::widget::LabelText;
 use crate::win_handler::RUN_COMMANDS_TOKEN;
 use crate::{
@@ -119,12 +120,12 @@ impl<T: Data> Window<T> {
 
     fn post_event_processing(
         &mut self,
+        widget_state: &mut WidgetState,
         queue: &mut CommandQueue,
         data: &T,
         env: &Env,
         process_commands: bool,
     ) {
-        let widget_state = self.root.state();
         // If children are changed during the handling of an event,
         // we need to send RouteWidgetAdded now, so that they are ready for update/layout.
         if widget_state.children_changed {
@@ -136,6 +137,8 @@ impl<T: Data> Window<T> {
                 false,
             );
         }
+        // Add all the requested timers to the window's timers map.
+        self.timers.extend_drain(&mut widget_state.timers);
         // If there are any commands and they should be processed
         if process_commands && !queue.is_empty() {
             // Ask the handler to call us back on idle
@@ -207,6 +210,12 @@ impl<T: Data> Window<T> {
             ctx.is_handled
         };
 
+        // Clean up the timer token and do it immediately after the event handling
+        // because the token may be reused and re-added in a lifecycle pass below.
+        if let Event::Internal(InternalEvent::RouteTimer(token, _)) = event {
+            self.timers.remove(&token);
+        }
+
         if let Some(focus_req) = widget_state.request_focus.take() {
             let old = self.focus;
             let new = self.widget_for_focus_request(focus_req);
@@ -222,18 +231,7 @@ impl<T: Data> Window<T> {
             self.handle.set_cursor(&cursor);
         }
 
-        self.post_event_processing(queue, data, env, false);
-
-        //In some platforms, timer tokens are reused. So it is necessary to remove the token from
-        //the window's timer map before adding new tokens to it.
-        if let Event::Internal(InternalEvent::RouteTimer(token, _)) = event {
-            self.timers.remove(&token);
-        }
-
-        //If at least one widget requested a timer, add all the requested timers to window's timers map.
-        if widget_state.request_timer {
-            self.timers.extend(widget_state.timers);
-        }
+        self.post_event_processing(&mut widget_state, queue, data, env, false);
 
         is_handled
     }
@@ -246,30 +244,35 @@ impl<T: Data> Window<T> {
         env: &Env,
         process_commands: bool,
     ) {
+        let mut widget_state = WidgetState::new(self.root.id());
+        // AnimFrame has separate logic, and due to borrow checker restrictions
+        // it will also create its own LifeCycleCtx.
         if let LifeCycle::AnimFrame(_) = event {
-            self.do_anim_frame(queue, data, env)
+            self.do_anim_frame(&mut widget_state, queue, data, env);
         } else {
             let mut state = ContextState::new::<T>(queue, &self.handle, self.id);
-            let mut widget_state = WidgetState::new(self.root.id());
             let mut ctx = LifeCycleCtx {
                 state: &mut state,
                 widget_state: &mut widget_state,
             };
 
             self.root.lifecycle(&mut ctx, event, data, env);
         }
-
-        self.post_event_processing(queue, data, env, process_commands);
+        self.post_event_processing(&mut widget_state, queue, data, env, process_commands);
     }
 
     /// AnimFrame has special logic, so we implement it separately.
-    fn do_anim_frame(&mut self, queue: &mut CommandQueue, data: &T, env: &Env) {
+    fn do_anim_frame(
+        &mut self,
+        widget_state: &mut WidgetState,
+        queue: &mut CommandQueue,
+        data: &T,
+        env: &Env,
+    ) {
         let mut state = ContextState::new::<T>(queue, &self.handle, self.id);
-
-        let mut widget_state = WidgetState::new(self.root.id());
         let mut ctx = LifeCycleCtx {
             state: &mut state,
-            widget_state: &mut widget_state,
+            widget_state,
         };
 
         // TODO: this calculation uses wall-clock time of the paint call, which
@@ -298,7 +301,7 @@ impl<T: Data> Window<T> {
         };
 
         self.root.update(&mut update_ctx, data, env);
-        self.post_event_processing(queue, data, env, false);
+        self.post_event_processing(&mut widget_state, queue, data, env, false);
     }
 
     pub(crate) fn invalidate_and_finalize(&mut self) {
@@ -353,7 +356,7 @@ impl<T: Data> Window<T> {
             env,
             Rect::from_origin_size(Point::ORIGIN, size),
         );
-        self.post_event_processing(queue, data, env, true);
+        self.post_event_processing(&mut widget_state, queue, data, env, true);
     }
 
     /// only expose `layout` for testing; normally it is called as part of `do_paint`