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

Conhost color selection doesn't work correctly with wrapping and DBCS #8572

Closed
j4james opened this issue Dec 13, 2020 · 2 comments · Fixed by #8577
Closed

Conhost color selection doesn't work correctly with wrapping and DBCS #8572

j4james opened this issue Dec 13, 2020 · 2 comments · Fixed by #8577
Labels
Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@j4james
Copy link
Collaborator

j4james commented Dec 13, 2020

Environment

Windows build number: Version 10.0.18363.1198
Windows Terminal version (if applicable):

Steps to reproduce

  1. Open a cmd shell in conhost.
  2. Open the Properties dialog and make sure Enable line wrapping selection is turned on.
  3. Select a range of text wrapped across multiple lines.
  4. Press Ctrl+4 to highlight the selected text in green.
  5. Output some text from double byte character set.
  6. Select a range of those characters starting and ending halfway through a double width character.
  7. Again press Ctrl+4 to highlight the selected text in green.

Expected behavior

When the selection wraps across multiple lines, the green highlighting should also wrap in exactly the same way. When selecting halfway through a double width character, the selection always covers the full width, so I'd expect the green highlighting to work in the same way.

image

Actual behavior

The wrapped selection changes to a block selection when painting the green background. And only half of the start and end characters in the DBCS range are painted green (I should note that this renders differently in the current openconsole build, but is still doesn't match the initial selection).

image

The reason for this behaviour is that the color selection code just uses a simple rect spanning the start and end points of the selection, where typically a selection operation should be using the Selection::GetSelectionRects method to obtain the affected range (which automatically handles the line wrapping option, and makes sure double width characters are fully covered).

The code I'm referring to is here:

ColorSelection(_srSelectionRect, selectionAttr);

And my recommendation would be to change the line above to use the GetSelectionRects method like this:

const auto selectionRects = GetSelectionRects();
for (const auto& selectionRect : selectionRects)
{
    ColorSelection(selectionRect, selectionAttr);
}

I suppose it's possible the current behaviour is deliberate, but I think it's more likely it was just an oversight when conhost was updated to support wrapped selections.

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Dec 13, 2020
@DHowett
Copy link
Member

DHowett commented Dec 13, 2020

Definitely an oversight, probably owing to Color Select being a hidden feature 😄

Legend has it, someone who uses CDB a lot inside the Windows organization added it as well as the “strip leading zeroes on copy” option long before Michael took over the project in ca. 2015.

I think it’s cool; my only beef with it (apart from this bug. Excellent find on your part!) is that it actually manipulates the buffer.

We have some plans to build on the TextBuffer pattern range support + a new interface to pass to the renderer (ICustomTextEffect?) to make this kind of stuff render-only. Eventually, eventually.

@ghost ghost added the In-PR This issue has a related PR label Dec 13, 2020
@ghost ghost closed this as completed in #8577 Dec 14, 2020
@ghost ghost removed the In-PR This issue has a related PR label Dec 14, 2020
ghost pushed a commit that referenced this issue Dec 14, 2020
In conhost there is a keyboard shortcut that applies colors to the
selected range of text, and another shortcut that searches for the
selected text, and applies colors to any matching content. The former
operation doesn't work correctly when the selection is wrapped, and both
have problems when the selected text spans DBCS characters. This PR
attempts to fix those issues.

The problem with the color section was that it applied the color to a
simple rect spanning the start and end points of the selection. I've now
updated it to use the `Selection::GetSelectionRects` method, which
correctly handles a wrapped range of lines, and makes sure that double
width characters are fully covered.

The problem with the "find-and-color" operation was in the way it
obtained the search text from the selected screen cells. Since it
retrieved one cell at a time, and a DBCS character can span two cells,
that resulted in some characters being repeated in the search text. I've
now corrected that code to take the width of the characters into
account.

## Validation Steps Performed
I've manually verified that the test cases described in #8572 and #8574
are now working correctly.

Closes #8572
Closes #8574
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Dec 14, 2020
@ghost
Copy link

ghost commented Jan 28, 2021

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

Handy links:

mpela81 pushed a commit to mpela81/terminal that referenced this issue Jan 28, 2021
In conhost there is a keyboard shortcut that applies colors to the
selected range of text, and another shortcut that searches for the
selected text, and applies colors to any matching content. The former
operation doesn't work correctly when the selection is wrapped, and both
have problems when the selected text spans DBCS characters. This PR
attempts to fix those issues.

The problem with the color section was that it applied the color to a
simple rect spanning the start and end points of the selection. I've now
updated it to use the `Selection::GetSelectionRects` method, which
correctly handles a wrapped range of lines, and makes sure that double
width characters are fully covered.

The problem with the "find-and-color" operation was in the way it
obtained the search text from the selected screen cells. Since it
retrieved one cell at a time, and a DBCS character can span two cells,
that resulted in some characters being repeated in the search text. I've
now corrected that code to take the width of the characters into
account.

## Validation Steps Performed
I've manually verified that the test cases described in microsoft#8572 and microsoft#8574
are now working correctly.

Closes microsoft#8572
Closes microsoft#8574
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants