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

support for synchronized output #195

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

Conversation

Funami580
Copy link

@Funami580 Funami580 commented Dec 31, 2023

I got the advice that support for synchronized output should be implemented in this crate instead of indicatif (related indicatif PR: console-rs/indicatif#618).

Currently, only checking whether synchronized output is supported is implemented. Before I continue, I want to know what the public API should look like (previous examples: console-rs/indicatif#618 (comment)).

@djc already suggested to use guards, and that I should ask the console maintainers, too. I'd also like to see some vague code for the public API, for example:

let term = Term::stdout();
if term.supports_sync_update() {
	term.begin_sync_update();
}
// do something
if term.supports_sync_update() {
	term.end_sync_update();
}
term.flush();

Another thing to notice is that we don't necessarily need to check whether that feature is supported, as terminals should ignore the control sequence (e. g. begin sync: \x1b[?2026h) if they don't support it, I think. But we could still provide the check function to the end users, as they might make use of that information.

@mitsuhiko
Copy link
Collaborator

It does make sense to me, but I'm not entirely sure if having such a stateful external API is not a massive footgun. Unfortunately since you actually need to flip those flags on the term I'm not sure if something nice can be done. There is nothing that prevents you from having two terminals to stdout/stderr in this crate. So anyone flipping this flag would obviously affect the output.

If such an API is added, I think a guard like in your other PR should be added as well.

Definitely not opposed though.

Copy link

@djc djc left a comment

Choose a reason for hiding this comment

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

Some stylistic nits.

}

fn process_byte(&mut self, byte: u8) {
match byte {
Copy link

Choose a reason for hiding this comment

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

Suggest writing this as such:

self.state = match (self.state, byte) {
    ..
};

@@ -360,3 +361,289 @@ pub fn wants_emoji() -> bool {
pub fn set_title<T: Display>(title: T) {
print!("\x1b]0;{}\x07", title);
}

fn with_raw_terminal<R>(f: impl FnOnce(&mut fs::File) -> R) -> io::Result<R> {
Copy link

Choose a reason for hiding this comment

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

If this is only used within sync_output, I would move it inside the module.

}

/// Specification: https://gist.github.com/christianparpart/d8a62cc1ab659194337d73e399004036
mod sync_output {
Copy link

Choose a reason for hiding this comment

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

I think Rust style would suggest not abbreviating synchronized_ to sync_ for the module name.

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.

None yet

3 participants