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

Promote disabled to Widget level #1785

Merged
merged 64 commits into from
Feb 21, 2023
Merged

Conversation

davep
Copy link
Contributor

@davep davep commented Feb 14, 2023

Currently the only interactive widget in Textual that supports disabled is the Button. Really this should be something that can be set on any interactive widget (Input, Tree, DataTable, etc...). Moreover, ideally it should be something that can be set on any widget and it and all children will be disabled.

This PR seeks to implement this. Key changes include:

  • The disabled property and related code is removed from Button.
  • A new disabled property is added to Widget.
  • A new :enabled pseudoclass is added to CSS.
  • A new :disabled pseudoclass is added to CSS.
  • A new _self_or_ancestors_disabled internal property is added to Widget.
  • A new focusable property is added to Widget -- to be focusable a widget needs to have can_focus set to True and _self_or_ancestors_disabled needs to be False.
  • A new watch_disabled watcher is added to Widget, which updates the styles of the current widget and all of its children.
  • The focus_chain is now built from widgets that are focusable rather than those where can_focus is True.
  • set_focus has been changed to only allow focus to be set if focusable is True (before it was if can_focus is True).
  • The forwarding of events now takes _self_or_ancestors_disabled state into account when dealing with mouse events (in other words, if a widget is in any way disabled, it won't receive mouse events).
  • A default *:disabled style is added to App.DEFAULT_CSS which applies disabled styling to all disabled widgets.
  • A disabled keyword argument has been added to all the main "interactive" widgets.

If you need a standalone test application for this see this one here in my sandbox.

This seems to be a hangover from the early days of the development of
`Input`, and the styles do nothing as there's nothing else in the `Input`
code that makes use of the class that's involved.

Removed in anticipation of Textualize#1748 taking care of this.
f495870 got a little too carried away.
This doesn't go close to what Textualize#1748 is intending to do, but moves `disabled`
to where I want it and keeps `Button` working as before.
Note that this doesn't touch the application of stylesheets yet, in terms of
things like specificity; this just makes sure that the classes exist and can
be seen.
There was no need to default to None and then convert to a bool, defaulting
to False is just fine.
It's done as an internal, but can be called from child classes of course.
This is intended to be a single central method of asking the app to update
styles while also not caring if there is no active app available just yet.
I was going too early with setting this; it needs to happen after pretty
much everything else is set up *and* after the super's __init__ has been
called.
There's still a bit to do here, but this migrates the main work up to the
`Widget`. At this point `Button` is pretty much built expressed as a
function of what `Widget` provides in terms of things being disabled.

Focus can still move into disabled controls (or in this case right now, into
a disabled `Button`). The next step is to add something that works alongside
`can_focus` to say if a control is currently capable of receiving focus (in
other words, it's `not disabled and can_focus`).
Don't use comments as version control! Or, really, I don't need this note to
self any more about the code as it's being handled elsewhere.
Some hangover from the work to migrate `disabled` out of `Button` and into
`Widget`, that I forgot to remove.
We want to maintain `can_focus` as "this is a thing that can, at some point,
receive focus, assuming it isn't disabled". So `can_focus` is now very much
about the ability to receive focus at all.

The new `focusable` property becomes about "can this widget receive focus
right now?".

[This is a rewording of a previous commit -- I get the wrong thing]
Some hangover from the work to migrate `disabled` out of `Button` and into
`Widget`, that I forgot to remove.
Rather than it being about widgets where `can_focus` is `True`, have it be
about widgets that are currently able to receive focus. Before this change
widgets with a positive `can_focus` but which were disabled would still end
up in the chain.
@davep davep added the enhancement New feature or request label Feb 14, 2023
@davep davep self-assigned this Feb 14, 2023
@davep davep linked an issue Feb 14, 2023 that may be closed by this pull request
Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, some minor requests

src/textual/widget.py Outdated Show resolved Hide resolved
src/textual/widget.py Outdated Show resolved Hide resolved
I typo.

Co-authored-by: Rodrigo Girão Serrão <[email protected]>
@davep davep changed the title [WiP] Promote disabled to Widget level Promote disabled to Widget level Feb 16, 2023
@davep davep marked this pull request as ready for review February 16, 2023 10:33
Copy link
Contributor

@rodrigogiraoserrao rodrigogiraoserrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a bunch of questions but I found no blockers.

src/textual/widget.py Show resolved Hide resolved
src/textual/widgets/_data_table.py Show resolved Hide resolved
src/textual/widgets/_static.py Show resolved Hide resolved
src/textual/widgets/_text_log.py Show resolved Hide resolved
src/textual/widgets/_tree.py Show resolved Hide resolved
tests/test_disabled.py Outdated Show resolved Hide resolved
@davep
Copy link
Contributor Author

davep commented Feb 16, 2023

Re: the docstrings that are missing in total, I've added #1811.

@willmcgugan willmcgugan merged commit 4530700 into Textualize:main Feb 21, 2023
@davep davep deleted the promote-disabled branch February 21, 2023 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a base disabled property to the Textual widget hierarchy
3 participants