-
Notifications
You must be signed in to change notification settings - Fork 162
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
External printer API rework #737
Draft
IanManske
wants to merge
8
commits into
nushell:main
Choose a base branch
from
IanManske:external-printer
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
2d6fe65
Rework external printer
IanManske 570546b
Fix event polling
IanManske 0d8089e
Add doc comments and one test
IanManske fda063b
Refactor using `Receiver::try_iter`
IanManske 2baf0ff
Allow sending raw bytes
IanManske 1a548ca
Remove old comment
IanManske f5b7290
Fix empty lines being skipped
IanManske 5ddee21
Use `std::sync::mpsc` instead of `crossbeam-channel`
IanManske File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,72 +1,70 @@ | ||
//! To print messages while editing a line | ||
//! | ||
//! See example: | ||
//! | ||
//! ``` shell | ||
//! cargo run --example external_printer --features=external_printer | ||
//! ``` | ||
#[cfg(feature = "external_printer")] | ||
use { | ||
crossbeam::channel::{bounded, Receiver, SendError, Sender}, | ||
std::fmt::Display, | ||
}; | ||
use std::sync::mpsc::{self, Receiver, SyncSender}; | ||
|
||
#[cfg(feature = "external_printer")] | ||
pub const EXTERNAL_PRINTER_DEFAULT_CAPACITY: usize = 20; | ||
|
||
/// An ExternalPrinter allows to print messages of text while editing a line. | ||
/// The message is printed as a new line, the line-edit will continue below the | ||
/// output. | ||
/// An external printer allows one to print messages of text while editing a line. | ||
/// The message is printed as a new line, and the line-edit will continue below the output. | ||
/// | ||
/// Use [`sender`](Self::sender) to receive a [`SyncSender`] for use in other threads. | ||
/// | ||
/// ## Required feature: | ||
/// `external_printer` | ||
#[cfg(feature = "external_printer")] | ||
#[derive(Debug, Clone)] | ||
pub struct ExternalPrinter<T> | ||
where | ||
T: Display, | ||
{ | ||
sender: Sender<T>, | ||
receiver: Receiver<T>, | ||
#[derive(Debug)] | ||
pub struct ExternalPrinter { | ||
sender: SyncSender<Vec<u8>>, | ||
receiver: Receiver<Vec<u8>>, | ||
} | ||
sholderbach marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
#[cfg(feature = "external_printer")] | ||
impl<T> ExternalPrinter<T> | ||
where | ||
T: Display, | ||
{ | ||
/// Creates an ExternalPrinter to store lines with a max_cap | ||
pub fn new(max_cap: usize) -> Self { | ||
let (sender, receiver) = bounded::<T>(max_cap); | ||
impl ExternalPrinter { | ||
/// The default maximum number of lines that can be queued up for printing | ||
pub const DEFAULT_CAPACITY: usize = 20; | ||
|
||
/// Create a new `ExternalPrinter` with the [default capacity](Self::DEFAULT_CAPACITY) | ||
pub fn new() -> Self { | ||
Self::default() | ||
} | ||
|
||
/// Create a new `ExternalPrinter` with the given capacity | ||
/// | ||
/// The capacity determines the maximum number of lines that can be queued up for printing | ||
/// before subsequent [`send`](SyncSender::send) calls on the [`sender`](Self::sender) will block. | ||
pub fn with_capacity(capacity: usize) -> Self { | ||
let (sender, receiver) = mpsc::sync_channel(capacity); | ||
Self { sender, receiver } | ||
} | ||
/// Gets a Sender to use the printer externally by sending lines to it | ||
pub fn sender(&self) -> Sender<T> { | ||
|
||
/// Returns a new [`SyncSender`] which can be used in other threads to queue messages to print | ||
pub fn sender(&self) -> SyncSender<Vec<u8>> { | ||
self.sender.clone() | ||
} | ||
/// Receiver to get messages if any | ||
pub fn receiver(&self) -> &Receiver<T> { | ||
|
||
pub(crate) fn receiver(&self) -> &Receiver<Vec<u8>> { | ||
&self.receiver | ||
sholderbach marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
/// Convenience method if the whole Printer is cloned, blocks if max_cap is reached. | ||
/// | ||
pub fn print(&self, line: T) -> Result<(), SendError<T>> { | ||
self.sender.send(line) | ||
impl Default for ExternalPrinter { | ||
fn default() -> Self { | ||
Self::with_capacity(Self::DEFAULT_CAPACITY) | ||
} | ||
} | ||
|
||
/// Convenience method to get a line if any, doesn´t block. | ||
pub fn get_line(&self) -> Option<T> { | ||
self.receiver.try_recv().ok() | ||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
|
||
#[test] | ||
fn impls_send() { | ||
fn impls_send<T: Send>(_: &T) {} | ||
|
||
let printer = ExternalPrinter::new(); | ||
impls_send(&printer); | ||
impls_send(&printer.sender()) | ||
} | ||
} | ||
|
||
#[cfg(feature = "external_printer")] | ||
impl<T> Default for ExternalPrinter<T> | ||
where | ||
T: Display, | ||
{ | ||
fn default() -> Self { | ||
Self::new(EXTERNAL_PRINTER_DEFAULT_CAPACITY) | ||
#[test] | ||
fn receives_message() { | ||
let printer = ExternalPrinter::new(); | ||
let sender = printer.sender(); | ||
assert!(sender.send(b"some text".into()).is_ok()); | ||
assert_eq!(printer.receiver().recv(), Ok(b"some text".into())) | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
There was some subtlety around when we
crossterm::event::poll
andcrossterm::event::read
to avoid idling in a hotter than necessary loop #651So I am not sure if this
do ... while
towhile
transformation is correct.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.
Unfortunately, the current loop structure will block on the first
read
and later external messages will not get printed until the user types which is confusing. I wonder if there's a better solution though...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.
Damn you asynchronous concurrency....
Not blocking by polling certainly makes reading that channel possible.
Could we shove both kinds of events into one channel and block on that? (that would be the no async just threads everywhere path)
With how we dealt with
crossterm
and stdout so far I am certainly not screaming:cargo add tokio
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.
Using the channel + threads should work, but I think that means there will have to be 3 threads in total:
event::read
It is possible to remove the external printer thread if we wrap the external printer sender in a new type. This way, we can allow the external thread to send messages directly to the merged channel (
Reciever<ReedlineMessage>
), instead of having to move data from the external channelReciever<Vec<u8>>
to the merged channel. E.g.,:The potential problem with this is that
nu-system
, or wherever job controls ends up, would need to takereedline
as a dependency.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.
Crossterm mentions a restriction on the threads their blocking functions can be run from and that it is incompatible with their async oriented
EventStream
https://docs.rs/crossterm/latest/crossterm/event/index.html
Maybe we need to bite the
Future
bullet at some point? (providing an executor would suck for library users, but we may want to unlock things like async prompt or completion updates.)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 should be fine, one thread can both poll and read.
In the future (ha), it might be necessary. So far, it looks like we can probably work around it?
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.
Two other possible solutions:
Use a higher poll wait time like it was suggested in external_printer does not print until I press some key #757. We could use the higher wait time for the first poll only and then switch back to the current wait time after the first read.
Have only two threads:
poll
+read
loop and prints the prompt, etc.Then, we synchronize the two threads using a
Mutex
or something similar to ensure that only one of them prints to the terminal at a time. Compared to the three thread approach before, this has the benefit that it should be easier to isolateexternal_printer
handling, so users that do not use theexternal_printer
feature should not have to pay any extra performance cost.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.
Not sure how 1) would play out both in terms of external latency and syscall churn. (would need to see it)
crossterm
cursor or terminal size polling operations may collide with streaming output from another thread. But yeah with the appropriate locking this may be workable.