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.

Sign up for GitHub

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

Jump to bottom

Lines are not drawn correctly during IME edit line operations #803

Closed
miniksa opened this issue May 14, 2019 · 3 comments · Fixed by #6223
Closed

Lines are not drawn correctly during IME edit line operations #803

miniksa opened this issue May 14, 2019 · 3 comments · Fixed by #6223
Assignees
@miniksa
Labels
Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@miniksa
Copy link
Member

miniksa commented May 14, 2019

Ported from MSFT: 20973297

The underscores and the cursor bar aren't being transmitted or drawn correctly when working with the IME. Compare to v1 behavior.

I think there's something wrong with the communication of both the underscores and the positioning of the caret after the refactoring to use the unified output buffer. Investigate.
@miniksa miniksa added Product-Conhost For issues in the Console codebase Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) Issue-Bug It either shouldn't be doing this or needs an investigation. labels May 14, 2019
@miniksa miniksa self-assigned this May 14, 2019
@miniksa miniksa mentioned this issue May 14, 2019
Closed
@j4james
Copy link
Collaborator

j4james commented May 26, 2020

Btw, I was also looking at this at the same time as issue #810, and I managed to get something working as a proof-of-concept, but the solution I had was horrible, and I'm inclined to think that this area of code might need some rearchitecting which is above my pay grade. But if you're desperate for a better-than-nothing solution (assuming you don't already have a plan in mind), I could probably put together a PR with what I've got.

@miniksa
Copy link
Member Author

miniksa commented May 27, 2020

@j4james... Oh, this code is horrible and you're right that it probably needs a rearchitecting, but given that it's the IME in conhost which is an interactive component.... and our roadmap for interactivity is to move it all to Terminal... we're not really likely to rearchitect anything on that side of the fence.

That said, I looked at it this morning and I came up with a solution to the problem. It's hacky too, might even be the same as your proposed idea, but it's probably best if I bless the solution given I'm the one who fubar'd it all up in the first place. I'll send the PR shortly. Let me know how close I was to your idea.

@miniksa miniksa mentioned this issue May 27, 2020
Merged
11 tasks
@ghost ghost added the In-PR This issue has a related PR label May 27, 2020
@DHowett DHowett mentioned this issue May 27, 2020
Closed
8 tasks
@ghost ghost closed this as completed in #6223 Jun 3, 2020
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jun 3, 2020
ghost pushed a commit that referenced this issue Jun 3, 2020
@miniksa
Fix line drawing during IME operations. (#6223)
9b075e9 
## Summary of the Pull Request
Restores proper line drawing during IME operations in `conhost`

## PR Checklist
* [x] Closes #803
* [x] I work here.
* [x] Tested manually.
* [x] 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.
* [x] No doc, it should have worked this way.
* [x] 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
- [x] Manual operation of IME in conhost with Japanese IME.
- [x] Manual operation of IME in conhost with Chinese IME.
- [x] Manual operation of IME in conhost with Chinese (Traditional) IME.
- [x] 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. #6227 
- [x] Validated against API-filling calls through `SetConsoleTextAttribute` per @j4james's sample code
@ghost
Copy link

ghost commented Jun 18, 2020

🎉This issue was addressed in #6223, which has now been successfully released as Windows Terminal Preview v1.1.1671.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@miniksa miniksa

Labels
Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix line drawing during IME operations. microsoft/terminal
2 participants
@j4james @miniksa