Use floating DIPs throughout the stack #18027
Conversation
lhecker
added
Product-Terminal
| @@ -58,7 +58,14 @@ namespace winrt::Microsoft::Terminal::UI::implementation | |||
| // Misc | |||
| winrt::Windows::UI::Text::FontWeight Converters::DoubleToFontWeight(double value) | |||
| { | |||
| return winrt::Windows::UI::Text::FontWeight{ base::ClampedNumeric<uint16_t>(value) }; | |||
| uint16_t val = 400; | |||
There was a problem hiding this comment.
this looks like an unrelated change - but I see it removes base::ClampedNumeric so I get it
There was a problem hiding this comment.
Yeah, this got in accidentally, as I developed this simultaneously with my other branch that removes the chromium library. I left it in because I felt like it still sort of fits the theme of the PR.
| @@ -872,12 +872,12 @@ void AppHost::_RaiseVisualBell(const winrt::Windows::Foundation::IInspectable&, | |||
| // - delta: the wheel delta that triggered this event. | |||
| // Return Value: | |||
| // - <none> | |||
| void AppHost::_WindowMouseWheeled(const til::point coord, const int32_t delta) | |||
| void AppHost::_WindowMouseWheeled(const winrt::Windows::Foundation::Point coord, const int32_t delta) | |||
| @@ -228,6 +228,16 @@ LRESULT IslandWindow::_OnSizing(const WPARAM wParam, const LPARAM lParam) | |||
|
|
|||
| auto winRect = reinterpret_cast<LPRECT>(lParam); | |||
|
|
|||
| // If we're the quake window, prevent resizing on all sides except the | |||
| // bottom. This also applies to resizing with the Alt+Space menu | |||
| if (IsQuakeWindow() && wParam != WMSZ_BOTTOM) | |||
There was a problem hiding this comment.
Moving this earlier as it's not dependent on any of the calculations? Smart
| // NB: I don't think this is correct because the touch should be in the center of the rect. | ||
| // I suspect the point.Position() would be correct. |
There was a problem hiding this comment.
On GitHub? No, I don't think this is worth keeping track of, since this is just my "reviewer's thoughts". I didn't actually test whether this is a bug. I left a comment in the code because I feel like those comments will give good context if it does turn into a bug one day or maybe during a future refactor like the ones I intend to do.
| // Convert text buffer cursor position to client coordinate position | ||
| // within the window. This point is in _pixels_ |
There was a problem hiding this comment.
I don't understand why this is no longer true
There was a problem hiding this comment.
If you meant why I removed "This point is in pixels": The returned value is now in DIPs instead of pixels.
| const auto charSizeInPixels = CharacterDimensions(); | ||
| const auto htInDips = charSizeInPixels.Height / scale; | ||
| const auto wtInDips = charSizeInPixels.Width / scale; | ||
| const Thickness newThickness{ wtInDips, htInDips, 0, 0 }; | ||
| const Thickness newThickness{ charSizeInPixels.Height, charSizeInPixels.Width, 0, 0 }; |
There was a problem hiding this comment.
Wait up - CharacterDimensions calls FontSizeInDips and should not be in pixels
There was a problem hiding this comment.
I'm always a little confused if XAML wants DIPs or pixels, but I think it wants DIPs in this case, right? Assuming that is correct, I've just changed the variable name and left the rest as-is.
microsoft-github-policy-service
bot
added
Needs-Author-Feedback
I sure hope I didn't break anything!
While
til::mathwas a good idea its convenience led us to use itin the only place where it absolutely must not be used: The UI code.
So, this PR replaces all those
til::points, etc., with floats.Now we use DIPs consistently throughout all layers of the UI code,
except for the UIA area (that would've required too many changes).
Validation Steps Performed
Launch, looks good, no obvious defects, UIA positioning seems ok. ✅