Disable widgets #1632

xarvic · 2021-03-03T21:54:36Z

An implementation of a disabled state for widgets (issue: #746 ).
It adds the following:

set_enabled(bool) for EventCtx and UpdateCtx which changes if the state of the current widget is "manually disabled".
is_enabled() -> bool for all Context which returns false if this widget or any of its ancestors is "manually disabled".
LifeCycle::EnabledChanged(bool) which is called every time when is_enabled() would return a different value than before.
~~the EnabledIf widget, which enables the enclosed widget if the provided closure returns true~~

~~It contains a breaking change: LifeCycle::WidgetAdded was changed to LifeCycle::WidgetAdded{ initially_enabled: bool }. This allows widgets to be disabled before the first call to update()~~.

~~I also added boxed versions of the methods that add children to a flex.~~

cmyr · 2021-03-06T01:56:49Z

Will do a big batch of reviews this weekend or early next week.

One initial piece of feedback: I'm not sure about the change to WidgetAdded. If a widget wants to be initially disabled it makes sense (to me) for it to disable itself while handling WidgetAdded; since this happens before the the widget has layout or paint called, I think it should be fine? If there's a particular problem you ran into that motivated this change I'm happy to hear about it. :)

xarvic · 2021-03-06T12:01:09Z

One initial piece of feedback: I'm not sure about the change to WidgetAdded.

You are right, that was not necessary

cmyr

Thanks for taking this on! This is a very ambitious PR, and you're getting deep into the druid internals. Overall I think that the code is good, and I really appreciate the documentation.

I have two broad concerns:

I think this PR is doing a bit too much, and I would really like to try and split out the architectural changes from both the display logic (widgets handling this new state) as well as from any addtional API that isn't directly related to the functionality at hand; this will let me review revisions of this PR much more quickly, which will hopefully let us get it landed more quickly. You can stash those other changes and we can review them separately, which should also be much simpler, since the logic for the widget stuff should (🤞) be more straightforward.
I believe there are some quirks of the druid architecture that make specific aspects of this implementation slightly problematic. I think the big picture is good, but there are some tricky details that need to be sorted out; in particular I'm pretty sure we want to be dispatching events from the root of the tree, instead of injecting them in during the handling of the initial events.

In any case, I would encourage you to keep working on this; it's clear you've thought about it quite a bit and have a good grasp of the problem. If you have any questions please feel free to reach out here or on zulip, I'm happy to help however I can.

druid/src/widget/flex.rs

druid/src/widget/radio.rs

druid/src/contexts.rs

cmyr · 2021-03-12T16:33:14Z

druid/src/core.rs

@@ -104,6 +104,12 @@ pub(crate) struct WidgetState {
    pub(crate) viewport_offset: Vec2,

    // TODO: consider using bitflags for the booleans.
+    pub(crate) set_enabled: bool,


I would probably go with is_enabled, for symmetry; if we want to be more explicit maybe is_explicitly_disabled (I think having "disabled" in the names might be clearer, as it's the exceptional state? Like we don't store is_unfocused and is_not_active? But I don't have especially strong feelings about any of this)

I think everything connected to enabled/disabled should either use "enabled" or "disabled" consistently. My initial thought was that LifeCycle::DisabledChanged(false) would sound strange, but your probably right, the user will search for disabled, since all widgets are enabled by default.

I agree on both points, and also thought DisabledChanged sounds a bit weird but I don't mind it. We could also consider being verbose and adding WasEnabled and WasDisabled as separate events, although this would be a departure from current practice, although it might also be easier to reason about?

After thinking about this again DisabledChanged() is probably still the best option.

druid/src/core.rs

cmyr · 2021-03-12T16:59:37Z

druid/src/core.rs

@@ -579,6 +585,43 @@ impl<T: Data, W: Widget<T>> WidgetPod<T, W> {
        }
    }

+    fn update_enabled(


Enabled/Disabled is definitely interesting; one of the things I hadn't really thought about before is that it is the first piece of WidgetState (so far) that cascades down, and not up.

In general I would prefer for control flow to follow existing patterns: in particular I would like, if possible, to avoid dispatching an event in the middle of another event, and would prefer instead to complete the initial traversal, accumulating state in the widgets describing the changes, and then do a subsequent traversal that handles the update.

The reason for this is that it makes it easier to maintain the invariants around how widget state is updated; when we send a new event in the middle of another event, it's possible that some state was set during the previous event that has not been handled yet.

I think I have a reasonable example of how this could break:

widget A has a child, widget B

widget 'A' receives an event, let's say because the esc is pressed.

it passes the event to widget 'B'.

widget 'B' adds a new child, and sets children_changed

widget 'A' then decides to disable itself

widget 'A''s pod sees this, and sends EnabledChanged

widget 'B' now gets EnabledChanged, but it hasn't yet received WidgetAdded

widget 'B' crashes, because it configures some resource in 'WidgetAdded, which it understands should always be the first event a widget sees.

This might be a slightly contrived example, but I believe with this patch it is possible, and assuming we wish to maintain the invariant that WidgetAdded will always be the first thing a widget sees, this current implementation doesn't quite work.

What I think we need to do is to figure out a way to accumulate state during the first traversal in such a way that we can check when we finish the original event (as in the post_event_processing fn) and dispatch some sort of RouteEnabledChanged event that will do an additional traversal and actually deliver the EnabledChanged event to the correct widgets.

Thanks for the review :)

Hmm, i haven't thought about that at all.
I see the point. It could get messy if we just send the event during another one.

I can think of two options to solve this:

First: we add a fourth bool, something like child-enabled-changed and add a InternalLifeCycle::RouteEnabledChanged() event which gets send down the tree if child-enabled-changed is set or enabled was changed for the current widget. Then we could either update the focus chain directly or use children_changed.

Second: we generalize InternalLifeCycle::RouteWidgetAdded to InternalLifeCycle::RouteWidgetChanged which would perform both tasks at once. This should work, since a widget can't receive both (WidgetAdded and EnabledChanged) at the same time. I like this version better since we save another InternalLifeCycle event, won't pollute the widgetstate with further dirty flags and also it feels like both events behave quite similar, since they are the only events which change the focus-chain. But i think for this to work, widgets should not be allowed to call enabled_changed from Lifecycle::WidgetAdded.

What are your thoughts about this?

The second option hadn't occurred to me, but I think it's possible it could work? I'd want to really think it through. One interesting thing about this approach is that we could potentially re-use the children_changed flag to indicate where we need to recurse in order to deliver the FocusChanged event.

Thinking this through more, though, I have my doubts. Basically: I think that this is exchanging superficial complexity ("one less event type! one less flag in WidgetState) for potentially deep combinational complexity, e.g. "now these two distinct actions interact in ways that are hard to reason about". For instance: imagine if, in response to the same event, one widget changes a child, and another widget calls set_disabled. Now we want to send WidgetAdded, but in the course of doing this we reset children_changed, and how we can't correctly route the FocusChanged event? I'm not sure if this is an actual concern; the point is more that I'm already getting a bit confused thinking about it.

So I think I prefer the first option. It's more verbose, but it is also simpler to reason about. Adding an additional Internal event really doesn't cost us anything meaningful.

A separate question is, in this world, what state we need to store in WidgetState. My current understanding is that we will need, at least (and without committing to these names, I'm just using what comes to me right now):

is_self_disabled: whether this specific widget has been disabled

is_disabled: whether this widget is part of a disabled subtree (including the case where it is a subtree of one; is_self_disabled implies is_disabled).

child_disabled_changed a flag used when routing the actual event; when this is false we can stop recursing.

disabled_changed: a flag which indicates that the current widget has been disabled. Actually: I think maybe this isn't a bool, but is more like an Option<bool>, to indicate the new state?

Oh no, this gets worse.

What happens when a widget and one of its descendants both change their disabled state in response to the same event, but they do it in different directions? For instance: what if a subtree was disabled, and the parent re-enables it, and at the same time one of the children explicitly disables itself?

Maybe this isn't that bad. With the actual EnabledChanged event, we recurse down, stopping at any child that has either is_self_disabled set, or that has disabled_changed as Some(true), or whatever thing means "I'm becoming disabled"? In this latter case we would need to continue to descend only if the change was to also become disabled; otherwise we just set is_self_disabled, for that particular widget, but continue to notify children that they are now disabled.

So..... I think this is okay (in that it is not fundamentally broken) but it is definitely more complicated than I had initially thought, and I suspect there are edge cases I'm not thinking of yet. I would encourage you to try and think through these various permutations of possible states and orders, and I definitely think we are going to want some tests in place, if just to check that our understanding is correct.

I think the way you described should work, but i will definitely write some testcases this time :)

But i'm unsure about one thing:

is_disabled: whether this widget is part of a disabled subtree (including the case where it is a subtree of one; is_self_disabled implies is_disabled)

My current Implementation has two fields: one for the own state (manually disabled) and one for the tree (ancestors, not including my self, disabled). I think we need both, since otherwise we lose the information if calling set_enabled() will change the "enabled state" of this widget.

Also new_disabled feels more robust than enabled_changed. The question is, is it possible to receive an event between calling set_enabled() and receiving LifeCycle::EnabledChanged() and if so should this event use the old or the new state?

But i'm unsure about one thing:

is_disabled: whether this widget is part of a disabled subtree (including the case where it is a subtree of one; is_self_disabled implies is_disabled)

My current Implementation has two fields: one for the own state (manually disabled) and one for the tree (ancestors, not including my self, disabled). I think we need both, since otherwise we lose the information if calling set_enabled() will change the "enabled state" of this widget.

I agree that we need two flags, one for the specific widget and one that includes parent state, and overrides the state of the specific widget; if a parent is disabled but we're not, we still are. I think we're on the same page about this.

Also new_disabled feels more robust than enabled_changed.

If you can make this work with new_disabled that's great. I guess maybe how I was thinking about this is as a two-part process; in the first part (when the widget calls set_disabled() we store information on the pending change, and then in the second part (when we traverse the tree to reconcile the changes and send the events) we clear the pending state and set the actual state. So in my thinking a widget isn't disabled when it calls set_disabled; it is disabled when the tree is traversed afterwards.

The question is, is it possible to receive an event between calling set_enabled() and receiving LifeCycle::EnabledChanged() and if so should this event use the old or the new state?

The only event that it should be possible to receive here is RouteWidgetAdded; we need to always handle that before we do anything else. I think it should be fine to keep the new state in place while handling this event; immediately afterwards we will handle enabled/disabled, and then after that (if necessary) we will handle focus change.

druid/src/window.rs

…ledChanged - implemented the disabled state in WidgetPod - changed call to focus_change from event to post event processing - implemented disabled handling in window.rs and core.rs

- the focus chain was cleared, if the widget was disabled

(i hope)

cmyr

Cool, this looks much closer to what I would expect.

I have a few questions in line, and a few little suggestions.

One thing I would like to see included in this PR is a clearly written explanation of how enabled tracking works; what the different bits of state are that represent it, what their valid values are at different times, and how changes to focus are expected to cascade through the system.

I'm very encouraged by the tests; this is definitely introducing a bunch of new complicated logic, and I wouldn't be surprised if we see some tricky bugs crop up over time.

druid/src/contexts.rs

druid/src/core.rs

cmyr · 2021-03-30T14:55:14Z

druid/src/core.rs

+                self.state.focus_chain.insert(0, self.state.id);
+            }
+        }
+


I'm slightly scared of all the little bits of code scattered throughout this method, and I think it is going to be very important to carefully document what our invariants are, and what should go where.

For instance: why does this need to come after merge_up, and why does the preceding block need to come before?

I fixed some of them and added documentation to explain the rest.

Hmmm :/ It wasn't as easy as i thought, but i think some of this complexity is necessary.

The ID of this widget should be the first in our focus-chain

we should add our ID only if auto_focus is set

auto_focus is not set until after WidgetAdded

we only need to add the value if children_changed or children_disabled_changed is set, otherwise we duplicate the values.

some_one could call ctx.children_changed() while routing WidgetAdded, which makes no sense, but therefore after the call we don't know if the value was set

The current implementation always adds the own id when the focus_chain is cleared and focus_chain is set. Only in WidgetAdded the id is always added after the event.
I don't know if there is a better way to satisfy the the conditions i mentioned?

druid/src/core.rs

Update Documentation Co-authored-by: Colin Rofls <[email protected]>

Update documentation Co-authored-by: Colin Rofls <[email protected]>

cmyr

A couple additional notes, I'm wondering if there are ways we can simplify the control flow here? Where possible, I really want to design mechanisms that don't rely on a bunch of special cases. In particular I'm wondering if we might be able to have fewer branches and conditionals, and just always reset the focus chain before dispatching these events, and then always fixing the focus chain up, afterwards.

druid/src/core.rs

cmyr · 2021-03-31T14:11:09Z

druid/src/core.rs

+                        true
+                    } else {
+                        false
+                    }


okay trying to wrap my head around this.

Can we simplify this?

always clear the focus chain

always fixup the focus chain afterwards?

I'm also curious about the logic here. I would expect it to be true that in order to see this event, either we have been explicitly disabled, or some of our children are, and the final branch here should be unreachable?

Basically: looking back through this method, I am struck by how simple it is to handle the focus_chain stuff currently, and I feel like it should be possible to have the new logic be similarly simple? This all comes down to understanding our invariants; what guarantees can we make about our state before we recurse, and what guarantees can we expect afterwards?

If we always clear the focus_chain we need to route this event to all widgets in the window. I could change it if you think that's better.
In the current implementation i always clear the focus-chain if i recurse the event or send the extra event.

Okay, so I think the current logic is actually a closer match to what I was describing; RouteWidgetAdded only clears the focus chain when it is going to recurse. 🤔

Okay, so I think the current logic is actually a closer match to what I was describing; RouteWidgetAdded only clears the focus chain when it is going to recurse. thinking`

What do you mean with 'current logic'? My implementation or the current state of druid without this PR?

cmyr · 2021-03-31T14:27:07Z

druid/src/core.rs

+                } else if self.state.children_disabled_changed {
+                    self.state.reset_focus_chain();
+                    extra_event =
+                        Some(LifeCycle::Internal(InternalLifeCycle::RouteDisabledChanged));


okay I think I also missed this in my first pass, and I'm having trouble wrapping my head around it.

When do we want to switch from DisabledChanged back to RouteDisabledChanged?

If we are disabled and our Parent changed its state from Enabled to Disabled, this doesn't changes our state, but if one of our children changed its disabled state we still need to route RouteDisabledChanged.

I probably should write that in the code

Do we actually need to route in this case? The child was disabled previously and remains disabled. Or is this about ensuring that the internal state is correctly reset?

Oh, i forgot about that. It should be possible to leave the state dirty since it is already disabled. Even if is_explicitly_disabled is false and is_explicitly_disabled_new is true, this should get handled correctly and the widget would stay disabled when it receives DisabledChanged(false). But it feels like moving the complexity to another place, because now it is possible to receive events while the state is dirty. And i can't tell how this would affect other things.

cmyr

Okay, this looks like it's very close. My one remaining thought is whether we can remove some of the logic around deciding when to update the focus chain, and just always do that after any WidgetAdded or DisabledChanged event?

cmyr · 2021-04-05T15:08:07Z

druid/src/core.rs

            text_registrations: Vec::new(),
+            update_focus_chain: true,


do we need to track this? Can we just always update the focus change after any ChildrenAdded or DisabledChanged event?

cmyr · 2021-04-05T15:11:59Z

druid/src/core.rs

+                    if was_disabled != self.state.is_disabled() {
+                        // In case we change but none of our children we still need to update the
+                        // focus-chain
+                        self.state.update_focus_chain = true;


I would be happy to have less update logic here and just always rebuild the focus chain after this event; it's harder for us to screw that up later when we make some other little change.

cmyr · 2021-04-05T15:13:16Z

druid/src/core.rs

+                if was_disabled != self.state.is_disabled() {
+                    self.state.update_focus_chain = true;
+                    true
+                } else {
+                    false
+                }


yea if we didn't need to update this manually we could just write,

was_disabled != self.state.is_disabled()

druid/src/event.rs

Co-authored-by: Colin Rofls <[email protected]>

SecondFlight · 2021-04-05T16:05:59Z

While you're updating this PR, could you add this feature to the changelog?

xarvic · 2021-04-05T16:09:21Z

I will add it to the changelog with the next batch of fixes. I hope that doesn't take too long

cmyr

Okay I still would not be surprised if we run into some small issues with this going forward, but I think it's worth getting it merged now and then we will see what shakes out. Thank you @xarvic for taking this on, it was a complicated feature and I'm happy with how it's looking!

cmyr added the S-needs-review waits for review label Mar 5, 2021

cmyr requested changes Mar 12, 2021

View reviewed changes

cmyr added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Mar 16, 2021

xarvic added 13 commits March 20, 2021 13:51

- added LifeCycle::DisabledChanged and InternalLifeCycle::RouteDisab…

fe93a1b

…ledChanged - implemented the disabled state in WidgetPod - changed call to focus_change from event to post event processing - implemented disabled handling in window.rs and core.rs

created tests for disable

3e1874b

fixed tests

d6e34b2

fixed tests

500954e

updated core.rs and event.rs

f606199

fixed focus-chain bug:

6fd0b71

- the focus chain was cleared, if the widget was disabled

fix disabled update

25d19c9

update tests

e3d8b92

fixed code (all tests succeed)

77d4f8f

refactored core.rs and tests/mod.rs

4d48edd

updated tests

9b2c8ba

fixed focus-chain bug

d5116de

make clippy happy

6c79e1e

(i hope)

xarvic force-pushed the disable-widgets-2 branch from 3a87a9f to 6c79e1e Compare March 23, 2021 12:54

xarvic requested a review from cmyr March 23, 2021 12:57

xarvic and others added 2 commits March 23, 2021 13:03

Merge branch 'master' into disable-widgets-2

1fe6971

make clippy happy #2

40be21f

cmyr reviewed Mar 30, 2021

View reviewed changes

xarvic and others added 6 commits March 31, 2021 10:34

Apply suggestions from code review

5520e5b

Update Documentation Co-authored-by: Colin Rofls <[email protected]>

Update druid/src/contexts.rs

232e7ee

Update documentation Co-authored-by: Colin Rofls <[email protected]>

refactor DisabledChanged

eccd596

refactor DisabledChanged

9754051

fixed error, revered change of focus_chain

74b86b2

refactored tests

c181f51

xarvic requested a review from cmyr March 31, 2021 14:06

cmyr reviewed Mar 31, 2021

View reviewed changes

reordered lifecycle events

cfc544e

cmyr reviewed Mar 31, 2021

View reviewed changes

reverted changes to the focus_chain

05d45fc

HoNile mentioned this pull request Apr 4, 2021

Hide/Disable widgets or buttons from state #1694

Closed

xarvic added 5 commits April 5, 2021 16:02

implemented new focus-chain using LifeCycle::BuildFocusChain

b9b9aca

update tests

f091bf9

fixed problems

4766ceb

updated texts

a901d1c

clippy fix

eb3cfd7

cmyr reviewed Apr 5, 2021

View reviewed changes

xarvic and others added 5 commits April 5, 2021 17:32

fixed documentation

fdd510a

Update druid/src/event.rs

4ef9697

Co-authored-by: Colin Rofls <[email protected]>

fixed documentation

4745fe9

made logic simpler

4567f8f

refactored post_event_processing

ba58aad

xarvic and others added 4 commits April 5, 2021 18:28

updated CHANGELOG.md

8d6b784

fixed docs

d3e73d1

Merge branch 'master' into disable-widgets-2

7be336c

make clippy happy

a52437c

xarvic requested a review from cmyr April 5, 2021 17:18

Merge branch 'master' into disable-widgets-2

2919953

cmyr approved these changes Apr 6, 2021

View reviewed changes

cmyr merged commit 6de93b2 into linebender:master Apr 7, 2021

This was referenced Apr 12, 2021

Clarify the relation of focus and propagate_to_hidden #1716

Closed

Convenience traits for code generic over contexts. #1611

Closed

xStrom removed the S-waiting-on-author waits for changes from the submitter label Apr 19, 2022

Disable widgets #1632

Disable widgets #1632

Conversation

xarvic commented Mar 3, 2021 • edited Loading

cmyr commented Mar 6, 2021

xarvic commented Mar 6, 2021 • edited Loading

cmyr left a comment

Choose a reason for hiding this comment

cmyr Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xarvic Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

cmyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xarvic Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SecondFlight commented Apr 5, 2021

xarvic commented Apr 5, 2021

cmyr left a comment

Choose a reason for hiding this comment

xarvic commented Mar 3, 2021 •

edited

Loading

xarvic commented Mar 6, 2021 •

edited

Loading

cmyr Mar 12, 2021 •

edited

Loading

xarvic Mar 31, 2021 •

edited

Loading

xarvic Mar 31, 2021 •

edited

Loading