Associate timers with widget ids #831

sjoshid · 2020-04-12T18:16:08Z

Closes #494

2) Aggressive traversal is achieved by storing a map of TimerToken -> WidgetId in each window. 3) On Timer event, we check the map from step 2 and recurse only if child MIGHT contain the WidgetId.

xStrom · 2020-04-12T18:42:35Z

I haven't looked at the code closely yet, but can't we do this association without BaseState? BaseState gets replicated a lot. I'm thinking of something like sending the WidgetId in with self.window.request_timer() and then the WidgetId gets stored close to wherever the TimerToken is stored. When the timer fires, we can have a RouteTimer{WidgetId, TimerToken} internal event that gets translated into the classic Timer event when it reaches the target widget.

I'll have more time to look into this tomorrow hopefully, but just wanted to get that thought out there.

sjoshid · 2020-04-12T18:45:30Z

druid/src/contexts.rs

+        timer_token
+    }
+
+    pub fn remove_timer(&mut self, timer_token: &TimerToken) {


After widgets are done processing timer events, should we all ask them to remove map entry?
This is not used anywhere in examples.
I think framework needs to remove it rather than forcing widgets to remove it themselves.

After merging up, Im clearing the map of BaseState. So remove is not needed when event is handled.

sjoshid · 2020-04-12T19:03:39Z

WidgetId gets stored close to wherever the TimerToken is stored.

That was my first attempt. TimerToken is stored in platform window. After some discussion, we decided not to change druid shell.

When the timer fires, we can have a RouteTimer{WidgetId, TimerToken} internal event that gets translated into the classic Timer event when it reaches the target widget

The objective of this was to do aggressive traversing of Timer event. We can't let the event reach every widget to find out if it is a target or not. So I let 'timer -> widget id' entries merge_up to root. And when the event is fired, we use this map along with maybe_contains to determine if we need to recurse. When maybe_contains is false, it saves us from recursing into a potentially deep subtree.

xStrom · 2020-04-12T19:25:13Z

Yeah druid-shell shouldn't be modified, but we could probably store the HashMap<TimerToken, WidgetId> in the druid window. 🤔

As for agressive traversing, you can still do that if you just know the WidgetId at the root, because you can still use may_contain to check if a path may contain the WidgetId.

sjoshid · 2020-04-12T19:41:39Z

Whatever you said is what this PR does.

…ot needed when event is handled.

xStrom

It seems that the HashMap in window is nevered cleaned up. We should remove the TimerToken entry from the window HashMap at the end of the window's event generation function.

sjoshid · 2020-04-14T12:03:16Z

You are right. I cleaned up BaseState timer but not Window. Will fix this asap.

sjoshid · 2020-04-15T01:39:41Z

To remove timers from window, I check is_handled flag at the end of event() in window.
This has one drawback, all handlers need to call ctx.set_handled().

Is that acceptable?

xStrom · 2020-04-15T08:41:05Z

I think it's fine to set is_handled when the timer event reaches its destination.
Something like the following:

if *widget_id != child_ctx.base_state.id {
    recurse = child_ctx.base_state.children.may_contain(widget_id);
} else {
    child_ctx.is_handled = true;
}

That can be useful for preventing further propagation, however this is not how the removal should happen in the window, because the widget who requested the timer may not exist anymore. I think the window HashMap entry should be removed every time a Timer event comes to the window, regardless if it was handled by a widget or the widget no longer exists.

sjoshid · 2020-04-15T12:33:56Z

I think the window HashMap entry should be removed every time a Timer event comes to the window, regardless if it was handled by a widget or the widget no longer exists.

Sounds good. With that in mind, this should work at the end of event in window.

if let Event::Timer(token) = event {
    self.timers.remove(&token);
}

sjoshid · 2020-04-15T13:06:28Z

I spoke too early. With the change I suggested, the timer example crashes.
I will look in to more details.

xStrom · 2020-04-15T13:58:02Z

TimerToken is reused when available, so make sure you do the removal before you extend it with the new timer requests, because that new timer request set may contain the same id that you're removing.

…rToken � Conflicts: � druid/src/core.rs � druid/src/window.rs

sjoshid · 2020-04-19T15:24:52Z

You are absolutely right. Looks like tokens are reused only on Windows. Linux/Mac seem to work as token aren't reused there.

xStrom · 2020-04-19T17:21:30Z

Okay this looks functional now.

There is some more work opportunity here though. Instead of the window sending Event::Timer it should create a new Event::Internal(InternalEvent::Timer{ widget_id, token }) and send that, similarly to how LifeCycle::Internal(InternalLifeCycle::RouteFocusChanged{ widget_id, widget_id }) works. Then there's no need to pass the window's HashMap reference via EventCtx and inside WidgetPod we can handle the internal event. When it finally reaches the target widget we synthesize the real Event::Timer.

Let me know if this is something you wish to continue working on. It's completely fine if you want to just get this merged and be done for now. 👍

sjoshid · 2020-04-19T19:02:04Z

I prefer doing this the right way rather than just merging and creating a technical debt. Besides hopefully I'll learn something more.
I will look more in to your suggestion.
Thanks for asking.

cmyr · 2020-04-21T15:27:55Z

sorry i've been distracted, will try and take a look at this shortly!

cmyr · 2020-04-22T17:03:44Z

This approach is pretty good, but It isn't quite what i had in mind; as mentioned by @xStrom, I think it would be even cleaner if we used an internal RouteTimer event to deliver the timer to only the correct widget. The advantage of this method would be that widgets would no longer see timers that were not intended for them, and so they could be less careful about always stashing and checking timer tokens.

sjoshid · 2020-04-22T19:30:24Z

I agree. Im working on the approach you and @xStrom are talking about. But isn't

widgets would no longer see timers that were not intended for them

already working? I ask because that was THE goal I was working towards and I thought I implemented it. 🤔

cmyr · 2020-04-23T01:20:38Z

not if a widget has children; we will recurse into a container widget with the Timer event, which is not actually for the container but for the child.

…rToken � Conflicts: � druid/src/core.rs � druid/src/window.rs

xStrom

Looking better! Just some minor changes needed.

xStrom · 2020-04-27T12:38:15Z

druid/src/event.rs

@@ -147,6 +147,7 @@ pub enum InternalEvent {
    MouseLeave,
    /// A command still in the process of being dispatched.
    TargetedCommand(Target, Command),
+    RouteTimer(TimerToken, WidgetId),


Add a comment here describing this variant. Something simple like Used to route timer events. would work.

xStrom · 2020-04-27T12:49:55Z

druid/src/core.rs

-                recurse = child_ctx.base_state.request_timer;
-                Event::Timer(*id)
-            }
+            Event::Timer(token) => Event::Timer(*token),


Add recurse = false; here because if we get this then we're already a descendant of the targeted widget.

Good catch. Missed this important change. :(
Thanks.

xStrom · 2020-04-27T12:55:50Z

druid/src/window.rs

+        //In some platforms, timer tokens are reused. So it is necessary to remove token from
+        //window's timer before adding base state's timers to it.


In On some platforms, timer tokens are reused. So it is necessary to remove the token from the window's timer map before adding ~~base state's timers~~ new tokens to it.

xStrom · 2020-04-27T12:58:02Z

druid/src/window.rs

+            self.timers.remove(&token);
+        }
+
+        //If at least one widget requested timer, collect those timers from widgets and add to window's timers map.


If at least one widget requested a timer, ~~collect those timers from widgets and~~ add all the requested timers to window's timers map.

xStrom

Looks good now. Thanks for doing this!

sjoshid added 3 commits April 12, 2020 13:40

1) More aggressive Timer event traversal.

