-
Notifications
You must be signed in to change notification settings - Fork 58
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: Add spawn vim docs #651
base: main
Are you sure you want to change the base?
Conversation
This seems overly specific to the component template and has a lot of code just to show off the vim part. I have a few concerns with this:
If you were able to simplify this to an app that just showed off the problem and solution, this would be much clearer.
The solution really needs to show off how to:
This likely should also have a bit of discussion about termion at the end that shows how to access the backend to do this (requires an unstable flag to get at the backend_mut function and call the suspend functions) This is just my opinion - I know you've been working on this with @kdheepak who knows a lot more about this template than I do, so I'd let him chime in about whether this is worth keeping in this form or simplifying. |
Thanks for the feedback! |
I agree with your comments. It should be a lot simpler to discuss just the aspect of starting an external process like One thing I wanted to note is that when I've attempted to do do this without the pattern used in the See for example this commit from @orhun where he made a change to ensure it worked correctly: orhun/rattler-build@84ea16a I first saw this issue in kdheepak/taskwarrior-tui#46 When this issue occurs, the response is written to stdin which is read by crossterm as if a user is typing it and that is sent to the keyevents and that ends up getting processed by the TUI. I know that the combination of using This issue is particularly annoying because it is not always reproducible. I never noticed it on iTerm until I decreased the tick time when using Currently, I don't know why the other methods didn't work, and I definitely didn't comprehensively test everything. I was changing multiple things at the same time when I was testing this in I would like to ensure that this RGB issue doesn't occur with whatever recipe is posted on ratatui.rs because if it does we'll get a lot of questions about what is going on for users that copy paste the code. |
Thanks for sharing a PR and describing your steps to solve this problem here and on discourse! Even just this information will be useful for those that try to do the same thing and come across this! |
212c8ca
to
883d87b
Compare
@joshka @kdheepak
If any of you can review these new changes whenever you get a chance, that will be helpful! Thanks!! |
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.
The goal of the recipes section is less step by step tutorial and more an explanation of a snippet of code. What about dropping this down to just two code blocks, one showing the the main, handle_events and run_editor code in a single block, with the main and handle events methods collapsed, and when showing the full file, fully collapsed?, E.g. use the following and add a direct link to the code file in the repo.
```rust collapsed title="main.rs (click to expand)"
The main thing this article really needs to do is say something like (feel free to rephrase):
Before you run another app from your app, your app is in control of the input and output. Your app needs to doing that while the other app is running otherwise ...
It likely also needs to draw comparisons about synchronous, threaded, and async event handling.
code/how-to-spawn-vim/src/main.rs
Outdated
stdout().execute(LeaveAlternateScreen)?; | ||
disable_raw_mode()?; | ||
// Launch vim and wait for status | ||
// You may change this to other editors like nvim, nano, etc. | ||
Command::new("vim").arg("/tmp/a.txt").status()?; | ||
stdout().execute(EnterAlternateScreen)?; | ||
enable_raw_mode()?; | ||
// Clear the terminal and force a full redraw on the next draw call. | ||
terminal.clear()?; |
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.
This is really the crux of the entire article, and it deserves it's own code block, everything else is boilerplate that could be in any app. I'd suggest pulling this out into a function and showing it with some narrative.
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 agree that this part is important, but it is the obvious part imo, and pausing the key event handler before spawning vim and restarting the key event handler after resuming from vim exiting is imo the part that will trip most people.
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.
In these examples we don't have a background key handler, so what I expect will happen is that someone will copy this code into their app and it won't work as expected.
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 apologize for the delay in addressing this PR over the past two weeks. I agree with @kdheepak's concern about users with advanced setups experiencing issues when copying the snippet. Here are two possible solutions:
- Add a note to the documentation illustrating how to spawn an external process (e.g., Vim) using a component template. I have created a proof of concept here: feat: Add ExecuteCommand action in component template templates#62
- Enhance our snippet for spawning Vim by utilizing a similar but simplified version of the tui module component template.
…d-tutorial, update root lockfile Signed-off-by: Deep Panchal <[email protected]>
Signed-off-by: Deep Panchal <[email protected]>
Signed-off-by: Deep Panchal <[email protected]>
Signed-off-by: Deep Panchal <[email protected]>
…, update anchors Signed-off-by: Deep Panchal <[email protected]>
…tandable Signed-off-by: Deep Panchal <[email protected]>
… fix md link in index.md Signed-off-by: Deep Panchal <[email protected]>
5db9906
to
7be0016
Compare
I have updated the PR to focus more on the logic for spawning Vim. I have also updated the image of generated docs on PR description. Please let me know if this is acceptable :) |
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.
LGTM overall, but I feel like it is missing some parts.
Mainly, the edge case of ANSI code being printed should be mentioned somewhere. We can also give some example TUI apps which implements this.
Also, if the user only wants to launch vim to edit something, using e.g. edtui might also come in handy.
Lastly, clippy is complaining :)
events. The `handle_events` function listens for key events and returns an `Action` based on the key | ||
pressed. | ||
|
||
## Spawning vim |
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.
nit:
## Spawning vim | |
## Spawning Vim |
|
||
## Spawning vim | ||
|
||
Now, lets define the function `run_editor` function attached to `Action::EditFile` action. |
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.
Now, lets define the function `run_editor` function attached to `Action::EditFile` action. | |
Now, let's define the function `run_editor` function attached to `Action::EditFile` action. |
[full code](https://github.com/ratatui-org/ratatui-website/tree/main/code/how-to-spawn-vim/src/main.rs). | ||
Next, we spawn a child process with `Command::new("vim").arg("/tmp/a.txt").status()` which launches | ||
`vim` to edit the given file. At this point, we have given up control of our TUI app to vim. Our TUI | ||
app will now wait for the exit status of the child process (i.e., Vim). Once the user exits Vim, our |
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 feel like Vim is mentioned too many times here :D
app will now wait for the exit status of the child process (i.e., Vim). Once the user exits Vim, our | |
app will now wait for the exit status of the child process. Once the user exits Vim, our |
Description
This PR adds
how-to-spawn-vim
documentation underRecipes > Applications > Spawn External Editor (Vim)
I am now using
hello-world-template
as base for my example to keep things simple (31fc71c) and I have added logic to spawn vim in a separate commit (8380d5b).I am not familiar with astro but I tried to use other examples as reference to write this documentation. Please let me know if I missed anything. Thanks!!
Related Issue
Ref #406
Images