Fix line drawing during IME operations.

[miniksa]

Summary of the Pull Request

Restores proper line drawing during IME operations in conhost

PR Checklist

• Closes Lines are not drawn correctly during IME edit line operations #803
• I work here.
• Tested manually.
• Check the performance of this and see if it's worse-enough to merit a more confusing algorithm. It was worse for the majority case so I scoped it.
• No doc, it should have worked this way.
• Am core contributor.

Detailed Description of the Pull Request / Additional comments

• Changed ConsoleImeInfo::s_ConvertToCells to be less confusing. It's doing about the same thing, but it's way easier to read now and the compiler/linker/optimizer should just be the same.
• Edited Renderer::_PaintBufferOutputHelper to check each attribute for line drawing characters as the right half of a two-col character might have different line drawing characters than the left-half.

Validation Steps Performed

• Manual operation of IME in conhost with Japanese IME.
• Manual operation of IME in conhost with Chinese IME.
• Manual operation of IME in conhost with Chinese (Traditional) IME.
• Manual operation of IME in conhost with and Korean IME. - @leonMSFT says Korean doesn't work this way. But Korean is broken worse in that it's not showing suggestions at all. Filing new bug. Korean IME suggestions don't appear #6227
• Validated against API-filling calls through SetConsoleTextAttribute per @j4james's sample code

[miniksa]

@j4james, this is my proposal.

[j4james]

OK, this is much better than what I had. I ended up having to extend the OutputCellView and TextBufferCellIterator which is the part I hated the most (I need to look at the code again to see what made me think that was necessary). I was also concerned about the renderer having to draw the grid one character at a time, and wondering whether we might be able to optimise that a bit. But having that as a special case for wide characters does at least limit when that happens, so that's not too bad either.

I'm busy geeking out over the upcoming SpaceX launch at the moment, so I'll try and look at this in more detail later, but my initial impression is very positive.

[DHowett]

I hate that this is possible. 😄

#6224 probably needs separate "line drawing runs" ...
that actually makes this way easier.

[miniksa]

I hate that this is possible. 😄

#6224 probably needs separate "line drawing runs" ...
that actually makes this way easier.

Agreed. Each thing should probably be its own pass. If it can be efficient enough.

[j4james]

I've had a chance to run some more tests now, and I'm not sure this is working as well as I had thought. I have a little test case that tries to render every combination of grid flag around a wide character, and that doesn't seem to work with this PR. This is my test code:

#include <windows.h>
#include <stdio.h>

void test(WORD attr)
{
  HANDLE handle = GetStdHandle(STD_OUTPUT_HANDLE);
  SetConsoleTextAttribute(handle, 0x07);
  printf(" ");
  SetConsoleTextAttribute(handle, 0x07 | attr);
  printf("\xE5\xA4\xA7");
  SetConsoleTextAttribute(handle, 0x07);
}

void main()
{
  SetConsoleOutputCP(CP_UTF8);
  printf("\n");
  test(COMMON_LVB_GRID_LVERTICAL);
  test(COMMON_LVB_GRID_HORIZONTAL);
  test(COMMON_LVB_GRID_RVERTICAL);
  test(COMMON_LVB_UNDERSCORE);
  test(COMMON_LVB_UNDERSCORE | COMMON_LVB_GRID_LVERTICAL);
  test(COMMON_LVB_GRID_LVERTICAL | COMMON_LVB_GRID_HORIZONTAL);
  test(COMMON_LVB_GRID_HORIZONTAL | COMMON_LVB_GRID_RVERTICAL);
  test(COMMON_LVB_GRID_RVERTICAL | COMMON_LVB_UNDERSCORE);
  test(COMMON_LVB_UNDERSCORE | COMMON_LVB_GRID_HORIZONTAL);
  test(COMMON_LVB_GRID_LVERTICAL | COMMON_LVB_GRID_RVERTICAL);
  test(COMMON_LVB_GRID_LVERTICAL | COMMON_LVB_GRID_RVERTICAL | COMMON_LVB_UNDERSCORE);
  test(COMMON_LVB_GRID_LVERTICAL | COMMON_LVB_GRID_RVERTICAL | COMMON_LVB_GRID_HORIZONTAL);
  test(COMMON_LVB_GRID_LVERTICAL | COMMON_LVB_GRID_RVERTICAL | COMMON_LVB_GRID_HORIZONTAL | COMMON_LVB_UNDERSCORE);
  printf("\n");
}

This is what I expected it to look like (more or less - not sure about the center grid lines, but I think that's a separate issue):

This is what I'm seeing with this PR:

I'm hoping maybe I've just screwed up the build somehow. Will do some more digging.

[j4james]

One other thing I wanted to mention. It's not essential for this PR, but I thought while you're looking at this grid code, it might be a good time to get rid of COMMON_LVB_GRID_SINGLEFLAG. As far I could make out, it doesn't actually serve any purpose, and that would free up another bit for us to use in the attributes.

[miniksa]

大 is

encoding	code
UTF-8	0x35 0xa4 0xa7
UTF-16	0x59 0x27
Shift-JIS	0x91 0xe5

[miniksa]

Conhost v1 with the scratch code:

Repaired OpenConsole on commit e1c757f:

[miniksa]

One other thing I wanted to mention. It's not essential for this PR, but I thought while you're looking at this grid code, it might be a good time to get rid of COMMON_LVB_GRID_SINGLEFLAG. As far I could make out, it doesn't actually serve any purpose, and that would free up another bit for us to use in the attributes.

I went and read the code for this from conhostv1.

COMMON_LVB_GRID_SINGLEFLAG literally indicated to the IME output-buffer writer code that it needed to remove RVERTICAL from the left half of a two column character and LVERTICAL from the right half of a two column character. You know. Instead of determining that some other way and just stripping it out or only applying it to half the character.

So yeah, I think it can go away and/or be repurposed.

[miniksa]

I believe this is good for merge. It restores the behavior to match v1 conhost in both through-the-API and IME cases.

[zadjii-msft]

Okay, this looks sane, and there's enough comments that I could figure out what was going on here. Good enough for me 😄

[ghost]

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[j4james]

For the record this looked good to me too. Your solution turned out much simpler than what I thought would be required. 👍

[ghost]

🎉Windows Terminal Preview v1.1.1671.0 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Store Download