1d099bd

2) Aggressive traversal is achieved by storing a map of TimerToken -> WidgetId in each window. 3) On Timer event, we check the map from step 2 and recurse only if child MIGHT contain the WidgetId.

1) Comment

3f47c3f

Fix clippy warnings.

1bdc803

sjoshid commented Apr 12, 2020

View reviewed changes

cmyr added the S-needs-review waits for review label Apr 12, 2020

1) After merging up, Im clearing the map of BaseState. So remove is n…

dd7892e

…ot needed when event is handled.

xStrom reviewed Apr 14, 2020

View reviewed changes

xStrom added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Apr 14, 2020

sjoshid added 2 commits April 19, 2020 11:16

Merge branch 'master' of https://github.com/xi-editor/druid into Time…

12cbde9

…rToken � Conflicts: � druid/src/core.rs � druid/src/window.rs

1) Removing window's timer token before adding base state's token to it.

6349aa4

cmyr self-assigned this Apr 21, 2020

sjoshid added a commit to sjoshid/druid that referenced this pull request Apr 26, 2020

Changes as per linebender#831 (comment)

f27be6f

sjoshid added a commit to sjoshid/druid that referenced this pull request Apr 26, 2020

Changes as per linebender#831 (comment)

818d36d

sjoshid and others added 5 commits April 26, 2020 15:31

