Fix Windows width calculation#215
Conversation
|
Sorry, I don't have access to a Windows machine right now (and only Windows 7 at work). |
|
Would the above fix not work for |
|
Your fix will work for |
|
Yes, the normal prompt works. This fix is for when people put ANSI sequences into their prompts. |
|
Basically it lets both kinds of prompts work correctly. |
|
oh I see what you're saying. You're talking about the screenshot. No, the PR doesn't change the default behavior of the example. I just changed it to make my testing a little easier. |
|
Ok, My concerns are that:
When So I am not sure:
|
|
Just to be clear, the PR does not change the default prompt. The screenshot is only from my testing and is not part of the PR. The PR only changes how the length is calculated. If there is no ANSI in the prompt, the length is calculated correctly. |
|
@gwenn - I'm not clear what you're asking me to change so that the PR can be accepted. |
|
@jonathandturner diff --git a/src/tty/windows.rs b/src/tty/windows.rs
index 2a44206..aa47bbd 100644
--- a/src/tty/windows.rs
+++ b/src/tty/windows.rs
@@ -550,6 +550,7 @@ impl Term for Console {
let raw = original_stdstream_mode | wincon::ENABLE_VIRTUAL_TERMINAL_PROCESSING;
self.ansi_colors_supported =
unsafe { consoleapi::SetConsoleMode(self.stdstream_handle, raw) != 0 };
+ self.ansi_colors_supported = true;
}
Some(original_stdstream_mode)
} else {The prompt is displayed correctly: Could you please tell me:
? And if you have time, without your PR, add this line: diff --git a/src/tty/windows.rs b/src/tty/windows.rs
index 2a4420689..ca0feeccf 100644
--- a/src/tty/windows.rs
+++ b/src/tty/windows.rs
@@ -398,6 +398,7 @@ impl Renderer for ConsoleRenderer {
pos.col = 0;
pos.row += 1;
}
+ debug!(target: "rustyline", "orig: {:?}, s: {:?}, pos: {:?}", orig, s, pos);
pos
}And send me the logs. |
|
I'm on cmd in Windows 10 (latest stable release). Using git sha which was the latest as of a few days ago aba9b21 The fix you suggested of forcing ansi doesn't fix the calculation. Here are some sample logs: The column miscalculation is what this PR attempts to fix. |
|
Ok, Is unexpected ! static PROMPT: &'static str = ">> ";
...
let readline = rl.readline(PROMPT);
... let mut s = State::new(&mut stdout, prompt, helper, ctx);
... let prompt_size = out.calculate_position(prompt, Position::default());Expected: |
|
@gwenn - as I mentioned earlier I am toggling the prompt to be the ansi prompt to test both non-ansi and ansi prompts. The non-ansi version works fine (but it's not what I'm trying to fix). Example: |
|
Are you saying that putting ansi in the default prompt is what is breaking it? |
|
I see. Sorry it took so long. I couldn't understand why it wouldn't be okay to have ANSI codes in the prompt. As best as I can figure out now, you put two versions of the prompt in there: one with colors and one without. The one without colors is the one used for the calculation. I went back to using a simple prompt and a colored prompt separately, and then did |
|
Yes, because there is no way to remove them on Windows < 10 where ansi esc seq are not supported, or if stdout is not a tty, or when the terminal is not supported. And, I guess there is something wrong in // To enable ANSI colors (Windows 10 only):
// https://docs.microsoft.com/en-us/windows/console/setconsolemode
if original_stdstream_mode & wincon::ENABLE_VIRTUAL_TERMINAL_PROCESSING == 0 {
let raw = original_stdstream_mode | wincon::ENABLE_VIRTUAL_TERMINAL_PROCESSING;
self.ansi_colors_supported =
unsafe { consoleapi::SetConsoleMode(self.stdstream_handle, raw) != 0 };
} |
|
I will try to have access to a windows 10 machine... |
|
Since this PR fixes the issue where people use ANSI in their prompts, wouldn't it be okay to just accept it? It doesn't break people who don't use ANSI. A use case I've seen is something like this: let readline = rl.readline(&format!(
"{}> ",
Color::Green.paint(filepath.to_string())
));Where you're also mixing in parameters into the prompt to display things like: The most natural place to put this is when you write out the prompt. I'm not sure how else you would do it. |
|
Something like this. |
|
But you are right. It's not user friendly.
|
|
Ok, here is an example with a non static prompt. |
|
Windows bug related to ansi colors is also fixed: #227. |
|
Any way around this? Is this going to be fixed? :/ |
|
Will there finally be a fix? |
|
As already explained, there should be no ansi escape seq directly in the prompt, only highlight_prompt may return ansi escape seq. |
|
@gwenn You could also just allow the user to feed a usize into a readline function that places the cursor at the given distance from the beginning. That would allow for ansi codes to be used but doesn't affect non-ansi apps. |
|
@BlackBirdTV You mean that We may introduce a trait Prompt {
/// ANSI esc sequences stripped used colors are not supported or disabled.
fn rawText(&self) -> &str;
/// May contains ANSI esc sequences
fn styledText(&self) -> &str;
/// Display size
//fn width(&self) -> usize;
}with a default impl for |
|
Since I hit this issue today, I spent some time to use ANSI codes in the prompt the supported way. I wish to not clone the prompt string every time it is displayed, which is how #215 (comment) works. Unfortunately, I don't know what was supposed to appear in the "non-static example" in #215 (comment) because the link is broken. My workaround is splitting the prompt into separate types: one for the plain prompt, and one rendered with ANSI codes. #[derive(Debug)]
struct Prompt {
count: usize,
prompt: String,
}
impl Prompt {
fn get(&self) -> &str {
&self.prompt
}
}
#[derive(Completer, Debug, Helper, Hinter, Validator)]
struct PromptHelper {
colored_prompt: String,
}
impl PromptHelper {
fn set_count(&mut self, count: usize, prompt: &mut Prompt) {
prompt.count = count;
prompt.prompt = format!("awesome app ({count})> ");
self.colored_prompt = format!("{} ({count})> ", "awesome app".cyan());
}
}
impl Highlighter for PromptHelper {
fn highlight_prompt<'b, 's: 'b, 'p: 'b>(
&'s self,
_prompt: &'p str,
_default: bool,
) -> Cow<'b, str> {
Cow::Borrowed(&self.colored_prompt)
}
}This allows while let Ok(line) = rl.readline(prompt.get()) {
// Do stuff...
// Conditionally update the prompt.
if let (Some(helper), true) = (rl.helper_mut(), prompt.count != new_count) {
helper.set_count(new_count, &mut prompt);
}
}The cloning isn't that bad. But the workaround isn't, either. I prefer the workaround; it just wasn't obvious to discover. I see that some thought has been put into a trait for providing the two variants of the prompt string. That seems more reasonable. It's basically what I want! I can pass ownership of my own |
This fixes the Windows prompt width calculation by using the same width calculation Unix uses. This code might have originally been in place to work around a Windows shortcoming, but it seems that current versions of Windows calculate width the same way Unix does
Before:
After: