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: add ratatui widget feature #38

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joshka
Copy link

@joshka joshka commented Dec 5, 2024

Adds a new ratatui widget behind a ratatui feature flag.

Implemented backed by an Arc<Mutex<Vec>> which is cleared anytime
there's new info to be printed.

Adds a new ratatui widget behind a ratatui feature flag.

Implemented backed by an Arc<Mutex<Vec<u8>>> which is cleared anytime
there's new info to be printed.

/// Handles the Printing logic for the Spinner
#[derive(Default, Copy, Clone)]
Copy link
Author

Choose a reason for hiding this comment

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

This is technically a breaking change, but I don't think it's possible to implement this without removing Copy.

}

impl Stream {
Copy link
Author

Choose a reason for hiding this comment

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

There's a lot of churn in this. In summary:

  • rearranged to read well top to bottom
  • methods which were passed the writer now just get self and pull the writer if needed
  • write calls which would have written ansi / control characters to the stream are now separated from the calls which write info (e.g. \r and the erase line sequence.

/// ```
#[derive(Clone, Default)]
pub struct SpinnerWidget {
value: Arc<Mutex<Vec<u8>>>,
Copy link
Author

Choose a reason for hiding this comment

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

It's possible that this could have been the shared state instead of the entire widget, but I prefer passing around wrapped shared state values instead of raw values. Do you have preferences one way or another?

@FGRibreau
Copy link
Owner

Thanks a lot for your submission @joshka,

However I don't want to couple spinners with ratatui but however, if you have currently issues integrating spinners inside ratatui spinner widget because spinners needs some rework to make it work to make it further generec, then I'm OK if we go down that path :)

@joshka
Copy link
Author

joshka commented Dec 5, 2024

The problem is the integration with the way this lib does the stream. Any particular reason that you don't want this?

@FGRibreau
Copy link
Owner

The problem is the integration with the way this lib does the stream.

So let's only change that, and improve spinners genericity but not include anything related to ratatui so it says clean and futur-proof for us 🙏 Could you please update your PR in that direction ?

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