console-rs / console

A rust console and terminal abstraction
MIT License
961 stars 113 forks source link

Support for synchronized output, leveraging Funami580's work #205

Open chris-laplante opened 10 months ago

chris-laplante commented 10 months ago

Funami580 did the heavy lifting - all I did was take their SyncGuard and move it here into console.

See also https://github.com/console-rs/indicatif/pull/625

TODO before merge (see https://github.com/console-rs/indicatif/pull/618#issuecomment-1928582149):

djc commented 9 months ago

@chris-laplante this has some conflicts still, can you clean those up? @mitsuhiko got time to look at this?

chris-laplante commented 9 months ago

@chris-laplante this has some conflicts still, can you clean those up? @mitsuhiko got time to look at this?

Done.

Funami580 commented 9 months ago

As per request I copied this comment from my indicatif PR:

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

@Funami580 Thank you! Would you be able to make the change for std::ptr? I don't have a mac :(.

I've given you push access to my fork (https://github.com/chris-laplante/console). Or at least I think I did...

Funami580 commented 9 months ago

Would you be able to make the change for std::ptr?

Sure, done!

I don't have a mac :(.

I don't have a mac either ^^

chris-laplante commented 9 months ago

Would you be able to make the change for std::ptr?

Sure, done!

I don't have a mac :(.

I don't have a mac either ^^

Thanks! I guess I could have made that change after all lol 🤡

Happy to share the commit credit though :). If you want to make any of the other changes you described before, feel free.

Funami580 commented 9 months ago

If you want to make any of the other changes you described before, feel free.

Thanks for the option, but I'm not sure if the changes (I think you mean removing supports_synchronized_output) are the right thing to do, so I'll leave the decision to you guys. ^^

chris-laplante commented 9 months ago

If you want to make any of the other changes you described before, feel free.

Thanks for the option, but I'm not sure if the changes (I think you mean removing supports_synchronized_output) are the right thing to do, so I'll leave the decision to you guys. ^^

That's fair. I'm not a maintainer of this crate so I defer to @mitsuhiko, when he gets some time :).