-
Notifications
You must be signed in to change notification settings - Fork 881
Tree updates #5023
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
Tree updates #5023
Conversation
willmcgugan
commented
Sep 19, 2024
•
edited
Loading
edited
- Updates to the Tree control
- Additional ansi colors styles for tree / screen
@darrenburns This isn't ready for a full review, but could you give your thoughts on this? It adds more keybindings to navigate the tree. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts
src/textual/widgets/_tree.py
Outdated
@@ -484,8 +519,23 @@ class Tree(Generic[TreeDataType], ScrollView, can_focus=True): | |||
ICON_NODE_EXPANDED = "▼ " | |||
|
|||
BINDINGS: ClassVar[list[BindingType]] = [ | |||
Binding("shift+left", "cursor_parent", "Cursor to parent", show=False), | |||
Binding("shift+right", "cursor_parent_next_sibling", "Cursor out", show=False), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cursor_next_ancestor
maybe?
I'm not understanding "Cursor out" here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of something like a debugger, when you step in , step out.
I like cursor_next_ancestor
for an internal name. Maybe "move the cursor to the next ancestor" for the description?
src/textual/widgets/_tree.py
Outdated
Binding("enter", "select_cursor", "Select", show=False), | ||
Binding("space", "toggle_node", "Toggle", show=False), | ||
Binding("x", "toggle_expand_all", "Expand or collapse all", show=False), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we added a single character keybind previously we quickly received feedback that it was clashing with apps and we removed it.
Is "toggle expand all" a common enough action in the real world to justify this binding? Maybe we should leave it unbound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, maybe. I actually wanted "shift+space", but I can't seem to bind that. If I can get shift+space working, I feel that would be better. Its still toggling, but a larger scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed shift+space. Turned out to be an issue with the inline driver.
I hope you don't mind me also chiming in. Perhaps this isn't a major concern, but I just wanted to mention that some terminal emulators might not support the new Shift and arrow key bindings. For example, looking back at the previous review of keys and escape sequences, |
I tested in terminal.app and it supports them. Of course that's no guarantee they are supported everywhere, but I can test some others. |
I think |