Fix timer requests in update and lifecycle. by xStrom · Pull Request #964 · linebender/druid

xStrom · 2020-05-21T11:04:46Z

This PR fixes the handling of timer requests. Previously they were only collected in the window's event but I moved the collection to the general post processing step. I also removed the legacy request_timer bool which no longer served a purpose and was never being reset.

This also finally allowed to address #834. Scrollbars now briefly animate when the container is resized. This can be seen with e.g. the scroll_colors example when you resize the window.

Fixes #834.

cmyr

Overall this looks totally reasonable, but I'm curious about processing events after anim_frame. In particular, I don't think we should be sending any events between anim_frame and paint?

cmyr · 2020-05-21T13:54:52Z

druid/src/widget/scroll.rs


    /// 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)


Yea, this is annoying.

Something I've been wondering is whether we could have certain traits for behaviour that is common to (most?) contexts, and then for things like requesting a timer or sending a command you could have the argument be ctx: &mut dyn ContextCommon or something...

Yeap, I was thinking of the same thing based on the amount of code duplication we have in context.rs. Then having to do this request_timer closure here really boosted that thinking. I have it in my notes to attempt to better the situation with traits. (Would be a separate PR.)

I am accidentally working on this right now.

cmyr · 2020-05-21T13:59:39Z

druid/src/util.rs

+    K: Eq + Hash + Copy,
+    V: Copy,
+{
+    fn extend_drain(&mut self, source: &mut Self) {


just an observation: this is the kind of thing I'm always curious to profile, or at least take a look at the asm; always room for funny surprises. I certainly suspect its an improvement, but it is a suspicion, it's not impossible the compiler generating code like this as an optimization already.

There was a funny surprise alright. The empty target case is 0ns with this optimized function, which is a win. However unexpectedly even the slow path is faster! The slow path is faster only with drain though. If I bench extending with just iter vs iter, then the slow paths are equal. Some sort of non-generic code win I guess.

neat, thanks for measuring!

cmyr · 2020-05-21T14:03:18Z

druid/src/window.rs

            self.last_anim = Some(now);
        }
+
+        self.post_event_processing(&mut base_state, queue, data, env, process_commands);


Is there a specific reason we think this is necessary? do_anim_frame is part of paint, and paint calls this already.

It's necessary because a widget might call submit_command or request_timer in response to AnimFrame.

do_anim_frame is currently part of lifecycle not paint and so even if paint called post_event_processing that wouldn't be guaranteed for other AnimFrame calls to lifecycle.

paint doesn't call post_event_processing.

AnimFrame does not guarantee paint in the future as discussed on zulip.

in order:

to my mind, AnimFrame should only be used to update state in preparation for an imminent paint call. This is a weird case; AnimFrame is one of the bits of event flow that is really a poor fit to the current architecture, and when (like.... a year ago? 😬) I was playing around with a higher-level animation API, one of the changes was to just make a prepare_for_animation method on Widget that replaced AnimFrame. submit_command and request_timer should be discouraged here, and it's annoying that this isn't enforced by the type system.

do_anim_frame sort of is part of paint; LifeCycle::AnimFrame should only be sent from Window::do_paint, if it happens anywhere else that's a mistake.

this checks out, I must've misread again. That said, it looks like we call post_event_processing in Window::lifecycle, which is what calls do_anim_frame`?

I think this is false? it's not really that anim guarantees paint, it's that AnimFrame is only called from the painting code path.

Yes we do call post_event_processing in Window::lifecycle right now, which means it also applies to do_anim_frame. However due to borrow checking limitations, in order to keep that functionality, I had to add it to do_anim_frame because the Window::lifecycle call moved into an if block.

Yeah right now AnimFrame is sent just before paint. However based on that zulip discussion this may change. @raphlinus said It's up to the handler of that lifecycle event to request painting - from which I derive that if the handler doesn't request painting, the painting might not happen at all. (This is for future behavior, today that's not true.)

More generally, moving AnimFrame out of lifecycle makes sense if it's supposed to have different limitations. However as long as it's part of lifecycle I think it should behave like a lifecycle event. That includes a working request_timer and submit_command.

On the second point: I think there's a confusion about raph's comments. AnimFrame does mean you're going to paint; there's no short-circuiting. If you see it, it means a call to paint is next. To continue the animation, you do need to request another frame.

I'll take a look at the code you're talking about, I might've missed that.

cmyr · 2020-05-22T17:27:23Z

druid/src/window.rs

            };

            self.root.lifecycle(&mut ctx, event, data, env);
+            self.post_event_processing(&mut base_state, queue, data, env, process_commands);


we can move this out of the if branch if we move the base_state decl to above the block, this should solve our other little confusion?

Yes! That will work.

cmyr

looks good. I'm wondering if, for future work, we can actually just get rid of do_anin_frame at some point? it just seems like more confusion than its worth maybe.

xStrom added the S-needs-review waits for review label May 21, 2020

cmyr reviewed May 21, 2020

View reviewed changes

xStrom force-pushed the scroll-size branch from 152e34a to fb35901 Compare May 21, 2020 17:04

cmyr reviewed May 22, 2020

View reviewed changes

xStrom force-pushed the scroll-size branch from fb35901 to a8abd94 Compare May 22, 2020 17:46

cmyr approved these changes May 22, 2020

View reviewed changes

Fix timer requests in update and lifecycle.

2a8917e

xStrom force-pushed the scroll-size branch from a8abd94 to 2a8917e Compare May 22, 2020 19:07

xStrom removed the S-needs-review waits for review label May 22, 2020

xStrom merged commit 875ad40 into linebender:master May 22, 2020

xStrom deleted the scroll-size branch May 22, 2020 21:49

Conversation

xStrom commented May 21, 2020

Uh oh!

cmyr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmyr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants