Fix engine size not being changed on DPI changes #12713

lhecker · 2022-03-17T16:44:07Z

Previously we would only call SetWindowSize and TriggerRedrawAll if the
viewport size in characters changed. This commit removes the limitation.
Since the if-condition limiting full redraws is now gone, this commit
moves the responsibility of limiting the calls up the call chain.
With _refreshSizeUnderLock now being a heavier function call
than before, some surrounding code was thus refactored.

PR Checklist

Closes Text display area does not scale correctly #11317
I work here
Tests added/passed

Validation Steps Performed

Test relevant for #11317:

Print text, filling the entire window
Move the window from a 150% scale to a 300% scale monitor
The application works as expected ✅

Regression tests:

Text zoom with Ctrl+Plus/Minus/0 works as before ✅
Resizing a window works as before ✅
No deadlocks, etc. during settings updates ✅

lhecker · 2022-03-17T16:45:08Z

src/cascadia/TerminalControl/ControlCore.cpp

-        // Don't actually resize if viewport dimensions didn't change
-        if (vp.Height() == currentVP.Height() && vp.Width() == currentVP.Width())
-        {
-            return;
-        }


This is the bug fix. The rest are "performance-restoration-refactorizations" to limit calls to _refreshSizeUnderLock.

lhecker · 2022-03-17T16:46:01Z

src/cascadia/TerminalControl/ControlCore.cpp

-        _actualFont = { fontFace, 0, fontWeight.Weight, { 0, fontHeight }, CP_UTF8, false };
-        _actualFontFaceName = { fontFace };
-        _desiredFont = { _actualFont };
+        const auto sizeChanged = _setFontSizeUnderLock(_settings->FontSize());


The previous code was a carbon copy of _setFontSize and I've deduplicated the code.
The new _setFontSizeUnderLock returns true if _refreshSizeUnderLock needs to be called. The old _setFontSize was missing that check, so this is a fix to limit calls to _refreshSizeUnderLock.

lhecker · 2022-03-17T16:50:36Z

src/cascadia/TerminalControl/ControlCore.cpp

+        cx = std::max(cx, _actualFont.GetSize().X);
+        cy = std::max(cy, _actualFont.GetSize().Y);


The previous code would just return early if the size was too small, but then we wouldn't call SetWindowSize... So I turned it into a std::max.

lhecker · 2022-03-17T17:54:45Z

@DHowett A minimal change for 1.12: release-1.12...dev/lhecker/11317-dpi-change-fix-1.12
I tested it and it works.

Gido97081214

Rewrite in C for Linux

miniksa · 2022-03-18T23:34:37Z

src/cascadia/TerminalControl/ControlCore.cpp

-    void ControlCore::_setFontSize(int fontSize)
+    // Return Value:
+    // - Returns true if you need to call _refreshSizeUnderLock().
+    bool ControlCore::_setFontSizeUnderLock(int fontSize)


Oh. So it's not doing it "under lock". It's saying that you should call this only if you're under a lock.

Yeah, there's another function with a similar name pattern.

ghost · 2022-03-24T17:55:52Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Previously we would only call `SetWindowSize` and `TriggerRedrawAll` if the viewport size in characters changed. This commit removes the limitation. Since the if-condition limiting full redraws is now gone, this commit moves the responsibility of limiting the calls up the call chain. With `_refreshSizeUnderLock` now being a heavier function call than before, some surrounding code was thus refactored. ## PR Checklist * [x] Closes #11317 * [x] I work here * [x] Tests added/passed ## Validation Steps Performed Test relevant for #11317: * Print text, filling the entire window * Move the window from a 150% scale to a 300% scale monitor * The application works as expected ✅ Regression tests: * Text zoom with Ctrl+Plus/Minus/0 works as before ✅ * Resizing a window works as before ✅ * No deadlocks, etc. during settings updates ✅ (cherry picked from commit 9fa4169)

ghost · 2022-04-19T19:56:59Z

🎉Windows Terminal v1.12.1098 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2022-04-19T19:59:11Z

🎉Windows Terminal Preview v1.13.1098 has been released which incorporates this pull request.:tada:

Handy links:

Fix engine size not being changed on DPI changes

ee9825f

ghost added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Mar 17, 2022

lhecker commented Mar 17, 2022

View reviewed changes

DHowett requested a review from zadjii-msft March 17, 2022 19:53

Gido97081214 suggested changes Mar 18, 2022

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 18, 2022

Gido97081214 approved these changes Mar 18, 2022

View reviewed changes

bjvanbemmel approved these changes Mar 18, 2022

View reviewed changes

miniksa approved these changes Mar 18, 2022

View reviewed changes

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 18, 2022

DHowett approved these changes Mar 24, 2022

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 24, 2022

ghost merged commit 9fa4169 into main Mar 24, 2022

ghost deleted the dev/lhecker/11317-dpi-change-fix branch March 24, 2022 17:56

zadjii-msft mentioned this pull request Apr 8, 2022

Region invalidation malfunctioning after text buffer has begun to scroll #11214

Closed

ghost mentioned this pull request Apr 19, 2022

Text display area does not scale correctly #11317

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix engine size not being changed on DPI changes #12713

Fix engine size not being changed on DPI changes #12713

lhecker commented Mar 17, 2022 •

edited

Loading

lhecker Mar 17, 2022

lhecker Mar 17, 2022

lhecker Mar 17, 2022

lhecker commented Mar 17, 2022 •

edited

Loading

Gido97081214 left a comment

miniksa Mar 18, 2022

lhecker Mar 18, 2022

ghost commented Mar 24, 2022

ghost commented Apr 19, 2022

ghost commented Apr 19, 2022

		cx = std::max(cx, _actualFont.GetSize().X);
		cy = std::max(cy, _actualFont.GetSize().Y);

Fix engine size not being changed on DPI changes #12713

Fix engine size not being changed on DPI changes #12713

Conversation

lhecker commented Mar 17, 2022 • edited Loading

PR Checklist

Validation Steps Performed

lhecker Mar 17, 2022

Choose a reason for hiding this comment

lhecker Mar 17, 2022

Choose a reason for hiding this comment

lhecker Mar 17, 2022

Choose a reason for hiding this comment

lhecker commented Mar 17, 2022 • edited Loading

Gido97081214 left a comment

Choose a reason for hiding this comment

miniksa Mar 18, 2022

Choose a reason for hiding this comment

lhecker Mar 18, 2022

Choose a reason for hiding this comment

ghost commented Mar 24, 2022

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Apr 19, 2022

ghost commented Apr 19, 2022

lhecker commented Mar 17, 2022 •

edited

Loading

lhecker commented Mar 17, 2022 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.