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

wpf: color table is a kaleidoscope of misery in DotNet_x64Test configuration #10485

Closed
DHowett opened this issue Jun 22, 2021 · 2 comments · Fixed by #10486
Closed

wpf: color table is a kaleidoscope of misery in DotNet_x64Test configuration #10485

DHowett opened this issue Jun 22, 2021 · 2 comments · Fixed by #10486
Labels
Area-WPFControl Things related to the WPF version of the TermControl 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

@DHowett
Copy link
Member

DHowett commented Jun 22, 2021

Windows Terminal version (or Windows build number)

1.8

Other Software

No response

Steps to reproduce

Run the WpfTerminalTestNetCore project in the DotNet_x64Test configuration. The color table is all offset and looks like a crazy dance party.

Expected Behavior

No response

Actual Behavior

In reality, the struct is being marshalled 4 bytes shorter than it needs to be, and this causes the color table to be offset.
@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 Jun 22, 2021
DHowett added a commit that referenced this issue Jun 22, 2021
@DHowett
wpf: fix the TerminalTheme struct to be marshallable on all platforms
b7ab0f5 
The CursorStyle enum is declared as being of type `uint` on the C# side,
but as `size_t` on the C++ side. There's a C# size_t impostor we could
use, System.UIntPtr, but I don't want to risk changing the public API of
TerminalTheme and I don't know if it can be used as a base type for an
enum value.

Anyway, since we don't have more than four billion cursor types I chose
to narrow the field to a uint32_t and unpack it in TerminalSetTheme.

Fixes #10485
@DHowett DHowett added Area-WPFControl Things related to the WPF version of the TermControl zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. labels Jun 22, 2021
@DHowett DHowett mentioned this issue Jun 22, 2021
Merged
@ghost ghost added the In-PR This issue has a related PR label Jun 22, 2021
@ghost ghost closed this as completed in #10486 Jun 22, 2021
@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 22, 2021
ghost pushed a commit that referenced this issue Jun 22, 2021
@DHowett
wpf: fix the TerminalTheme struct to marshal the same on all platforms (
2770228 
#10486)

The CursorStyle enum is declared as being of type `uint` on the C# side,
but as `size_t` on the C++ side. There's a C# size_t impostor we could
use, System.UIntPtr, but I don't want to risk changing the public API of
TerminalTheme and I don't know if it can be used as a base type for an
enum.

Anyway, since we don't have more than four billion cursor types I chose
to narrow the field to a uint32_t and unpack it in TerminalSetTheme.

Fixes #10485
DHowett added a commit that referenced this issue Jul 7, 2021
@DHowett
wpf: fix the TerminalTheme struct to marshal the same on all platforms (
bf6fd9d 
#10486)

The CursorStyle enum is declared as being of type `uint` on the C# side,
but as `size_t` on the C++ side. There's a C# size_t impostor we could
use, System.UIntPtr, but I don't want to risk changing the public API of
TerminalTheme and I don't know if it can be used as a base type for an
enum.

Anyway, since we don't have more than four billion cursor types I chose
to narrow the field to a uint32_t and unpack it in TerminalSetTheme.

Fixes #10485

(cherry picked from commit 2770228)
DHowett added a commit that referenced this issue Jul 7, 2021
@DHowett
wpf: fix the TerminalTheme struct to marshal the same on all platforms (
caeb5b5 
#10486)

The CursorStyle enum is declared as being of type `uint` on the C# side,
but as `size_t` on the C++ side. There's a C# size_t impostor we could
use, System.UIntPtr, but I don't want to risk changing the public API of
TerminalTheme and I don't know if it can be used as a base type for an
enum.

Anyway, since we don't have more than four billion cursor types I chose
to narrow the field to a uint32_t and unpack it in TerminalSetTheme.

Fixes #10485

(cherry picked from commit 2770228)
@ghost
Copy link

ghost commented Jul 14, 2021

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

Handy links:

@ghost
Copy link

ghost commented Jul 14, 2021

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

Handy links:

@miniksa miniksa removed zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. labels Sep 27, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-WPFControl Things related to the WPF version of the TermControl 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
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

wpf: fix the TerminalTheme struct to marshal the same on all platforms microsoft/terminal
2 participants
@DHowett @miniksa