Skip to content

Fix engine size not being changed on DPI changes - #12713

Merged
1 commit merged into
mainfrom
dev/lhecker/11317-dpi-change-fix
Mar 24, 2022
Merged

1 commit merged into
mainfrom
dev/lhecker/11317-dpi-change-fix

Conversation

@lhecker

@lhecker Leonard Hecker (lhecker) commented Mar 17, 2022 •

Copy link
Copy Markdown
Member

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

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 ✅

@ghost 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
Comment on lines -865 to -869
// Don't actually resize if viewport dimensions didn't change
if (vp.Height() == currentVP.Height() && vp.Width() == currentVP.Width())
{
return;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +821 to +822
cx = std::max(cx, _actualFont.GetSize().X);
cy = std::max(cy, _actualFont.GetSize().Y);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Leonard Hecker (lhecker) commented Mar 17, 2022 •

Copy link
Copy Markdown
Member Author

Dustin L. Howett (@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 Gido97081214 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrite in C for Linux

@ghost 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
void ControlCore::_setFontSize(int fontSize)
// Return Value:
// - Returns true if you need to call _refreshSizeUnderLock().
bool ControlCore::_setFontSizeUnderLock(int fontSize)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ghost 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 Dustin L. Howett (DHowett) added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 24, 2022
@ghost

Copy link
Copy Markdown

Hello Dustin L. Howett (@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.

@ghost
ghost merged commit 9fa4169 into main Mar 24, 2022
@ghost
ghost deleted the dev/lhecker/11317-dpi-change-fix branch March 24, 2022 17:56
Dustin L. Howett (DHowett) pushed a commit that referenced this pull request Mar 28, 2022
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

Copy link
Copy Markdown

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

Handy links:

@ghost

Copy link
Copy Markdown

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

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Text display area does not scale correctly

5 participants