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

synchronized output #618

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

Conversation

Funami580
Copy link

@Funami580 Funami580 commented Dec 30, 2023

Added support for synchronized output (https://gist.github.com/christianparpart/d8a62cc1ab659194337d73e399004036). Before, my multi progressbars flickered a lot. But not anymore.

Note: I added a new method to a public trait which is a breaking change. I'm still not sure, if this is the correct design (supports_ansi_codes in the public trait).

  • Is this the correct design?

Otherwise, the PR seems to work fine for me.

@Funami580 Funami580 marked this pull request as draft December 30, 2023 11:10
Cargo.toml Outdated Show resolved Hide resolved
@djc
Copy link
Member

djc commented Dec 30, 2023

I think it would be good to support this, but I also think a lot of the supporting code (about detecting and using the sync mechanism) would probably make more sense in the lower-level console crate (which indicatif uses). Maybe start a PR there (feel free to mention me)?

@Funami580
Copy link
Author

Funami580 commented Dec 30, 2023

Yes, I can do a separate PR in console. @djc Should the API look like this?

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

With sync update doing sth like write_str("\x1b[?2026h")?

Another option would be to use a closure

let term = Term::stdout();
term.[maybe_]sync_update(|| {
	term.write_line("Hello world");
});

which handles both begin and end sync update. Though here the question is if we should check whether the feature is supported inside the maybe_sync_update and otherwise fallback to not syncing at all.

@djc
Copy link
Member

djc commented Dec 30, 2023

I would do a guard type that ends the sync on dropping. But, probably good to ask the console maintainer(s) to weigh in on this.

@Funami580
Copy link
Author

Since the console crate seems a little bit inactive, I made this PR simpler. It now uses once_cell and might work on Windows, too, though I haven't checked (should be checked before merging, though). Note: I added a new method to a public trait which is a breaking change. I'm still not sure, if this is the correct design (supports_ascii_codes in the public trait).

src/term_like.rs Outdated Show resolved Hide resolved
@Funami580 Funami580 marked this pull request as ready for review January 8, 2024 17:27
@djc
Copy link
Member

djc commented Jan 9, 2024

I pinged a console maintainer on Discord to see if we can get that conversation going. :)

@Funami580
Copy link
Author

If we were to add a Sync Guard in the console crate, how would we be able to use it in indicatif? I guess via the TermLike trait, but what about the in_memory term then?

@chris-laplante
Copy link
Collaborator

If we were to add a Sync Guard in the console crate, how would we be able to use it in indicatif? I guess via the TermLike trait, but what about the in_memory term then?

The in-mem term is just for testing purposes. I don't think we'd need sync guard there necessarily?

@Funami580
Copy link
Author

Okay, but then what about custom TermLike implementations? indicatif provides the ability to create a ProgressBar via a Box<TermLike>. Right now with supports_ansi_codes, it will work for these, too, but how would we generalize the TermLike trait to allow for custom implementations and also be able to use the console crate's SyncGuard?

@chris-laplante
Copy link
Collaborator

Okay, but then what about custom TermLike implementations? indicatif provides the ability to create a ProgressBar via a Box<TermLike>. Right now with supports_ansi_codes, it will work for these, too, but how would we generalize the TermLike trait to allow for custom implementations and also be able to use the console crate's SyncGuard?

Ah ok. I see what you mean. Let me think about it a bit...

@Funami580
Copy link
Author

Any update, @chris-laplante?

@chris-laplante
Copy link
Collaborator

Any update, @chris-laplante?

Sorry, I finally got time to look at this. I think I was able to put all the pieces together in a way that makes sense for both crates. Please see console-rs/console#205 for the console side of things, and my PR here: #625

@Funami580
Copy link
Author

Sorry, I finally got time to look at this. I think I was able to put all the pieces together in a way that makes sense for both crates. Please see console-rs/console#205 for the console side of things, and my PR here: #625

Thanks for taking a look at this! These PRs look good to me!

Still TODO: implement support for WASM, Windows terminals.

Just a note for the future: The Windows 10 terminal can support ANSI codes, if you enable them. If you call colors_supported(), it tries to enable ANSI escape sequences and returns true on success. ANSI escape sequences are needed for the supports_synchronized_output() function (as well as the SyncGuard) to work correctly.

https://github.com/console-rs/console/blob/de2f15a31a8fef0b0e65ef4bdf92cd03c3061dac/src/windows_term/mod.rs#L77

So in the future it might look like this.

#[inline]
pub fn is_synchronized_output_supported(&self) -> bool {
    #[cfg(unix)]
    {
        supports_synchronized_output()
    }
    #[cfg(not(unix))]
    {
        self.colors_supported() && supports_synchronized_output()
    }
}

I think it would be more efficient to cache the result of colors_supported(), though, instead of re-doing the operation for every SyncGuard.

But right now, the with_raw_terminal that supports_synchronized_output uses doesn't work for Windows, so it won't work.

In my last update for this PR, I simply removed supports_synchronized_output since the ANSI escape sequences for synchronizing are invisible in the terminal, if the terminal does not support synchronization (when the terminal supports ANSI escape sequences).

By the way, this one line that I removed in console (use std::ptr;) was wrong, since macOS needs this and the tests for it now fail. It should be possible to put this line in fn select_fd(...), however.

@chris-laplante
Copy link
Collaborator

Sorry, I finally got time to look at this. I think I was able to put all the pieces together in a way that makes sense for both crates. Please see console-rs/console#205 for the console side of things, and my PR here: #625

Thanks for taking a look at this! These PRs look good to me!

Still TODO: implement support for WASM, Windows terminals.

Just a note for the future: The Windows 10 terminal can support ANSI codes, if you enable them. If you call colors_supported(), it tries to enable ANSI escape sequences and returns true on success. ANSI escape sequences are needed for the supports_synchronized_output() function (as well as the SyncGuard) to work correctly.

https://github.com/console-rs/console/blob/de2f15a31a8fef0b0e65ef4bdf92cd03c3061dac/src/windows_term/mod.rs#L77

So in the future it might look like this.

#[inline]
pub fn is_synchronized_output_supported(&self) -> bool {
    #[cfg(unix)]
    {
        supports_synchronized_output()
    }
    #[cfg(not(unix))]
    {
        self.colors_supported() && supports_synchronized_output()
    }
}

I think it would be more efficient to cache the result of colors_supported(), though, instead of re-doing the operation for every SyncGuard.

But right now, the with_raw_terminal that supports_synchronized_output uses doesn't work for Windows, so it won't work.

In my last update for this PR, I simply removed supports_synchronized_output since the ANSI escape sequences for synchronizing are invisible in the terminal, if the terminal does not support synchronization (when the terminal supports ANSI escape sequences).

By the way, this one line that I removed in console (use std::ptr;) was wrong, since macOS needs this and the tests for it now fail. It should be possible to put this line in fn select_fd(...), however.

Would you mind copying this comment to the PR in console? That way I can reply to the console specific parts there.

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.

4 participants