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, leveraging Funami580's work #205

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

chris-laplante
Copy link

@chris-laplante chris-laplante commented Feb 5, 2024

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

See also console-rs/indicatif#625

TODO before merge (see console-rs/indicatif#618 (comment)):

  • Conditionally usestd::ptr for macOS (I don't have a mac to compile for testing purposes)
  • Remove supports_synchronized_output entirely?
  • Windows term support

@djc
Copy link
Member

djc commented Feb 6, 2024

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

Funami580 and others added 2 commits February 6, 2024 10:04
Co-authored-by: Funami580 <63090225+Funami580@users.noreply.github.com>
@chris-laplante chris-laplante force-pushed the cpl/sync-output-drop-guard branch from d997294 to 5d0d3ea Compare February 6, 2024 15:08
@chris-laplante
Copy link
Author

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

Done.

@Funami580
Copy link

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.

enable_ansi_on(out)

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
Author

@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
Copy link

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
Copy link
Author

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
Copy link

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
Copy link
Author

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

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.

3 participants