console-rs / indicatif

A command line progress reporting library for Rust
MIT License
4.41k stars 241 forks source link

synchronized output #618

Open Funami580 opened 10 months ago

Funami580 commented 10 months ago

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

Otherwise, the PR seems to work fine for me.

djc commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

djc commented 10 months ago

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

Funami580 commented 10 months ago

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 commented 10 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

Any update, @chris-laplante?

chris-laplante commented 9 months ago

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 https://github.com/console-rs/console/pull/205 for the console side of things, and my PR here: https://github.com/console-rs/indicatif/pull/625

Funami580 commented 9 months ago

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 https://github.com/console-rs/console/pull/205 for the console side of things, and my PR here: https://github.com/console-rs/indicatif/pull/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 commented 9 months ago

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.