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

New Tree Control #1170

Merged
merged 34 commits into from
Nov 23, 2022
Merged

New Tree Control #1170

merged 34 commits into from
Nov 23, 2022

Conversation

willmcgugan
Copy link
Collaborator

@willmcgugan willmcgugan commented Nov 13, 2022

  • Replace TreeControl with a new implementation, and renamed it to Tree.
  • Updated DirectoryTree with new implementation.
  • Updated code_browser.py example.
  • Made bindings inherit from base classes, so they are merged by default. This can be disabled by setting inherit_bindings=False in the same way as inherit_css
  • Updated docs for tree widgets. Still a little thin, but a start.
Screen.Recording.2022-11-20.at.16.52.05.mov

@willmcgugan willmcgugan marked this pull request as draft November 13, 2022 16:46
Copy link
Contributor

@UriyaHarpeness UriyaHarpeness left a comment

Choose a reason for hiding this comment

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

Saw it's a draft only after..

src/textual/css/styles.py Outdated Show resolved Hide resolved
src/textual/widget.py Show resolved Hide resolved
@willmcgugan willmcgugan changed the title WIP new tree control New Tree Control Nov 20, 2022
@willmcgugan willmcgugan marked this pull request as ready for review November 20, 2022 16:59
@@ -551,6 +551,22 @@ def _align_size(self, child: tuple[int, int], parent: tuple[int, int]) -> Offset
self._align_height(height, parent_height),
)

@property
def partial_rich_style(self) -> Style:
"""Get the style properties associated with this node only (not including parents in the DOM).
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps use a more property-style docstring here rather than a function style?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean the "int: description"?

I've found that mkdocstrings doesn't render them correctly. It just omits the type :-(

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes and no. While that'd be ideal (and it does render really nicely in Emacs and VSCode hints, from what I've seen), I'm meaning more the wording. "Get..." sort of describing it as a function that does something, rather than as a property that has a value that has a particular meaning. So in this case likely just "The style properties associated with this node only (not including parents in the DOM)".

It's unfortunate that mkdocstrings doesn't handle it given it's, as far as I can tell, a documented aspect of the Google docstring convention.

PS: Just omits or just emits?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The type in properties docstrings is just omitted. You see the description but no type.

With a function style, you do see the type.

Will assume that's a bug in mkdocstrings, and go for property style docs then.

Copy link
Contributor

Choose a reason for hiding this comment

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

I may see about raising that with mkdocstrings then; it's unfortunate it does that.

@@ -966,6 +983,18 @@ def content_region(self) -> Region:
content_region = self.region.shrink(self.styles.gutter)
return content_region

@property
def scrollable_content_region(self) -> Region:
"""Gets an absolute region containing the scrollable content (minus padding, border, and scrollbars).
Copy link
Contributor

Choose a reason for hiding this comment

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

Would likely be better with a property-style docstring.

src/textual/widgets/_tree.py Outdated Show resolved Hide resolved

Args:
label (TextType): The new node's label.
data (TreeDataType): Data associated with the new node.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
data (TreeDataType): Data associated with the new node.
data (TreeDataType | None, optional): Data associated with the new node.

return self._cursor_node

@property
def last_line(self) -> int:
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing docstring

"tree--highlight-line",
}

show_root = reactive(True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this and the other reactives here have docstrings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably should, yeah.

"""Render a label for the given node. Override this to modify how labels are rendered.

Args:
node (TreeNode[TreeDataType]): A tree node.
Copy link
Contributor

Choose a reason for hiding this comment

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

base_style and style are missing from the docstring.

Copy link
Contributor

@davep davep left a comment

Choose a reason for hiding this comment

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

Handful of "perhaps needs a docstring" and "perhaps needs to be a property-style docstring rather than function-style", that's all I could see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants