A margin variable is being constrained by dpi scale X instead of dpi Scale Y

[ScriptKat]

Environment

Major Minor Build Revision

---

10 0 19041 546

Steps to reproduce

I noticed an issue while reading through: Microsoft.Terminal.Wpf.TerminalControl.CalculateMargins(). Specifically, on line 194 in the project. In this example, it is the line - height = controlSize.Height - (this.TerminalRendererSize.Height / dpiScale.DpiScaleX); :

private Thickness CalculateMargins(Size controlSize = default)
{
var dpiScale = VisualTreeHelper.GetDpi(this);
double width = 0, height = 0;

        if (controlSize == default)
        {
            controlSize = new Size()
            {
                Width = this.terminalUserControl.ActualWidth,
                Height = this.terminalUserControl.ActualHeight,
            };
        }

        // During initialization, the terminal renderer size will be 0 and the terminal renderer
        // draws on all available space. Therefore no margins are needed until resized.
        if (this.TerminalRendererSize.Width != 0)
        {
            width = controlSize.Width - (this.TerminalRendererSize.Width / dpiScale.DpiScaleX);
        }

        if (this.TerminalRendererSize.Height != 0)
        {
            height = controlSize.Height - (this.TerminalRendererSize.Height / dpiScale.DpiScaleX);
        }

        width -= this.scrollbar.ActualWidth;

        // Prevent negative margin size.
        width = width < 0 ? 0 : width;
        height = height < 0 ? 0 : height;

        return new Thickness(0, 0, width, height);
    } 

Expected behavior

Shouldn't height be constrained by dpiScale.DpiScaleY and not dpiScale.DpiScaleX?

Actual behavior

Height appears to be associated with dpiScale.DpiScaleX instead of dpiScale.DpiScaleY.

I'm happy to help change the variable if others agree that this is an issue.

[DHowett]

Good catch! I’m not aware of any situations in which Windows will scale the display differently in each axis, but it would be good for us to get this right regardless. Feel free to submit a PR!

[ScriptKat]

@DHowett Thanks! I opened a PR, but noticed four of the automated checks failed.. i'm new to contributing to this repo, so am not sure why that may have happened? I submitted another very small PR of similar scope about 40 minutes later and everything passed, so am not sure what in this PR specifically caused some of the checks to fail.. or does the CICD system have issues intermittently?I notice looking through the logs that the checks were failing at the step to restore packages.. is there any way to re-trigger it if it fails outside of updating the PR?

[ghost]

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

Handy links:

• Release Notes
• Store Download

[ghost]

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

Handy links:

• Release Notes
• Store Download