Repository: microsoft/terminal (language: cpp)

Issue:
Inverse cursor color logic is (sometimes) broken
<!-- 
🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨

I ACKNOWLEDGE THE FOLLOWING BEFORE PROCEEDING:
1. If I delete this entire template and go my own path, the core team may close my issue without further explanation or engagement.
2. If I list multiple bugs/concerns in this one issue, the core team may close my issue without further explanation or engagement.
3. If I write an issue that has many duplicates, the core team may close my issue without further explanation or engagement (and without necessarily spending time to find the exact duplicate ID number).
4. If I leave the title incomplete when filing the issue, the core team may close my issue without further explanation or engagement.
5. If I file something completely blank in the body, the core team may close my issue without further explanation or engagement.

All good? Then proceed!
-->

<!--
This bug tracker is monitored by Windows Terminal development team and other technical folks.

**Important: When reporting BSODs or security issues, DO NOT attach memory dumps, logs, or traces to Github issues**.
Instead, send dumps/traces to secure@microsoft.com, referencing this GitHub issue.

If this is an application crash, please also provide a Feedback Hub submission link so we can find your diagnostic data on the backend. Use the category "Apps > Windows Terminal (Preview)" and choose "Share My Feedback" after submission to get the link.

Please use this form and describe your issue, concisely but precisely, with as much detail as possible.

-->

# Environment

```none
Windows build number:
Microsoft Windows [Version 10.0.18362.449]

Windows Terminal version (if applicable):
No.

Any other software?
No.
```

# Steps to reproduce
1. Open cmd.exe.
1. Go to window properties, "Colors" page.
2. Set "Screen Background" value to exactly `128 128 128`.
<!-- A description of how to trigger this bug. -->

# Expected behavior
Cursor is visible no matter what background color is.

<!-- A description of what you're expecting, possibly containing screenshots or reference material. -->

# Actual behavior
Now you see it, now you don't.

<!-- What's actually happening? -->

# 
Current "inverse" logic is too straightforward - it literally takes the inverse of the background, and for `128 128 128` the inverse is... `127 127 127`, which is basically the same thing. Oops.

Note: **Legacy console handles this corner case properly, as well as all Windows versions prior to 10**.

Also, legacy console inversion algorithm is not exactly "inverse" - the colors are slightly different (and subjectively more pleasant). Is it possible to get this classic inversion in the modern console please?

This issue affects a text editor app that uses console color `8` (`128 128 128` in classic color scheme) to highlight various parts of text (e.g. current line). Sudden cursor disappearing is confusing and annoying.

Example:

![image](https://user-images.githubusercontent.com/11453922/69289150-871cee00-0bf3-11ea-8893-219d107a5ca5.png)



Discussion / hints:
I've experimented a bit and managed to get exactly the same cursor colors as in the Classic Console:

GDI:
```DIFF
diff --git a/src/renderer/gdi/paint.cpp b/src/renderer/gdi/paint.cpp
index 2f067fc5a..a9f9c77d1 100644
--- a/src/renderer/gdi/paint.cpp
+++ b/src/renderer/gdi/paint.cpp
@@ -733,7 +733,11 @@ bool GdiEngine::FontHasWesternScript(HDC hdc)
 
         for (auto r : cursorInvertRects)
         {
-            RETURN_HR_IF(E_FAIL, !(InvertRect(_hdcMemoryContext, &r)));
+            // Make the inverted cursor slightly darker to keep it distinct in all cases (see gh-3647).
+            const auto PrevObject = SelectObject(_hdcMemoryContext, GetStockObject(LTGRAY_BRUSH));
+            const auto Result = PatBlt(_hdcMemoryContext, r.left, r.top, r.right - r.left, r.bottom - r.top, PATINVERT);
+            SelectObject(_hdcMemoryContext, PrevObject);
+            RETURN_HR_IF(E_FAIL, !Result);
         }
     }
 
```
DX:
```DIFF 
diff --git a/src/renderer/dx/CustomTextRenderer.cpp b/src/renderer/dx/CustomTextRenderer.cpp
index f3a894a43..7ad00c923 100644
--- a/src/renderer/dx/CustomTextRenderer.cpp
+++ b/src/renderer/dx/CustomTextRenderer.cpp
@@ -395,8 +395,9 @@ try
         if (firstPass)
         {
             // Draw a backplate behind the cursor in the *background* color so that we can invert it later.
-            // We're going to draw the exact same color as the background behind the cursor
-            const til::color color{ drawingContext.backgroundBrush->GetColor() };
+            // We're going to draw a slightly lighter color than the background behind the cursor,
+            // so that after the inversion it will be slightly darker and distinct in all cases (see gh-3647).
+            const til::color color{ til::color{ drawingContext.backgroundBrush->GetColor() } ^ RGB(63, 63, 63) };
             RETURN_IF_FAILED(d2dContext->CreateSolidColorBrush(color.with_alpha(255),
                                                                &brush));
         }

```
Atlas:
```DIFF
diff --git a/src/renderer/atlas/shader_ps.hlsl b/src/renderer/atlas/shader_ps.hlsl
index 789cd98b0..fce0b9517 100644
--- a/src/renderer/atlas/shader_ps.hlsl
+++ b/src/renderer/atlas/shader_ps.hlsl
@@ -168,6 +168,8 @@ float4 main(float4 pos: SV_Position): SV_Target
         [flatten] if (cursorColor == INVALID_COLOR && glyphs[cellPos].a != 0)
         {
             color = float4(1 - color.rgb, 1);
+            // Make the inverted cursor slightly darker to keep it distinct in all cases (see gh-3647).
+            color = abs(color - 0.25);
         }
     }
```

#### Before:
![image](https://user-images.githubusercontent.com/11453922/179037063-ad319709-3c9a-489a-a3d7-0dc57ce7163a.png)
^^^ - this!

#### After:
![image](https://user-images.githubusercontent.com/11453922/179037092-1aa500c9-7a74-413f-9093-6f89dc407fc4.png)

The logic is pretty simple and equivalent to making the inverted color 25% darker:
```C++
uint32_t Color = current_cell_color();
uint32_t InvertedColor = ~Color & 0x00FFFFFF;
uint32_t CursorColor = InvertedColor ^ 0x003F3F3F;
```


I've added DX and Atlas renderer changes to the previous comment.
Could you have a look please?
Would a PR be welcomed?
Lemme /cc @lhecker cause I think he's got some thoughts on the matter. 
Correct me if I'm wrong but the XOR results in this:
![image](https://user-images.githubusercontent.com/2256941/181643056-95a6a2e1-2f52-4d71-bd62-972c21269958.png)

But the solution for AtlasEngine is unlike that since it always subtracts 0.25 brightness so you get an intersection between input and output for `RGB(96, 96, 96)`.
So in case of AtlasEngine we could get a bit more creative... For instance this:
```hlsl
bool odd = (cellPos.x ^ cellPos.y) & 1;
float inv = odd ? 0.8 : 1.0;
color = float4(saturate(inv - color.rgb), 1);
```

This, with the help of `odd`, creates a checkerboard image (background 127 gray):
![image](https://user-images.githubusercontent.com/2256941/181647187-e5da22fe-197b-445f-8127-832ef0b94581.png)

Or default "black" (RGB = 12) background:
![image](https://user-images.githubusercontent.com/2256941/181647357-9e9f04f2-ef71-45d7-bc2d-70480b8edd31.png)

We can also use the alpha channel of the cursor texture for inversion:
```hlsl
color = float4(saturate(glyphs[cellPos].a - color.rgb), 1);
```

...and then draw a only a semi-transparent content in the glyph texture in `AtlasEngine::_drawCursor` which results in the same darkening effect.

In general I'm in favor of such a change, but I'm unsure what the best approach for the visuals is.
@lhecker thanks, the XOR plot is correct, or, alternatively:

![image](https://user-images.githubusercontent.com/11453922/181658072-ba41650e-cf3a-49b1-ad61-9df0ecf597bf.png)

> the solution for AtlasEngine is unlike that

Mea culpa 🤦
It was supposed to be the same, but for some reason I decided to "simplify" it...
Something like this should work (and can probably be improved further):
```DIFF
diff --git a/src/renderer/atlas/shader_ps.hlsl b/src/renderer/atlas/shader_ps.hlsl
index 789cd98b0..320f2895e 100644
--- a/src/renderer/atlas/shader_ps.hlsl
+++ b/src/renderer/atlas/shader_ps.hlsl
@@ -168,6 +168,8 @@ float4 main(float4 pos: SV_Position): SV_Target
         [flatten] if (cursorColor == INVALID_COLOR && glyphs[cellPos].a != 0)
         {
             color = float4(1 - color.rgb, 1);
+            float3 shifted = ((uint3(color.rgb * 255) ^ 0x3f) & 0xff) / 255.0;
+            color = float4(shifted.rgb, 1);
         }
     }
```

> I'm unsure what the best approach for the visuals is.

I'm all for creativity and fancy visuals, but it would be nice to at least have a uniform, always readable and already tested solution everywhere first, even if not the most creative one.
If you'd like to submit a PR for this - go for it!

Fix patch:
diff --git a/.github/actions/spelling/allow/apis.txt b/.github/actions/spelling/allow/apis.txt
index 4a8161c6aac..6717224e675 100644
--- a/.github/actions/spelling/allow/apis.txt
+++ b/.github/actions/spelling/allow/apis.txt
@@ -103,6 +103,7 @@ lround
 Lsa
 lsass
 LSHIFT
+LTGRAY
 MAINWINDOW
 memchr
 memicmp
@@ -146,6 +147,7 @@ OUTLINETEXTMETRICW
 overridable
 PACL
 PAGESCROLL
+PATINVERT
 PEXPLICIT
 PICKFOLDERS
 pmr
diff --git a/src/renderer/atlas/shader_ps.hlsl b/src/renderer/atlas/shader_ps.hlsl
index 87c77f0526a..867bcccf2fd 100644
--- a/src/renderer/atlas/shader_ps.hlsl
+++ b/src/renderer/atlas/shader_ps.hlsl
@@ -101,13 +101,27 @@ float4 main(float4 pos: SV_Position): SV_Target
     // Colored cursors are drawn "in between" the background color and the text of a cell.
     [branch] if (cell.flags & CellFlags_Cursor)
     {
-        [flatten] if (cursorColor != INVALID_COLOR)
+        [branch] if (cursorColor != INVALID_COLOR)
         {
             // The cursor texture is stored at the top-left-most glyph cell.
             // Cursor pixels are either entirely transparent or opaque.
             // --> We can just use .a as a mask to flip cursor pixels on or off.
             color = alphaBlendPremultiplied(color, decodeRGBA(cursorColor) * glyphs[cellPos].a);
         }
+        else if (glyphs[cellPos].a != 0)
+        {
+            // Make sure the cursor is always readable (see gh-3647)
+            // If we imagine the two colors to be in 0-255 instead of 0-1,
+            // this effectively XORs them with 63. This avoids a situation
+            // where a gray background color (0.5) gets inverted to the
+            // same gray making the cursor invisible.
+            float2x4 colors = { color, fg };
+            float2x4 ip; // integral part
+            float2x4 frac = modf(colors * (255.0f / 64.0f), ip);
+            colors = (3.0f - ip + frac) * (64.0f / 255.0f);
+            color = float4(colors[0].rgb, 1);
+            fg = float4(colors[1].rgb, 1);
+        }
     }
 
     // Layer 2:
@@ -146,7 +160,7 @@ float4 main(float4 pos: SV_Position): SV_Target
         // What a nice coincidence that we have exactly 8 flags to handle right now!
         // `mask` will mask away any positive results from checks we don't want.
         // (I.e. even if we're in an underline, it doesn't matter if we don't want an underline.)
-        bool4x2 mask = {
+        bool2x4 mask = {
             cell.flags & CellFlags_BorderLeft,
             cell.flags & CellFlags_BorderTop,
             cell.flags & CellFlags_BorderRight,
@@ -159,7 +173,7 @@ float4 main(float4 pos: SV_Position): SV_Target
         // The following <lineWidth checks rely on underflow turning the
         // uint into a way larger number than any reasonable lineWidth.
         // That way we don't need to write `y >= lo && y < hi`.
-        bool4x2 checks = {
+        bool2x4 checks = {
             // These 2 expand to 4 bools, because cellPos is a
             // uint2 vector which results in a bool2 result each.
             cellPos < thinLineWidth,
@@ -176,16 +190,6 @@ float4 main(float4 pos: SV_Position): SV_Target
         }
     }
 
-    // Layer 3 (optional):
-    // Uncolored cursors are used as a mask that inverts the cells color.
-    [branch] if (cell.flags & CellFlags_Cursor)
-    {
-        [flatten] if (cursorColor == INVALID_COLOR && glyphs[cellPos].a != 0)
-        {
-            color = float4(1 - color.rgb, 1);
-        }
-    }
-
     // Layer 4:
     // The current selection is drawn semi-transparent on top.
     [branch] if (cell.flags & CellFlags_Selected)
diff --git a/src/renderer/dx/CustomTextRenderer.cpp b/src/renderer/dx/CustomTextRenderer.cpp
index f3a894a4396..f316c96d51b 100644
--- a/src/renderer/dx/CustomTextRenderer.cpp
+++ b/src/renderer/dx/CustomTextRenderer.cpp
@@ -395,8 +395,8 @@ try
         if (firstPass)
         {
             // Draw a backplate behind the cursor in the *background* color so that we can invert it later.
-            // We're going to draw the exact same color as the background behind the cursor
-            const til::color color{ drawingContext.backgroundBrush->GetColor() };
+            // Make sure the cursor is always readable (see gh-3647)
+            const til::color color{ til::color{ drawingContext.backgroundBrush->GetColor() } ^ RGB(63, 63, 63) };
             RETURN_IF_FAILED(d2dContext->CreateSolidColorBrush(color.with_alpha(255),
                                                                &brush));
         }
diff --git a/src/renderer/gdi/paint.cpp b/src/renderer/gdi/paint.cpp
index 2f067fc5a1a..47437502d37 100644
--- a/src/renderer/gdi/paint.cpp
+++ b/src/renderer/gdi/paint.cpp
@@ -733,7 +733,11 @@ bool GdiEngine::FontHasWesternScript(HDC hdc)
 
         for (auto r : cursorInvertRects)
         {
-            RETURN_HR_IF(E_FAIL, !(InvertRect(_hdcMemoryContext, &r)));
+            // Make sure the cursor is always readable (see gh-3647)
+            const auto PrevObject = SelectObject(_hdcMemoryContext, GetStockObject(LTGRAY_BRUSH));
+            const auto Result = PatBlt(_hdcMemoryContext, r.left, r.top, r.right - r.left, r.bottom - r.top, PATINVERT);
+            SelectObject(_hdcMemoryContext, PrevObject);
+            RETURN_HR_IF(E_FAIL, !Result);
         }
     }
 


Test patch: