Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'disabled' state to widgets. #746

Closed
cmyr opened this issue Mar 23, 2020 · 7 comments · Fixed by #1717
Closed

Add 'disabled' state to widgets. #746

cmyr opened this issue Mar 23, 2020 · 7 comments · Fixed by #1717
Labels
architecture changes the architecture, usually breaking

Comments

@cmyr
Copy link
Member

cmyr commented Mar 23, 2020

This is a way to communicate to control widgets that they should not respond to user input, and that they should paint themselves as 'disabled'. The 'not responding' bit might even be handled in the WidgetPod; it could just not send certain events on to the widget when this is set?

We may also want lifecycle events for DisabledChanged? That's sort of funky because it is missing concerns a little bit. This probably needs a bit more thought.

@cmyr cmyr added the architecture changes the architecture, usually breaking label Mar 23, 2020
@futurepaul
Copy link
Collaborator

Hey just bumping this because I'm dealing with it for a Button right now in my own app. There seem like so many different ways to deal with this I don't really know the right approach, but here's what I'm leaning toward for my own project:

Setting a canonical "DISABLED" key in the Env makes it easy to visually respond to the disabled state. It's also easy to do wrap a whole section of the tree in a env_scope and disabled it all at once.

The on_click handler in Button can also check the Env and just do nothing when "DISABLED" is true.

More complicated interactions maybe need more bespoke handling of what DISABLED means to them.

Could set this key directly with

.env_scope(|env, data| {
            env.set(DISABLED, data.should_be_disabled);
        });

Or have some sort of convenience method in WidgetExt:

.set_disabled(|data| data.should_be_disabled);

This is all about ignoring events at the leaf, does nothing to address not sending them down, so I don't know if this is an ideal solution but it seems reasonable to me.

@derekdreery
Copy link
Collaborator

Flutter uses "null" to indicate disabled for things like text boxes.

@cmyr
Copy link
Member Author

cmyr commented Sep 18, 2020

The big question to me is whether this should be something that is set in the Env or if it is something that is part of WidgetState. Env definitely has some nice properties, but it weirds up the API a bit; things like is_focused and is_active are all exposed via the ctxs, and it feels weird if is_disabled is something you get out of the Env?

I do think there might be some problems with putting it in WidgetState, though. In particular, we'd want to be cascading this down the tree; if a parent is disabled, then so are the children; and if the children are disabled and the parent is enabled, we want to still be disabled. This is doable, but it might be some slightly finicky book-keeping?

In any case I do think this should be added, and should be used in built-in widgets.

@xarvic
Copy link
Collaborator

xarvic commented Feb 1, 2021

I'm not sure if it would be enough to set a flag in Env, since the widget could still get focused, but since it does not receive events, it can not resign its focus. Therefore switching focus via tab would be broken. This happend to me: https://github.com/linebender/druid-widget-nursery/blob/master/examples/multi_value.rs.

I do think there might be some problems with putting it in WidgetState, though. In particular, we'd want to be cascading this down the tree; if a parent is disabled, then so are the children; and if the children are disabled and the parent is enabled, we want to still be disabled. This is doable, but it might be some slightly finicky book-keeping?

Maybe the WidgetPod could contain 2 flags: widget_manually_disabled and parent_widget_disabled. The is_disabled method on ctx would return widget_manually_disabled | parent_widget_disabled.

@cmyr
Copy link
Member Author

cmyr commented Feb 3, 2021

I agree that the env is not enough for this; 'disabled' is a special state that needs to be known to the framework.

@xarvic
Copy link
Collaborator

xarvic commented Feb 4, 2021

I would like to try to implement this, if no one is already doing it.

@cmyr
Copy link
Member Author

cmyr commented Feb 8, 2021

@xarvic you're welcome to play around, but there are some deep questions involved, like exactly how disabled widgets should participate in event delivery; do we stop delivering all events, and then begin again later? In this case we may want to add 'Disabled/Enabled' lifecycle events, and I haven't thought through all of the possible consequences of this..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture changes the architecture, usually breaking
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants