Correct for the size of the tabs when calculating our initial window size #4825
Conversation
…he 6px are coming from
| // For whatever reason, there's about 6px of unaccounted-for space | ||
| // in the application. I couldn't tell you where these 6px are | ||
| // coming from, but they need to be included in this math. | ||
| proposedSize.Y += (tabControl.DesiredSize().Height + 6) * scale; |
There was a problem hiding this comment.
I mean I don't like it, but I trust you rooted around forever to try to find it and we'll just correct it later should something change.
miniksa
added
AutoMerge
|
Hello @miniksa! Because this pull request has the Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about 7 hours 45 minutes. No worries though, I will be back when the time is right! 😉 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 (
|
ghost
requested review from
| @@ -417,6 +417,8 @@ SIZE NonClientIslandWindow::GetTotalNonClientExclusiveSize(UINT dpi) const noexc | |||
|
|
|||
| islandFrame.top = -topBorderVisibleHeight; | |||
|
|
|||
| // If we have a titlebar, this is being called after we've initialized, and | |||
| // we can just ask that titlebar how big it wants to be. | |||
| const auto titleBarHeight = _titlebar ? static_cast<LONG>(_titlebar.ActualHeight()) : 0; | |||
There was a problem hiding this comment.
Isn't it adding the title bar twice now: Here and in AppLogic::GetLaunchDimensions? But this works because it's not initialized yet so it adds 0.
BTW the name/description of this method is confusing. It should be something like "get size of everything in the window rect except the content of a tab" I think. Because it's not really the non client area (it includes the fake title bar which is inside the client area).
In fact it seems that because of this confusion there is a bug (not introduced by this PR) where with "showTabsInTitlebar": false, the resize doesn't snap correctly vertically. I just made a bug report for this in #4827.
There was a problem hiding this comment.
Thanks for filing that! You're right, that method name is definitely confusing. IIRC, in the PR that introduced it there was some back and forth on the naming but we couldn't come up with a better name. With this PR, it should be a little easier to at least solve that bug (#4827).
zadjii-msft
added
Area-User Interface
|
Er did we ever figure out if greg904 was right? Probably we should have blocked this |
|
Oh sorry, never replied to that - no, we're shockingly not. Hence the comment there. In startup, |
…size (microsoft#4825) ## Summary of the Pull Request This fixes our calculation for the initial size of the window. WE weren't accounting for the height of the tabs, so the `initialRows` was consistently wrong. ## PR Checklist * [x] Closes microsoft#2061 * [x] I work here * [ ] Tests added/passed * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments For the tabs below the titlebar case, there's 6px (unscaled) of space that I cannot account for. I seriously have no idea where it's coming from. When we end up creating the first `TermControl` after startup, there's an inexplicable `6*scale` difference between the height of the `tabContent` and the `SwapChainPanel`'s size. ## Validation Steps Performed Checked all six of the following cases: * 1.0 DPI scaling, Tabs in Titlebar * 1.25 DPI scaling, Tabs in Titlebar * 1.0 DPI scaling, Tabs NOT in Titlebar, always show tabs * 1.0 DPI scaling, Tabs NOT in Titlebar, DON'T always show tabs * 1.25 DPI scaling, Tabs NOT in Titlebar, always show tabs * 1.25 DPI scaling, Tabs NOT in Titlebar, DON'T always show tabs
Summary of the Pull Request
This fixes our calculation for the initial size of the window. WE weren't accounting for the height of the tabs, so the
initialRowswas consistently wrong.PR Checklist
Detailed Description of the Pull Request / Additional comments
For the tabs below the titlebar case, there's 6px (unscaled) of space that I cannot account for. I seriously have no idea where it's coming from. When we end up creating the first
TermControlafter startup, there's an inexplicable6*scaledifference between the height of thetabContentand theSwapChainPanel's size.Validation Steps Performed
Checked all six of the following cases: