feat: Add spawn vim docs #651

deepanchal · 2024-07-01T02:43:12Z

Description

This PR adds how-to-spawn-vim documentation under Recipes > 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

joshka · 2024-07-01T03:25:03Z

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:

we have to maintain that code - this is likely to get stale fast
the specific problem and solution are obscured by everything else that is part of the application (actions, events, keys, etc.)
people unfamiliar with the component template have to learn how that works just to understand the recipe
The tui module setup code likely will go away at some point as it gets replaced by stuff from feat(backend): Add convenience methods for setting up terminals ratatui#1180 or feat: simplify backend and terminal creation ratatui#280 (or whatever we settle on)
What the problem is and how the solution solves it isn't all that clear

If you were able to simplify this to an app that just showed off the problem and solution, this would be much clearer.
The problem are:

applications read events, either synchronously, in another thread, or in an async task via the event stream
applications manage the terminal state, such as raw mode, alternate screen, mouse capture etc.

The solution really needs to show off how to:

before spawning processes suspend / stop any event reading
restore the terminal state up that whatever your sub process needs to work properly
reset these after the sub process finishes
any abstractions like the component template on top of the plain crossterm stuff should probably be omitted (or perhaps noted for follow up with a some small snippets)

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.

deepanchal · 2024-07-01T03:37:05Z

Thanks for the feedback!
As I was writing this, I also noticed that there's quite a lot of boilerplate coming from component template. I was in the process of removing excess code from this template. I totally agree with your key points on clear problem and solution. I will address those points and make it more cleaner. Also, I am not familiar with writing documentation so I am sure that the wording in the documentation can be improved. I will try to improve docs as well. I will wait for @kdheepak's opinion before I make any big changes in the PR.

kdheepak · 2024-07-01T05:28:48Z

I agree with your comments. It should be a lot simpler to discuss just the aspect of starting an external process like vim.

One thing I wanted to note is that when I've attempted to do do this without the pattern used in the component template, it resulted in issues where ANSI RGB values would be printed into the TUI on returning back from vim.

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
The reason this occurred is because when vim wants to find out the background color of the terminal, and the response to that request is written to stdin.

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 select! + cancellation_token + tokio the way it is used in the component template does not cause this issue. But I haven't tested other ways to narrow down exactly what causes it and what doesn't cause it.

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 tokio. I noticed it happened more when I was using a background thread to send events to the main thread and was not able to solve it when using a AtomicBool as a Mutex lock to pause the background thread.

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 taskwarrior-tui and landed on a solution with tokio, select! and cancellation_tokens that worked.

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.

kdheepak · 2024-07-01T05:29:59Z

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!

deepanchal · 2024-07-01T16:46:00Z

@joshka @kdheepak
I have force pushed to this branch with a much simpler example using hello-world template and updated the docs accordingly. I just had a few questions

I feel that I am not explaining the process clearly in Handle Events section. Is the template too simple that it doesn't address the problems and solutions mentioned in feat: Add spawn vim docs #651 (comment)
Does this example need vhs gif to demonstrate how it works?
I have added a tip to use edit crate for opening editor at the very bottom. Should this be moved to Handle Events section?
Should I include my original diff from this pr comment Add section on how to spawn vim from a ratatui application #406 (comment) as a tip in this doc?

If any of you can review these new changes whenever you get a chance, that will be helpful! Thanks!!

code/how-to-spawn-vim/src/main.rs

joshka

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.

joshka · 2024-07-02T17:06:50Z

code/how-to-spawn-vim/src/main.rs

+                                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()?;


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.

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.

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.

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.

src/content/docs/recipes/apps/index.md

code/how-to-spawn-vim/src/main.rs

deepanchal · 2024-07-13T19:53:49Z

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 :)
Thanks!

orhun

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 :)

src/content/docs/recipes/apps/spawn-vim.md

…d-tutorial, update root lockfile 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]>

… fix warnings

deepanchal · 2024-09-02T16:13:49Z

@orhun I apologize for the delay in addressing your review comments. I have pushed new changes in 29b30d8

