feat: TreeItem trait #33
Conversation
|
When you are reading this, feel free to provide a review. I am curious about your thoughts. I am also interested in smaller things like hard to understand stuff. Better comments / function names will also improve the code here. Another aspect is the usage of this. Personally I will attempt migrating This change involves a lot of generics / lifetimes and having just written them I kinda know what I did so other pairs of eyes might be interesting here. Mentioning some people that might have insights here. Maybe @Teufelchen1 or (can't mention |
There was a problem hiding this comment.
I like the change overall. Makes a lot of sense.
Regarding lifetimes, there's a few named 'a that could be 'content perhaps.
In the Tree type, you refer to the concept of a tree nodes implicitly, but there's no trait or struct backing the concept. This means there's two synonymous concepts (or very closely related concepts if a node isn't an item). A possible approach to avoid the rename could be to name the trait TreeNode instead of TreeItem (you might still rename that if it's not quite the right name).
Do you happen to know the actual size of the metadata tree btw?
A good idea for an example to really test out the trait is a WidgetNode (which can render any boxed WidgetRef instead of just Text etc.). Obviously height is a problem there too.
| /// Get a mutable reference to a child by index. | ||
| /// Returns the render height of this item. | ||
| /// | ||
| /// When you choose to change the `identifier` the [`TreeState`](crate::TreeState) might not work as expected afterwards. | ||
| /// This is the height later available at [`render`](Self::render). | ||
| /// It is used to check which items are visible when the full [`Tree`](crate::Tree) is being rendered. | ||
| #[must_use] | ||
| pub fn child_mut(&mut self, index: usize) -> Option<&mut Self> { | ||
| self.children.get_mut(index) | ||
| } | ||
| fn height(&self) -> usize; |
There was a problem hiding this comment.
See note earlier about height - likely more applies to this line than the prerendered tree item
| fn inner<Item>(items: &[Item], error: &'static str) -> std::io::Result<()> | ||
| where |
There was a problem hiding this comment.
Perhaps a custom error might be better than io::Result - looking at e.g. HashMap, it defines an OccupiedError for a similar use case (adding an item that already exists).
There was a problem hiding this comment.
Perhaps instead of this, I wonder if looking for an Ordered Hash Map crate to use (no direct experience with these, just an idea), then the calling code might be something like:
fn add_child(identifier: Identifier, value: Value) -> SomeError
if map.contains(identifier) { ... }
map.push(...)
}
src/json.rs
Outdated
There was a problem hiding this comment.
It might be worth thinking about whether it could be useful for editing use cases to support mapping back to the json using Value::pointer / pointer_mut somehow.
Also, more generally it might be nice to consider how to make mutable treeviews work well.
Currently: 706 kB minified, 954 kB with tabs as indentation. This might change over time, but the main idea is to have a somewhat large JSON to benchmark against. |
EdJoPaTo
force-pushed
the
tree-item-trait
branch
3 times, most recently
from
00b7b8e
to
dc25aef
Compare
EdJoPaTo
force-pushed
the
tree-item-trait
branch
3 times, most recently
from
ea48aa6
to
cc59ed4
Compare
|
As an aside (and as much as I hate it) it's really difficult to review larger PRs when you rebase them. I noticed this on the assert_eq PR too. Github doesn't have a good way to help show how they change over time (without manually url hacking, and even then, that's painful), meaning that you have to manually wade through things to work out what has changed. I'd advise generally (except in really small change situations) just adding more commits and merging to bring in changes from main / other PRs. It sucks that this is the approach that seems to work best, but that's one of github's failures to be a good git citizen. |
The compare button in the history helps, but yeah. In these two branches / PR I regularly move improvements back on the main and reuse them in both. The idea here was quick iteration on improvements. As merge commits also have only one merge conflict instead of multiple like on a rebase (per commit) that might also work well 🤔 |
|
The |
Introduce a
TreeItemtrait which can render itself to the buffer directly when needed. Due to the decoupling of height and render the items in view can be determined without needing to generate the Text for each item upfront. Only the relevant items are rendered then.This has a huge performance benefit and renders the cargo metadata JSON data in 1/5 of the time. It still takes ~70 ms on a Raspberry Pi 2 to parse the cargo metadata JSON to
JsonTreeItems and render them, but it's better than ~320 ms ✨The current
TreeItemcontinues to exist asPrerenderedTreeItembut using more efficientTreeItemimplementations (like theJsonTreeItem) will be better especially with many (hidden) items.cargo benchoutput on Raspberry Pi 2: