Fix extra padding when tabs are outside the title bar #19402
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Thanks so much for doing this! Sorry for the delay in reviewing, as well.
It looks like the code formatter has flagged a couple things (also, I think your editor changed line endings in some unexpected places? I'm seeing changed lines that haven't actually changed, and usually that means git is playing line ending tricks on me. :))
microsoft-github-policy-service
bot
added
the
Needs-Author-Feedback
|
@DHowett thanks for the comment, I'll go back and look through to see what needs fixed and update the PR. |
microsoft-github-policy-service
bot
added
Needs-Attention
|
@DHowett I used clang-format so hopefully this 2nd commit fixed the formatting issues from the CI test. Let me know if I need to make further adjustments! |
|
Thanks! |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| // Hide the title bar = off, tab row visible. Account for | ||
| // both the system title bar and the tab strip height. | ||
| static constexpr auto titlebarAndTabBarHeight = 40; | ||
| pixelSize.Height += (titlebarAndTabBarHeight)*scale; | ||
| } | ||
| else | ||
| { | ||
| // Hide the title bar = off, tab row hidden. Only account | ||
| // for the native title bar height. | ||
| static constexpr auto titlebarHeight = 10; | ||
| pixelSize.Height += (titlebarHeight)*scale; | ||
| } | ||
| } | ||
| else if (!_settings.GlobalSettings().ShowTabsInTitlebar()) | ||
| else if (tabRowVisible) | ||
| { | ||
| // Hide the title bar = off, Always show tabs = on. | ||
| static constexpr auto titlebarAndTabBarHeight = 40; | ||
| pixelSize.Height += (titlebarAndTabBarHeight)*scale; | ||
| // Hide the title bar = on, tabs drawn in the title bar. Add | ||
| // the custom tab row height so the client area fits. | ||
| static constexpr auto titlebarHeight = 40; | ||
| pixelSize.Height += (titlebarHeight)*scale; |
There was a problem hiding this comment.
(Sorry to push to your branch before making this comment - the line endings were complicating the review!)
It looks like the code here on R1327 is the same as R1342.
It looks like it might be possible to consolidate them by flipping around the order in which we check:
if (tabRowVisible) {
// it's 40
} else {
if (!tabsInTitlebar) {
// it's 10 apparently
}
}
but I can't currently determine that that is identical. thoughts?
There was a problem hiding this comment.
No worries on the branch push! I'll try and consolidate this part of the code.
There was a problem hiding this comment.
@DHowett just pushed the commit to merge this logic, let me know what you think.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| if (tabRowVisible) | ||
| { | ||
| pixelSize.Height += titlebarAndTabBarHeight * scale; | ||
| } | ||
| else if (!tabsInTitlebar) | ||
| { | ||
| pixelSize.Height += titlebarHeight * scale; | ||
| } |
There was a problem hiding this comment.
@lhecker I think I need a truth table to work through all the logics here - I am starting to doubt my own problem-solving abilities :D
There was a problem hiding this comment.
I don't think this approach is correct in general. It can't be that we have to expose any WinRT properties from class A so that another class B can compute the size needed for A. A should know that by itself. But this (the way sizes are computed) is the framework we've got now and can't be easily changed.
The new boolean logic does make sense to me though.
3 participants
Summary
Rationale
showTabsInTitlebarwas false and only a single tab was openChanges
TerminalPage::IsTabRowVisible()and update_UpdateTabViewand_WindowSizeChangedto consult itTabRowVisibilityReflectsTabCountin the local TabTests suite as a regression guardFixes #19308