Can you or any other maintainers please re-review my new changes?

joshka

A few things to fix up based on Ratatui 0.28.1 changes, but otherwise LGTM

code/recipes/how-to-spawn-vim/src/main.rs

src/content/docs/recipes/apps/spawn-vim.md

joshka

LGTM - I'll wait on @kdheepak or @orhun to take a look at this one before merging it as they previously were looking at this.

deepanchal · 2024-09-02T21:28:55Z

Sounds good 👍
Thanks for feedback!!

Also just a side note, I was trying to build this tui for a cli note taking app when I ran into this vim spawning issue https://github.com/deepanchal/dnote-tui. Once I get that project in a more stable state, I will try to add it as showcase on ratatui docs :)

kdheepak · 2024-09-03T04:33:56Z

src/content/docs/recipes/apps/spawn-vim.md

+One more thing to note is that when attempting to start an external process without using the
+pattern in the component template, issues can arise such as ANSI RGB values being printed into the
+TUI on returning back from external process. If you encounter such issues, please refer to
+[orhun/rattler-build@84ea16a](https://github.com/orhun/rattler-build/commit/84ea16a4f5af33e2703b6330fcb977065263cef6)
+and [kdheepak/taskwarrior-tui#46](https://github.com/kdheepak/taskwarrior-tui/issues/46). Using
+`select!` + `cancellation_token` + `tokio` as in the component template avoids this problem.


Might be useful to mention the reason the RGB values are being printed to the terminal is because vim requests the terminal background color, and when the terminal responds over stdin, those are being read by crossterm instead.

I have added this change along with mentioning editor-command in tips section from this issue comment #406 (comment)

Edit: Also fixed format ci check

orhun

LGTM!

…and` crate

deepanchal marked this pull request as ready for review July 1, 2024 02:47

deepanchal mentioned this pull request Jul 1, 2024

Add section on how to spawn vim from a ratatui application #406

Open

deepanchal force-pushed the feat/how-to-spawn-vim-example branch from 212c8ca to 883d87b Compare July 1, 2024 16:15

deepanchal commented Jul 1, 2024

View reviewed changes

code/how-to-spawn-vim/src/main.rs Outdated Show resolved Hide resolved

deepanchal mentioned this pull request Jul 1, 2024

fix: rename Action::Refresh -> Action::ClearScreen, add handler, update tui restore logic after suspend ratatui/templates#61

Merged

joshka reviewed Jul 2, 2024

View reviewed changes

deepanchal mentioned this pull request Jul 13, 2024

feat: Add ExecuteCommand action in component template ratatui/templates#62

Closed

deepanchal force-pushed the feat/how-to-spawn-vim-example branch from 5db9906 to 7be0016 Compare July 13, 2024 19:44

deepanchal requested review from joshka and kdheepak July 13, 2024 19:54

orhun reviewed Jul 26, 2024

View reviewed changes

src/content/docs/recipes/apps/spawn-vim.md Outdated Show resolved Hide resolved

src/content/docs/recipes/apps/spawn-vim.md Outdated Show resolved Hide resolved

src/content/docs/recipes/apps/spawn-vim.md Outdated Show resolved Hide resolved

deepanchal added 7 commits August 31, 2024 21:46

feat(code): add how-to-spawn-vim example starter code from hello-worl…

c44aec2

…d-tutorial, update root lockfile Signed-off-by: Deep Panchal <[email protected]>

feat(code): add logic to spawn vim to how-to-spawn-vim example

af76471

Signed-off-by: Deep Panchal <[email protected]>

fix(code): update & add anchors in how-to-spawn-vim example

bb1741c

Signed-off-by: Deep Panchal <[email protected]>

feat(docs): add spawn-vim docs in recipes/apps section

f612ea5

Signed-off-by: Deep Panchal <[email protected]>

fix(code): use terminal.clear() to refresh terminal after exiting vim…

6ebd282

…, update anchors Signed-off-by: Deep Panchal <[email protected]>

refactor(code): update how-to-spawn-vim code to be modular and unders…

474c146

…tandable Signed-off-by: Deep Panchal <[email protected]>

feat(docs): improve spawn-vim docs by emphasizing spawning vim logic,…

798efbe

… fix md link in index.md Signed-off-by: Deep Panchal <[email protected]>

deepanchal force-pushed the feat/how-to-spawn-vim-example branch from 7be0016 to fc54fe7 Compare September 1, 2024 03:17

fix(docs): improve how-to-spawn-vim docs, upgrade ratatui to v0.28.1,…

29b30d8

… fix warnings

deepanchal force-pushed the feat/how-to-spawn-vim-example branch from fc54fe7 to 29b30d8 Compare September 2, 2024 16:00

deepanchal requested a review from orhun September 2, 2024 16:14

joshka reviewed Sep 2, 2024

View reviewed changes

deepanchal force-pushed the feat/how-to-spawn-vim-example branch 2 times, most recently from 10da0ee to 6a68598 Compare September 2, 2024 19:52

fix(docs): use new terminal funcs from v0.28, update docs

b85d980

deepanchal force-pushed the feat/how-to-spawn-vim-example branch from 6a68598 to b85d980 Compare September 2, 2024 21:23

joshka approved these changes Sep 2, 2024

View reviewed changes

kdheepak reviewed Sep 3, 2024

View reviewed changes

kdheepak approved these changes Sep 3, 2024

View reviewed changes

orhun approved these changes Sep 3, 2024

View reviewed changes

fix(docs): mention reason for rgb values being printed & `editor-comm…

6e406d4

…and` crate

deepanchal requested a review from kdheepak September 3, 2024 15:25

joshka merged commit 5ac7ecf into ratatui:main Sep 3, 2024

deepanchal deleted the feat/how-to-spawn-vim-example branch September 3, 2024 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add spawn vim docs #651

feat: Add spawn vim docs #651

deepanchal commented Jul 1, 2024 •

edited

Loading

joshka commented Jul 1, 2024

deepanchal commented Jul 1, 2024

kdheepak commented Jul 1, 2024 •

edited

Loading

kdheepak commented Jul 1, 2024 •

edited

Loading

deepanchal commented Jul 1, 2024

joshka left a comment

joshka Jul 2, 2024

kdheepak Jul 2, 2024

kdheepak Jul 2, 2024

deepanchal Jul 13, 2024

deepanchal commented Jul 13, 2024

orhun left a comment

deepanchal commented Sep 2, 2024

joshka left a comment

joshka left a comment

deepanchal commented Sep 2, 2024

kdheepak Sep 3, 2024

deepanchal Sep 3, 2024 •

edited

Loading

orhun left a comment

feat: Add spawn vim docs #651

feat: Add spawn vim docs #651

Conversation

deepanchal commented Jul 1, 2024 • edited Loading

Description

Related Issue

Images

joshka commented Jul 1, 2024

deepanchal commented Jul 1, 2024

kdheepak commented Jul 1, 2024 • edited Loading

kdheepak commented Jul 1, 2024 • edited Loading

deepanchal commented Jul 1, 2024

joshka left a comment

Choose a reason for hiding this comment

joshka Jul 2, 2024

Choose a reason for hiding this comment

kdheepak Jul 2, 2024

Choose a reason for hiding this comment

kdheepak Jul 2, 2024

Choose a reason for hiding this comment

deepanchal Jul 13, 2024

Choose a reason for hiding this comment

deepanchal commented Jul 13, 2024

orhun left a comment

Choose a reason for hiding this comment

deepanchal commented Sep 2, 2024

joshka left a comment

Choose a reason for hiding this comment

joshka left a comment

Choose a reason for hiding this comment

deepanchal commented Sep 2, 2024

kdheepak Sep 3, 2024

Choose a reason for hiding this comment

deepanchal Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

orhun left a comment

Choose a reason for hiding this comment

deepanchal commented Jul 1, 2024 •

edited

Loading

kdheepak commented Jul 1, 2024 •

edited

Loading

kdheepak commented Jul 1, 2024 •

edited

Loading

deepanchal Sep 3, 2024 •

edited

Loading