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

feat: crates-tui based async tutorial #440

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Conversation

kdheepak
Copy link
Contributor

No description provided.

Copy link

cloudflare-workers-and-pages bot commented Feb 11, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: b77a2c2
Status: ✅  Deploy successful!
Preview URL: https://4c4d41f8.ratatui.pages.dev
Branch Preview URL: https://kd-async-tutorial.ratatui.pages.dev

View logs

@joshka
Copy link
Member

joshka commented Feb 11, 2024

The crates-tui still has a lot of complexity that might be mitigated first to make it easier to explain in the context of a tutorial:
See ratatui/crates-tui#25
And ratatui/crates-tui#26

@kdheepak kdheepak force-pushed the kd/async-tutorial branch 3 times, most recently from 94fff65 to 897bb93 Compare February 19, 2024 04:51
Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

Consider how you went about building this incrementally yourself. There would have been many points where you stopped and ran the application. The order which this is presented as a tutorial doesn't allow for that approach. I wonder if it should. To do that you would need to move the widgets and draw earlier, incrementally add functionality, etc. E.g. maybe consider having a specific result type that is pre-formatted, instead of just wrapping the api type. When you then add the actual calls, this just become a mapping rather than implementing behavior.

code/crates-tui-tutorial-app/LICENSE Outdated Show resolved Hide resolved
code/crates-tui-tutorial-app/src/app.rs Outdated Show resolved Hide resolved
code/crates-tui-tutorial-app/src/app.rs Outdated Show resolved Hide resolved
code/crates-tui-tutorial-app/src/errors.rs Outdated Show resolved Hide resolved
code/crates-tui-tutorial-app/src/errors.rs Outdated Show resolved Hide resolved
src/content/docs/tutorials/crates-tui/errors.md Outdated Show resolved Hide resolved
src/content/docs/tutorials/crates-tui/events.md Outdated Show resolved Hide resolved
src/content/docs/tutorials/crates-tui/main.md Outdated Show resolved Hide resolved
code/crates-tui-tutorial-app/src/events.rs Outdated Show resolved Hide resolved
src/content/docs/tutorials/crates-tui/app_handle_event.md Outdated Show resolved Hide resolved
@kdheepak kdheepak force-pushed the kd/async-tutorial branch 6 times, most recently from 7168bb6 to 2c518f7 Compare February 20, 2024 11:54
@kdheepak kdheepak force-pushed the kd/async-tutorial branch 5 times, most recently from ed4293f to 5e3f562 Compare February 21, 2024 01:59
@joshka
Copy link
Member

joshka commented Feb 24, 2024

Partial review:

Crates TUI | Ratatui

  1. Should we base the tutorials on a template to simplify the tutorial?
  2. One thing that someone is going to do sometime is publish their tutorial crate directly... We should pre-empt that with a reservation of the name to prevent this.
  3. Link to the finished source code in the first page - some people like to just read along rather than typing code (we might need to create a code base with the anchors and one without - or something like that)
  4. Screenshot:
    • last key event seems like it should go at the bottom, not the top
    • Let's add a header noting that this is the tutorial
    • Downloads header should be right aligned
    • Add commas to the download counts
  5. Cargo.toml Put the instruction above the code
  6. Perhaps in the note: "If you haven't done any asynchronous programming, we recommend to read the Tokio tutorials before reading this one."

Main | Ratatui

  1. I tend to avoid putting the output in the same block as shell code that you intend people to copy paste from (I think there was a starlight or astro thread about this somewhere - possibly referencing the google style guide). Minor nit I think
  2. Tip seems extraneous to the tutorial.
  3. Homework isn't anything to do with Ratatui
  4. This page seems not relevant to ratatui at all

