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

Tui Lazy Attributes and Layout #329

Merged
merged 37 commits into from May 3, 2022
Merged

Conversation

ealmloff
Copy link
Member

@ealmloff ealmloff commented Mar 23, 2022

Make the Tui renderer lazily update it's attributes, and layouts. To do this it adds a render tree which could also be used in a focus system.

other changes:

  1. Events reworked to jump to nodes that are listening to an event instead of traversing the nodes
  2. Mouse events work on absolutely positioned elements
  3. Adds a native-core crate with generic render tree and Stretch layout utilities
  4. Adds a TuiContext that allows the user to gracefully exit (needed for benchmarks)
  5. Change alpha channel from f32 to u8 for performance

todo:

benchmarks:
updating one element at a time on a square grid (excuding rendering time)

box count new old
9 458.84 us 1.4662 ms
36 2.0414 ms 19.075 ms
81 5.9310 ms 95.541 ms
144 14.224 ms 288.39 ms
225 29.410 ms 705.10 ms
324 54.425 ms 1.6291 s
441 95.033 ms 3.0568 s
576 153.13 ms 5.3373 s

chart

chart (1)

@ealmloff
Copy link
Member Author

(rebase with master)

@ealmloff ealmloff changed the title Tui Lazy Attributes Tui Lazy Attributes and Layout Mar 31, 2022
@ealmloff ealmloff marked this pull request as ready for review April 5, 2022 12:17
@ealmloff
Copy link
Member Author

ealmloff commented Apr 8, 2022

While working on the native renderer there are some improvements I think I can make to the api. I can't re-draft this, but don't merge yet.

  • Multiple of each type of state that can update independently.
  • User-defined state structure that contains all children.
  • State that depends on attributes rather that VNode's (to not expose children and parents).
  • State that depends on state of another type (text size scaling with the width/height).
  • Documentation on native-core

@jkelleyrtp jkelleyrtp marked this pull request as draft April 9, 2022 22:18
packages/tui/src/layout.rs Outdated Show resolved Hide resolved
@@ -1 +1,443 @@
# Custom Renderer
Copy link
Member Author

Choose a reason for hiding this comment

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

let queued_events = Rc::new(RefCell::new(Vec::new()));
let queued_events2 = Rc::<RefCell<std::vec::Vec<_>>>::downgrade(&queued_events);

cx.push_future(async move {
Copy link
Member Author

Choose a reason for hiding this comment

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

This only ran when work with deadline was called which was causing a race condition.

@ealmloff
Copy link
Member Author

I would love to have nodes that depend on mutable dependencies, but without GATs I don't think it is possible (same as the borrowed iterator problem).

@ealmloff ealmloff marked this pull request as ready for review April 17, 2022 13:32
@jkelleyrtp
Copy link
Member

I wonder if there's a way we can merge this piece-by-piece, or maybe try pair-reviewing it to make the process go a bit faster? I'm going through the changes locally and am really impressed :)

@ealmloff
Copy link
Member Author

I can pull out the tui bugfixes and quality of life improvements. I would be happy to pair on it!

@ealmloff
Copy link
Member Author

I didn't realize there were any attributes missing; I think that is last one without leaking children and parents.

@ealmloff
Copy link
Member Author

Nothing changed in the force-push. I just committed to the wrong branch and reverted it.

@jkelleyrtp
Copy link
Member

Is this good to review? If so I'll see if I can get it in today.

@ealmloff
Copy link
Member Author

Yes

@ealmloff
Copy link
Member Author

ealmloff commented May 2, 2022

Sorry to change things while you are reviewing, but I found a bug. Nodes without any attributes effecting layout don't get the layout initialized causing a panic. This should fix that.

Cargo.toml Show resolved Hide resolved

tui = "0.17.0"
crossterm = "0.23.0"
anyhow = "1.0.42"
tokio = { version = "1.15.0", features = ["full"] }
futures = "0.3.19"
stretch2 = "0.4.1"

stretch2 = { git = "https://github.com/DioxusLabs/stretch" }
Copy link
Member

Choose a reason for hiding this comment

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

Sounds like we need a new release of stretch so the tui crate can be published.

packages/tui/src/style_attributes.rs Outdated Show resolved Hide resolved
@jkelleyrtp
Copy link
Member

jkelleyrtp commented May 2, 2022

There's a lot going into this PR but not much of it touches the broader workspace.

I need some more time to dig into native-core - seems like some of the widget system seems pretty verbose/complex. Though, I imagine the frequency in which people should be writing widgets to be pretty low, so there's no issue with making it somewhat obtuse.

I added a few comments, but for the most part, native-core-macro and native-core get a rubber stamp of approval. I'm not sure what your timeline is on wanting to publish/release these crates.

We should also probably think about granting you edit rights to this repo since you seem to understand the codebase pretty well :)

@jkelleyrtp jkelleyrtp merged commit f7e67cb into DioxusLabs:master May 3, 2022
@jkelleyrtp
Copy link
Member

Thanks!!

There will be some cleanup before releasing but this is great work!

@ealmloff ealmloff deleted the lazy_tui branch May 3, 2022 01:39
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.

None yet

3 participants