Merge branch 'master' of https://github.com/xi-editor/druid into Time…

e6b8496

…rToken � Conflicts: � druid/src/core.rs � druid/src/window.rs

Changes as per linebender#831 (comment)

3d4870a

Changes as per linebender#831 (comment)

2e6b493

Removing unused import.

9a91913

1) Fmt changes

0b77763

xStrom reviewed Apr 27, 2020

View reviewed changes

1) Code review changes.

38210fa

xStrom approved these changes Apr 28, 2020

View reviewed changes

xStrom merged commit 6d1ff52 into linebender:master Apr 28, 2020

sjoshid deleted the TimerToken branch April 28, 2020 17:07

xStrom removed the S-waiting-on-author waits for changes from the submitter label May 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Associate timers with widget ids #831

Associate timers with widget ids #831

sjoshid commented Apr 12, 2020

xStrom commented Apr 12, 2020

sjoshid Apr 12, 2020

sjoshid Apr 13, 2020

sjoshid commented Apr 12, 2020 •

edited

Loading

xStrom commented Apr 12, 2020

sjoshid commented Apr 12, 2020

xStrom left a comment

sjoshid commented Apr 14, 2020

sjoshid commented Apr 15, 2020

xStrom commented Apr 15, 2020 •

edited

Loading

sjoshid commented Apr 15, 2020

sjoshid commented Apr 15, 2020

xStrom commented Apr 15, 2020

sjoshid commented Apr 19, 2020

xStrom commented Apr 19, 2020

sjoshid commented Apr 19, 2020

cmyr commented Apr 21, 2020

cmyr commented Apr 22, 2020

sjoshid commented Apr 22, 2020

cmyr commented Apr 23, 2020

xStrom left a comment

xStrom Apr 27, 2020

xStrom Apr 27, 2020

sjoshid Apr 27, 2020

xStrom Apr 27, 2020

xStrom Apr 27, 2020

xStrom left a comment

		//In some platforms, timer tokens are reused. So it is necessary to remove token from
		//window's timer before adding base state's timers to it.

Associate timers with widget ids #831

Associate timers with widget ids #831

Conversation

sjoshid commented Apr 12, 2020

xStrom commented Apr 12, 2020

sjoshid Apr 12, 2020

Choose a reason for hiding this comment

sjoshid Apr 13, 2020

Choose a reason for hiding this comment

sjoshid commented Apr 12, 2020 • edited Loading

xStrom commented Apr 12, 2020

sjoshid commented Apr 12, 2020

xStrom left a comment

Choose a reason for hiding this comment

sjoshid commented Apr 14, 2020

sjoshid commented Apr 15, 2020

xStrom commented Apr 15, 2020 • edited Loading

sjoshid commented Apr 15, 2020

sjoshid commented Apr 15, 2020

xStrom commented Apr 15, 2020

sjoshid commented Apr 19, 2020

xStrom commented Apr 19, 2020

sjoshid commented Apr 19, 2020

cmyr commented Apr 21, 2020

cmyr commented Apr 22, 2020

sjoshid commented Apr 22, 2020

cmyr commented Apr 23, 2020

xStrom left a comment

Choose a reason for hiding this comment

xStrom Apr 27, 2020

Choose a reason for hiding this comment

xStrom Apr 27, 2020

Choose a reason for hiding this comment

sjoshid Apr 27, 2020

Choose a reason for hiding this comment

xStrom Apr 27, 2020

Choose a reason for hiding this comment

xStrom Apr 27, 2020

Choose a reason for hiding this comment

xStrom left a comment

Choose a reason for hiding this comment

sjoshid commented Apr 12, 2020 •

edited

Loading

xStrom commented Apr 15, 2020 •

edited

Loading