Crates IO API Helper | Ratatui

  1. Strong dislike for things named "Helper" in general. (It often indicates a missing concept with some better name)
  2. I wouldn't worry about telling how to run individual tests. Just run everything - if things are slow then the tests should be fixed. I'd guess a large proportion of users will be using VSCode and can just click the Run test button.
  3. Use @example.com for the email domain
  4. Use crates-tui-tutorial for the user agent
  5. Add a comment about why the 1 second rate limit is there (with a link to the policy).
  6. The tip about the email variables gets in the way of getting to the content - remove or drastically simplify. Up to the Crates Query section, the doc is 2.5 pages for 4 lines of code. Signal to noise is way too low.
  7. (Maybe we need some collapsible admonishments?)
  8. As someone familiar with async, I don't like the response in the parameters pattern we have going on here. It looks janky. As a newb reading this I'd wonder why we're doing that - it seems odd.
  9. Feels like we jumped all the way into the center of the app too soon without establishing a need for the solution being presented -
  10. The breaking up of the code and headers above every block is too much organization that breaks the flow of reading. Drop the headers
  11. Why is crates assigned twice?
  12. I don't understand the naming of app_crates
  13. Ugh - The write it and refactor it pattern is bad - just write good code upfront.
  14. Why the String error in the refactor section? We have Color eyre - use it.
  15. Why let and return clippy?
  16. Why not use crates_io_api?
  17. fetch_crates → query_crates perhaps (is there a canonical rust name for this - I know that fetch would appeal to web devs, but it seems out of place here)
  18. Why the spawn task just to await it in the test?
  19. Why println in the test?
  20. This page seems overcomplicated, and builds a lego block that seems oddly shaped without rationale. Suggestion, start with a Tui related problem statement and then align the code that we write on that page to solving that problem. Perhaps build the desired UI and model first and then write the backing code second?
  21. I think I'd just call this crates_search.rs / CratesSearch (with appropriate methods) - maybe, but I'd want it to fit with the model of the UI rather than being designed without that in mind.

Tui | Ratatui

  1. I just noticed that the page titles could be a bit better...
  2. Link to Raw mode and alternate screen docs
  3. Enter raw mode - to avoid buffering keys until a new line, echoing keys when they're pressed, and turning off newline output. Nothing mouse related
  4. Why no use statements for crossterm/std::io? The code looks verbose - which makes it less easy to scan. Perhaps just collapse the imports?
  5. "returns an instance of ratatui::terminal::Terminal" instead of "returns the ratatui::terminal::Terminal struct.". or something like that - newb focus might find that wording confusing?
  6. I think I'd prefer the first render to perhaps have a frame of an app instead of hello world. Too slow at this point. Give me some content here.
  7. I'm 4 pages in and don't have anything meaningful to show yet....

Errors | Ratatui

  1. Perhaps drop the narrative on this page entirely and point at the counter app error handling Counter App Error Handling | Ratatui Just show the code.
  2. Too many options discussed for a tutorial - I think it might be useful to have these, but they interrupt the text too much and make it difficult to get the signal through the noise - perhaps move extra stuff to the end (like human panic)
  3.         env!("CARGO_PKG_REPOSITORY") isn't going to be set by the user unless they're using github or something like that...
    
  4. reset doesn't work on window

Events | Ratatui

  1. We’ve already discussed Events and an EventHandler extensively in the counter app. And you can use the exact same approach in your async application. If you do so, you can ignore this section.
I think this could talk more about what difference you're talking about here - assuming that the reader has read a particular tutorial and knows it well enough to understand which particular context is meant here is not a good idea - expect non-linear reading

  2. I'd prefer less you can do it like blah, or blah and more: In this tutorial we're going to do it like blah because ...
  3. There's an existing type alias... BoxStream in futures::stream - Rust or LocalBoxtStream And StreamExt in futures::stream - Rust fn boxed<'a>(self) -> Pin<Box<dyn Stream<Item = Self::Item> + Send + 'a>> where Self: Sized + Send + 'a, /// Wrap the stream in a Box, pinning it.
  4. This page doesn't have a goal / problem / solution. This make the reader wonder why we're doing this. Tell me what we're doing up front, then do it, then tell me what we did.
  5. Crossterm event - do we not care about errors - seems bad to just swallow them.
  6. Render stream. "We're going to render at a constant frame rate so that ..."
  7. Demo - code is too long without a break. too much nesting in the code makes it difficult to consume fast.
  8. I like the idea of the demo, but I think perhaps introducing the render event later could have been more effective. Instead if we arrange this around the simplest code needed to get an async request for the search data - will expand this point in a message.
  9. The homework seems badly phrased - perhaps "last" instead of "current"?

App Basic Structure | Ratatui

  1. Ugh - refactor?
  2. Q: what's the purpose of the last key event idea from a teaching perspective?
  3. I think I prefer exit over quit - quit is something the user does, exit is something the app does.
  4. Given the limited number of events (Crossterm/Render/Draw), I think i'd leave the main match statement in the loop with something like this setup...
```rust
    while let Some(event) = events.next().await {
    Event::Crossterm(e) => self.handle_crossterm(e)?,
    Event::Render() => self.draw()?,
Event::Error() => self.handle_error()?,
    Event::Exit() => break, // maybe....
    }
5. The imports inside the methods seem unidiomatic in a way that readers will find odd
6. When you provide multiple code that could be the way to do something it makes it difficult for casual readers, as someone reading non-linearly may see one part but not the update. Instead, talk about why we're doing StatefulWidget and just write that.

[App Mode | Ratatui](https://4c4d41f8.ratatui.pages.dev/tutorials/crates-tui/app-mode/)
1. Nesting in Handle_event is too bad - my criteria for this is any time you have to make multiple choices that are strongly orthogonal, they will be more easy to understand as separate methods - here that's what type of event is this, what key is pressed, what mode are we in - so that's too many paths through a function. Reduce the complexity measure.
2. Not sure switch_mode adds value
3. The mock data idea is good, but far far too late - this should be the second or third thing the user does. And the data belongs in something that is aligned to a model that is meaningful (e.g. SearchResults containing a Vec<CrateResult> or something like that).
4. Instead of App having a Table - make the SearchResults a Widget and render it? I want to push users towards building more with Widgets rather than building with methods
5. This would be more idiomatic, concise and appropriate, but I don't know what the colors are for. What is the prompt method for?
```rust
let color = match self.mode {
    Mode::Prompt => Color::Yellow,
    _ => Color::Blue,
}
```
6. At this point, still not sure how we're going to build up the display - an outline of the steps would help. Tell me what we're going to do, do that, tell me what we did.
7. Render... I don't understand why the length is 5 - the image up top said we were going to display or not display the prompt...
8. render() there's stuff at various levels of abstraction (table calls a method, then renders that using Widget::render), prompt renders two returned values as a tuple it's not clear why the block can't just be added to the paragraph, or what the paragraph represents. Last key creates a widget right there.
9. Drop the cargo run part of the demo - useful for the first part, annoying for the general case.

[App Async | Ratatui](https://4c4d41f8.ratatui.pages.dev/tutorials/crates-tui/app-async/)
1. At this point I'm kinda tired of reading the tutorial. and there's 6 more pages to go...

@joshka
Copy link
Member

joshka commented Feb 24, 2024

@kdheepak's goals were:

  1. A learner who has never used async before should be able to read this and understand that a) they can spawn tokio tasks, b) they can use actions to communicate back from a tokio task to the main thread/task, c) they have an example of how to generate crossterm events using the crossterm stream.
  2. A learner should understand what the action pattern is, ie every action has a corresponding method, and actions can be created from any where in the app
  3. a learner should be able understand the values of making their application (or sub components) as stateful widgets
  4. The tutorial should show a learner the pattern of returning widgets from methods
  5. tutorials should show how to set up logging, panic handling, command line arguments, configuration for styles and key presses.
  6. a reader who finishes this tutorial can read the crates-tui source code and understand more complex apps.
  7. showing people how to run tests

I recrafted these a little:

  1. Someone familiar but not experienced with Tokio async can write an app that uses async background tasks to be able to do simultaneous work (a web api request, Crossterm events)
  2. Splitting the application into stateful widgets and modules helps setup a mental framework for organizing code into easy to maintain chunks
  3. The action pattern is introduced to solve the problem of communicating between the various areas of the application.
  4. Our learnings from how best to write any orthogonal concerns (logging, errors, cli args, config) should be generally baked into the tutorial rather than being developed in them.

Tutorials shouldn't generally be the place where the canonical information about why to do things lies (as their text should generally be aligned with the goal of the tutorial rather than ratatui apps as a whole. So we should eagerly identify parts of the tutorial to split out and write about elsewhere: e.g. "Want to go deeper on this topic, see https://ratatui.rs/how-to/test-widgets ..."

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.

2 participants