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

feat: supoort Windows Terminal #40

Closed
wants to merge 2 commits into from
Closed

feat: supoort Windows Terminal #40

wants to merge 2 commits into from

Conversation

TerakomariGandesblood
Copy link
Contributor

Trying to support the preview version of Windows Terminal.
Known issues: #37 (comment)

Comment on lines +48 to +51
let mut picker = {
let font_size = (8, 16);
Picker::new(font_size)
};
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 used to calculate the amount of space an image will occupy in character cells, it should be 10x20 for Windows Terminal (that's the cell size of the original VT340 hardware terminal).

You can also query the cell size with the CSI 16 t escape sequence.

Comment on lines +264 to +267
// Since only the latest preview version of Windows Terminal supports Sixels,
// it is necessary to get the version and determine whether it supports Sixels or not.
#[cfg(windows)]
fn support_sixels() -> Result<bool> {
Copy link

Choose a reason for hiding this comment

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

You should be able to determine sixel support from the device attributes query, as is already done here:

ratatui-image/src/picker.rs

Lines 344 to 346 in e630e68

if buf.contains(";4;") || buf.contains("?4;") || buf.contains(";4c") || buf.contains("?4c") {
return Ok(ProtocolType::Sixel);
}

That way you'll be able to support sixels in other terminals as well.

Copy link
Owner

@benjajaja benjajaja left a comment

Choose a reason for hiding this comment

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

@j4james pointed out elsewhere that windows terminal effectively has a way to query font size!

So we can query the font size and sixel support without having to rely on guessing by processes and version files and so on. But to query, we currently do a bunch of things.

On POSIX, we put the terminal into canonical mode, disable echoing, and use fcntl to set the terminal in non-blocking mode.

On windows, I think (with the help of ChatGPT) that we should use SetConsoleMode to disable ENABLE_ECHO_INPUT and ENABLE_LINE_INPUT, and use PeekConsoleInput to read input without blocking indefinitely.

My plan of refactor would be:

  1. Try out CSI 16 t (or 14) to get the font-size without tcgetwinsize first to test the waters.
  2. Let query_device_attrs take the query as param, and let it return the response string.
  3. Put the response->protocol parsing somewhere else, e.g. just inside guess_protocol.
  4. Provide a windows version of query_device_attrs with the above mentioned Windows API calls instead of the POSIX API calls.
  5. Consider removing from_termios() and just have one method/constructor that queries the terminal for everything 😍

@TerakomariGandesblood
Copy link
Contributor Author

TerakomariGandesblood commented Oct 7, 2024

Sounds good. I hadn't thought of these things because I'm not familiar enough with terminals, so it looks like I have some learning to do.

@benjajaja
Copy link
Owner

I can take care of steps 1-3.

@benjajaja benjajaja force-pushed the master branch 2 times, most recently from 122bd1f to df4605b Compare October 7, 2024 21:40
@benjajaja
Copy link
Owner

benjajaja commented Oct 13, 2024

@TerakomariGandesblood I did some changes to Picker on master. You should try it out, it should just work with the terminal of WSL if I understand correctly. For non-WSL (so only halfblocks but proper alignement) it may also be possible to translate query_stdio_capabilities to the Windows Console API.

Edit: fixing the CI for windows builds, should be ready in a while...

@TerakomariGandesblood
Copy link
Contributor Author

Great job, I'll check it out in a few days

@TerakomariGandesblood
Copy link
Contributor Author

Already implemented this feature in other PR, so close this PR.

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