starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-07-08 18:16:28 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

Handle things above U+FFFF in GDI renderer #14295

New Issue
Closed
opened 2026-01-31 04:06:32 +00:00 by claunia · 10 comments
claunia commented 2026-01-31 04:06:32 +00:00
Owner
Copy Link

Originally created by @alabuzhev on GitHub (Jun 21, 2021).

Why not let Windows draw surrogate pairs? It can do that.

Basically, the comment says everything:
c90de69250/src/renderer/gdi/paint.cpp (L346-L347)

However, handling things above U+FFFF doesn't really require extra effort.

Proposed technical implementation details (optional)

  • Put all characters to the output buffer
  • Set the first width to cluster width and the rest to 0
  • Sit back and relax while Windows does the rest
diff --git a/src/renderer/gdi/paint.cpp b/src/renderer/gdi/paint.cpp
index 9295d3d6..e436b156 100644
--- a/src/renderer/gdi/paint.cpp
+++ b/src/renderer/gdi/paint.cpp
@@ -328,11 +328,13 @@ using namespace Microsoft::Console::Render;
 
         const auto pPolyTextLine = &_pPolyText[_cPolyText];
 
-        auto& polyString = _polyStrings.emplace_back(cchLine, UNICODE_NULL);
+        auto& polyString = _polyStrings.emplace_back();
+        polyString.reserve(cchLine);
 
         COORD const coordFontSize = _GetFontSize();
 
-        auto& polyWidth = _polyWidths.emplace_back(cchLine, 0);
+        auto& polyWidth = _polyWidths.emplace_back();
+        polyWidth.reserve(cchLine);
 
         // Sum up the total widths the entire line/run is expected to take while
         // copying the pixel widths into a structure to direct GDI how many pixels to use per character.
@@ -343,11 +345,11 @@ using namespace Microsoft::Console::Render;
         {
             const auto& cluster = til::at(clusters, i);
 
-            // Our GDI renderer hasn't and isn't going to handle things above U+FFFF or sequences.
-            // So replace anything complicated with a replacement character for drawing purposes.
-            polyString[i] = cluster.GetTextAsSingle();
-            polyWidth[i] = gsl::narrow<int>(cluster.GetColumns()) * coordFontSize.X;
-            cchCharWidths += polyWidth[i];
+            const auto text = cluster.GetText();
+            polyString += text;
+            polyWidth.push_back(gsl::narrow<int>(cluster.GetColumns()) * coordFontSize.X);
+            cchCharWidths += polyWidth.back();
+            polyWidth.append(text.size() - 1, 0);
         }
 
         // Detect and convert for raster font...
@@ -396,7 +398,7 @@ using namespace Microsoft::Console::Render;
         const auto bottomOffset = _currentLineRendition == LineRendition::DoubleHeightTop ? halfHeight : 0;
 
         pPolyTextLine->lpstr = polyString.data();
-        pPolyTextLine->n = gsl::narrow<UINT>(clusters.size());
+        pPolyTextLine->n = gsl::narrow<UINT>(polyString.size());
         pPolyTextLine->x = ptDraw.x;
         pPolyTextLine->y = ptDraw.y;
         pPolyTextLine->uiFlags = ETO_OPAQUE | ETO_CLIPPED;
@@ -436,10 +438,23 @@ using namespace Microsoft::Console::Render;
 
     if (_cPolyText > 0)
     {
+#if 1
+        assert(_cPolyText == 1);
+        for (size_t i = 0; i != _cPolyText; ++i)
+        {
+            const auto& t = _pPolyText[i];
+            if (!ExtTextOutW(_hdcMemoryContext, t.x, t.y, t.uiFlags, &t.rcl, t.lpstr, t.n, t.pdx))
+            {
+                hr = E_FAIL;
+                break;
+            }
+        }
+#else
         if (!PolyTextOutW(_hdcMemoryContext, _pPolyText, (UINT)_cPolyText))
         {
             hr = E_FAIL;
         }
+#endif
 
         _polyStrings.clear();
         _polyWidths.clear();
  • This patch also includes PolyTextOutW -> ExtTextOutW replacement, discussed in #10472 - it's needed to show these glyphs in the first place.

Testing

@echo off
chcp 65001

echo 𠜎𠝹𠱓𠱸𠲖𠳏𠳕𠴕𠵼𠵿𠸎
echo 👨👩👧👦

Save this as a UTF-8 cmd file and run.

Before the change

image

After the change

image

Originally created by @alabuzhev on GitHub (Jun 21, 2021). # Why not let Windows draw surrogate pairs? It can do that. Basically, the comment says everything: https://github.com/microsoft/terminal/blob/c90de692509b074bfde191910d67154cfe389911/src/renderer/gdi/paint.cpp#L346-L347 However, handling things above U+FFFF doesn't really require extra effort. # Proposed technical implementation details (optional) - Put *all* characters to the output buffer - Set the first width to cluster width and the rest to 0 - Sit back and relax while Windows does the rest ```DIFF diff --git a/src/renderer/gdi/paint.cpp b/src/renderer/gdi/paint.cpp index 9295d3d6..e436b156 100644 --- a/src/renderer/gdi/paint.cpp +++ b/src/renderer/gdi/paint.cpp @@ -328,11 +328,13 @@ using namespace Microsoft::Console::Render; const auto pPolyTextLine = &_pPolyText[_cPolyText]; - auto& polyString = _polyStrings.emplace_back(cchLine, UNICODE_NULL); + auto& polyString = _polyStrings.emplace_back(); + polyString.reserve(cchLine); COORD const coordFontSize = _GetFontSize(); - auto& polyWidth = _polyWidths.emplace_back(cchLine, 0); + auto& polyWidth = _polyWidths.emplace_back(); + polyWidth.reserve(cchLine); // Sum up the total widths the entire line/run is expected to take while // copying the pixel widths into a structure to direct GDI how many pixels to use per character. @@ -343,11 +345,11 @@ using namespace Microsoft::Console::Render; { const auto& cluster = til::at(clusters, i); - // Our GDI renderer hasn't and isn't going to handle things above U+FFFF or sequences. - // So replace anything complicated with a replacement character for drawing purposes. - polyString[i] = cluster.GetTextAsSingle(); - polyWidth[i] = gsl::narrow<int>(cluster.GetColumns()) * coordFontSize.X; - cchCharWidths += polyWidth[i]; + const auto text = cluster.GetText(); + polyString += text; + polyWidth.push_back(gsl::narrow<int>(cluster.GetColumns()) * coordFontSize.X); + cchCharWidths += polyWidth.back(); + polyWidth.append(text.size() - 1, 0); } // Detect and convert for raster font... @@ -396,7 +398,7 @@ using namespace Microsoft::Console::Render; const auto bottomOffset = _currentLineRendition == LineRendition::DoubleHeightTop ? halfHeight : 0; pPolyTextLine->lpstr = polyString.data(); - pPolyTextLine->n = gsl::narrow<UINT>(clusters.size()); + pPolyTextLine->n = gsl::narrow<UINT>(polyString.size()); pPolyTextLine->x = ptDraw.x; pPolyTextLine->y = ptDraw.y; pPolyTextLine->uiFlags = ETO_OPAQUE | ETO_CLIPPED; @@ -436,10 +438,23 @@ using namespace Microsoft::Console::Render; if (_cPolyText > 0) { +#if 1 + assert(_cPolyText == 1); + for (size_t i = 0; i != _cPolyText; ++i) + { + const auto& t = _pPolyText[i]; + if (!ExtTextOutW(_hdcMemoryContext, t.x, t.y, t.uiFlags, &t.rcl, t.lpstr, t.n, t.pdx)) + { + hr = E_FAIL; + break; + } + } +#else if (!PolyTextOutW(_hdcMemoryContext, _pPolyText, (UINT)_cPolyText)) { hr = E_FAIL; } +#endif _polyStrings.clear(); _polyWidths.clear(); ``` - This patch also includes PolyTextOutW -> ExtTextOutW replacement, discussed in #10472 - it's needed to show these glyphs in the first place. # Testing ```CMD @echo off chcp 65001 echo 𠜎𠝹𠱓𠱸𠲖𠳏𠳕𠴕𠵼𠵿𠸎 echo 👨👩👧👦 ``` Save this as a UTF-8 cmd file and run. # Before the change ![image](https://user-images.githubusercontent.com/11453922/122832196-ed438880-d2e2-11eb-93dd-931954efedbf.png) # After the change ![image](https://user-images.githubusercontent.com/11453922/122832217-f2a0d300-d2e2-11eb-99f0-e129e5544667.png)
claunia commented 2026-01-31 04:06:33 +00:00
Author
Owner
Copy Link

@lhecker commented on GitHub (Jun 21, 2021):

This looks awesome! But it's also a bit scary for me personally. Your other issue is quite easy for me to understand, but the changes you suggest here would add surrogate support, despite us not properly supporting surrogate pairs anywhere else yet... I'm not 100% sure if your change would work in all corner cases.

But granted, I'm relatively new in these parts of our codebase and other maintainers of this project are a better fit to review the changes you suggest.

@lhecker commented on GitHub (Jun 21, 2021): This looks awesome! But it's also a bit scary for me personally. Your other issue is quite easy for me to understand, but the changes you suggest here would add surrogate support, despite us not properly supporting surrogate pairs anywhere else yet... I'm not 100% sure if your change would work in all corner cases. But granted, I'm relatively new in these parts of our codebase and other maintainers of this project are a better fit to review the changes you suggest.
claunia commented 2026-01-31 04:06:33 +00:00
Author
Owner
Copy Link

@DHowett commented on GitHub (Jun 22, 2021):

Surrogates definitely need a little more love on the text buffer side, for sure. Right now, they (perhaps puzzlingly) only work for double-width glyphs because technically they're being stored directly int eh buffer.

This complicates a number of things:

  1. The clipboard code doesn't know what to do if you manage to select half of one.
    • you end up being able to select partial surrogate pair wide glyphs because the codepoint width estimator is asked about the individual code units and probably says "heck, i don't know"
  2. single-width surrogates will be rendered either broken or by removing an entire column from the line (something we measured as two columns (see 1.x above) takes up one, everything after it is offset)

As opposed to #10472 (which I would love to see a pull request for! for attribution's sake 😄) I'm more hesitant on this one. It will make things look like they're working more correctly than they actually are, and that may get us into more compliance trouble than simply rejecting them would.

@DHowett commented on GitHub (Jun 22, 2021): Surrogates definitely need a little more love on the text buffer side, for sure. Right now, they (perhaps puzzlingly) only work for double-width glyphs because _technically_ they're being stored directly int eh buffer. This complicates a number of things: 1. The clipboard code doesn't know what to do if you manage to select half of one. * you end up being able to select partial surrogate pair wide glyphs because the codepoint width estimator is asked about the individual code units and probably says "heck, i don't know" 2. single-width surrogates will be rendered either broken or by removing an entire column from the line (something we measured as two columns (see 1.x above) takes up one, everything after it is offset) As opposed to #10472 (which I would love to see a pull request for! for attribution's sake :smile:) I'm more hesitant on this one. It will make things look like they're working more correctly than they actually are, and that may get us into more compliance trouble than simply rejecting them would.
claunia commented 2026-01-31 04:06:34 +00:00
Author
Owner
Copy Link

@DHowett commented on GitHub (Jun 22, 2021):

(We're working on surrogate pairs et al over in #8000)

@DHowett commented on GitHub (Jun 22, 2021): (We're working on surrogate pairs et al over in #8000)
claunia commented 2026-01-31 04:06:34 +00:00
Author
Owner
Copy Link

@alabuzhev commented on GitHub (Jun 22, 2021):

despite us not properly supporting surrogate pairs anywhere else yet...

@lhecker I'd say you do support surrogates more or less acceptably already, except for this rendering gap. This already works in Windows Terminal and, philosophically, as a middleman you don't have to do much at all:

  • display what you've been given using the correct number of cells
  • resize the cursor properly when needed.

Everything else is up to the client.

Here's an example of an app working with surrogate pairs in a patched OpenConsole:
image

As you can see, the cursor occupies two cells already, everything is sized, clipped and positioned as expected.

Indeed, it's a quite complex subject and there are known issues with surrogates that logically occupy one cell etc., but I'd say that a partially garbled output is still better than a bunch of perfectly drawn "�".

@alabuzhev commented on GitHub (Jun 22, 2021): > despite us not properly supporting surrogate pairs anywhere else yet... @lhecker I'd say you *do* support surrogates more or less acceptably already, except for this rendering gap. This already works in Windows Terminal and, philosophically, as a middleman you don't have to do much at all: - display what you've been given using the correct number of cells - resize the cursor properly when needed. Everything else is up to the client. Here's an example of an app working with surrogate pairs in a patched OpenConsole: ![image](https://user-images.githubusercontent.com/11453922/122838225-837cac00-d2ed-11eb-8faf-dbeb52f77916.png) As you can see, the cursor occupies two cells already, everything is sized, clipped and positioned as expected. Indeed, it's a quite complex subject and there are [known issues](https://github.com/microsoft/terminal/issues/10287) with surrogates that logically occupy one cell etc., but I'd say that a partially garbled output is still better than a bunch of perfectly drawn "�".
claunia commented 2026-01-31 04:06:34 +00:00
Author
Owner
Copy Link

@alabuzhev commented on GitHub (Jun 22, 2021):

#10472 (which I would love to see a pull request for! for attribution's sake 😄)

No problem, I can create a PR if you're ok with the proposed approach :)

@alabuzhev commented on GitHub (Jun 22, 2021): > #10472 (which I would love to see a pull request for! for attribution's sake 😄) No problem, I can create a PR if you're ok with the proposed approach :)
claunia commented 2026-01-31 04:06:34 +00:00
Author
Owner
Copy Link

@alabuzhev commented on GitHub (Jun 22, 2021):

@DHowett #10478

@alabuzhev commented on GitHub (Jun 22, 2021): @DHowett #10478
claunia commented 2026-01-31 04:06:34 +00:00
Author
Owner
Copy Link

@zadjii-msft commented on GitHub (Jul 7, 2021):

I'm gonna say if this just so happens to work for the DX renderer today, and a trivial change will make it work in GDI, then lets go for it. It might not be technically correct in the buffer, but hey, it's just as technically incorrect in Terminal as in conhost, and Terminal renders it fine.

@miniksa for a veto vote though.

@zadjii-msft commented on GitHub (Jul 7, 2021): I'm gonna say if this just so happens to work for the DX renderer today, and a trivial change will make it work in GDI, then lets go for it. It might not be technically correct in the buffer, but hey, it's just as technically incorrect in Terminal as in conhost, and Terminal renders it fine. @miniksa for a veto vote though.
claunia commented 2026-01-31 04:06:35 +00:00
Author
Owner
Copy Link

@miniksa commented on GitHub (Jul 7, 2021):

I'm good with @alabuzhev's proposed code. As far as I can recall... the substitution was based on it not working with PolyTextOut and with it working fine with ExtTextOut... I don't really have an excuse.

I concur with the argument that it is better than the existing... anything is better than a substitution glyph... and much of the code can handle basic surrogates already. Sure there will be rough edges... but its not worse than today.

And again... I think real characters over replacements isn't going to stop an assembly line. :P

@miniksa commented on GitHub (Jul 7, 2021): I'm good with @alabuzhev's proposed code. As far as I can recall... the substitution was based on it not working with `PolyTextOut` and with it working fine with `ExtTextOut`... I don't really have an excuse. I concur with the argument that it is better than the existing... anything is better than a substitution glyph... and much of the code can handle basic surrogates already. Sure there will be rough edges... but its not worse than today. And again... I think real characters over replacements isn't going to stop an assembly line. :P
claunia commented 2026-01-31 04:06:35 +00:00
Author
Owner
Copy Link

@alabuzhev commented on GitHub (Jul 7, 2021):

Thanks, I'll turn it into a PR then.

@alabuzhev commented on GitHub (Jul 7, 2021): Thanks, I'll turn it into a PR then.
claunia commented 2026-01-31 04:06:35 +00:00
Author
Owner
Copy Link

@ghost commented on GitHub (Jul 14, 2021):

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

Handy links:

@ghost commented on GitHub (Jul 14, 2021): :tada:This issue was addressed in #10580, which has now been successfully released as `Windows Terminal Preview v1.10.1933.0`.:tada: Handy links: * [Release Notes](https://github.com/microsoft/terminal/releases/tag/v1.10.1933.0) * [Store Download](https://www.microsoft.com/store/apps/9n8g5rfz9xk3?cid=storebadge&ocid=badge)
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label Area-Rendering Impact-Compatibility Impact-Correctness Issue-Task Priority-2 Product-Conhost Resolution-Fix-Committed
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#14295
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?