Repository navigation
Rewrite HwndTerminal to build on ControlInteractivity and -Core - #20812
Dustin L. Howett (DHowett) wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
check-spelling found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
This comment has been minimized.
This comment has been minimized.
Dustin L. Howett (DHowett)
left a comment
There was a problem hiding this comment.
I recommend a...
git diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space (git merge-base origin/dev/duhowett/win7-wpf-termcontrol-squash origin/main) origin/dev/duhowett/win7-wpf-termcontrol-squash
it'll show you what is copied (gray) and what has been rewritten (red, green)
I wonder if I should rename FlatC to HwndTerminal and really make a mess of the diff.
| <RootNamespace>Microsoft.Terminal.Wpf</RootNamespace> | ||
| <AssemblyName>Microsoft.Terminal.Wpf</AssemblyName> | ||
| <UseWpf>true</UseWpf> | ||
| <LangVersion>9.0</LangVersion> |
There was a problem hiding this comment.
yeah I used some fancy operators
| #undef GENERATOR_N | ||
| #undef API_NAME | ||
|
|
||
| __declspec(dllexport) HRESULT __stdcall CreateTerminal(HWND parentHwnd, TerminalDispatcherCallouts dispatcherCallouts, _Out_ void** hwnd, _Out_ PTERM* terminal); |
There was a problem hiding this comment.
THIS FILE IS NOT INTENDED FOR PUBLICATION AND IS AN IMPLEMENTATION DETAIL FOR NOW
The idea is that if we DO add a flat C API, we could generate it using these macros. This just makes my life easier.
dcf463d to
4f3012e
Compare
This comment has been minimized.
This comment has been minimized.
4f3012e to
8f9cd1e
Compare
8f9cd1e to
0591c34
Compare
This comment has been minimized.
This comment has been minimized.
This pull request replaces all of HwndTerminal, which used to be a thin layer on top of TerminalCore.lib, with a new implementation that relies on the Interactivity and Core layers in TerminalControl.lib. We used to spend a lot of time reimplementing interactivity components for HwndTerminal or--if I'm being honest--letting it rot and never giving it the improvements the rest of the application got. Among those improvements: - Automatic scrolling finally works (as of #20710) - Selection works the same way as it does in Terminal proper (starts on the half-cell, uses an exclusive range) - Consistent double- and triple-click behavior without having to reimplement word- and line-wise expansion - Scroll wheel behavior - Shift+Click to suppress VT mouse mode - Ctrl+Scroll zoom This pull request seeks to get HwndTerminal back to where it was (plus the things we naturally get from switching over). In the future, we can more easily add: - title reporting (surprised we didn't have this) - clickable URLs - automatic copy-to-clipboard - right-click menu support - DTTERM window manipulation - shaders Implementation Details ---------------------- The WPF control is backed by a flat "C" API, which takes in a `void*` and does some stuff to it. We call that a `PTERM`. References to "Flat C" in this commit body describe the body of work below WPF. X-macros are used to fill out the definintions of every C API and bridge it to a properly typed C++ member function. Ugh. No more crap where we split the implementation into half C export and half C++ member. ControlCore and ControlInteractivity require something that looks like TerminalSettings. The WPF control only supports the most basic settings, in `struct TerminalTheme`, and so I've added a bridge WinRT type to adapt TerminalTheme to IXxxSettings. Most of the values it returns are our "defaults", and some of them are overridden. Core requires an `ITerminalConnection`, but the WPF API uses a pair of "write output" and "send input" callbacks. Flat C provides an ITerminalConnection that bridges those callbacks. It does not communicate connection state or anything else. Resizing is probably going to be a problem. Core requires a dispatcher queue to ensure that some operations happen on the UI thread. I have stubbed out an implementation of `IDispatcherQueue` that calls a couple function pointers with a context pointer; the WPF code hooks up to that and bridges it into the WPF dispatcher. Yes, the Core's dispatcher queue _is_ the WPF main UI thread. It has to be, otherwise we'll explode the first time we try to update the scroll bar. Prep work was done in #20713. To make the above easier, I removed all use of DispatcherTimer (see #20711). Attempts to use it will almost certainly cause the WPF control to implode. Core needed to be taught a couple tricks to start up targeting an HWND instead of a composition handle. See #20722 for more. Refs #20710 Refs #20711 Refs #20713 Refs #20722 Related to #6999
3bdc1e8 to
86b388c
Compare
|
/cc Javier (@javierdlg) it is finally here |
There was a problem hiding this comment.
Typing literally lags by up to 5s while in the diff view.
GitHub, this is the most awful experience imaginable and it's been going even further downhill for years.
If you were to make a graph of website performance, it would be a vertical line through the floor. That takes some special talent.
| auto scrollbarSize = GetSystemMetrics(SM_CXVSCROLL); | ||
| scrollbarSize = gsl::narrow_cast<decltype(scrollbarSize)>(scrollbarSize * (dpi / 96)); |
There was a problem hiding this comment.
Eh?
And also: scrollbarSize * (dpi / 96) = integer scale only.
| TerminalTriggerResize | ||
| TerminalTriggerResizeWithDimension |
There was a problem hiding this comment.
SCHOCKSCHWERENOT! AN UNSORTED LIST!
| typedef void(__stdcall* PSCROLLCB)(int, int, int); | ||
| typedef void(__stdcall* PWRITECB)(const wchar_t*); |
There was a problem hiding this comment.
Weird names with no param names?
BTW C# does not support strings with lengths huh? (For PWRITECB.)
| bool(__stdcall* pDispatcherTryEnqueueWork)(void*, int, void*); | ||
| bool(__stdcall* pDispatcherHasThreadAccess)(void*); |
There was a problem hiding this comment.
Why p prefix? If anything, it should be fn or pfn, right?
| } TerminalDispatcherCallouts; | ||
|
|
||
| #define TERMINAL_API_TABLE(XX) \ | ||
| XX(SendOutput, LPCWSTR, data) \ |
There was a problem hiding this comment.
I cannot discern an order of these functions, but I think it could be sorted.
| 0, // Mouse | ||
| MouseButtonStateFromWParam(wParam), | ||
| uMsg, | ||
| std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::system_clock::now().time_since_epoch()).count(), |
There was a problem hiding this comment.
We use std::chrono::microseconds throughout ControlCore for timings? What the heck, that completely went by me!
And why system_clock? It should be steady_clock I'm pretty sure. And we should pass std::chrono::steady_clock::time_point or we migrate over to double and seconds, which would be good for the next 2000 years.
| 0, | ||
| term_window_class, | ||
| nullptr, | ||
| WS_CHILD | WS_CLIPCHILDREN | WS_CLIPSIBLINGS | WS_VISIBLE, |
There was a problem hiding this comment.
Why all these flags? We're a swap chain so we aren't subject to GDI in the first place, aren't we?
| wil::assign_to_opt_param(dimensions, til::size{ | ||
| static_cast<til::CoordType>(width / fontSizeInPx.Width), | ||
| static_cast<til::CoordType>(height / fontSizeInPx.Height), | ||
| }); |
There was a problem hiding this comment.
Odd indentation.
| auto concatenated = std::accumulate(std::begin(strings), std::end(strings), std::wstring{}, [](auto&& l, auto&& r) { | ||
| return l + r; | ||
| }); |
There was a problem hiding this comment.
That's an odd way to copy a hstring into a wstring. :D
It's also not needed, because make_cotaskmem_string_nothrow takes a hstring.c_str() anyway.
| bool _initialized{ false }; | ||
| bool _focused{ false }; |
There was a problem hiding this comment.
Unfortunate struct order?
This pull request replaces all of HwndTerminal, which used to be a thin layer on top of TerminalCore.lib, with a new implementation that relies on the Interactivity and Core layers in TerminalControl.lib.
We used to spend a lot of time reimplementing interactivity components for HwndTerminal or--if I'm being honest--letting it rot and never giving it the improvements the rest of the application got.
Among those improvements:
This pull request seeks to get HwndTerminal back to where it was (plus the things we naturally get from switching over). In the future, we can more easily add:
Implementation Details
The WPF control is backed by a flat "C" API, which takes in a
void*and does some stuff to it. We call that aPTERM. References to "Flat C" in this commit body describe the body of work below WPF.X-macros are used to fill out the definintions of every C API and bridge it to a properly typed C++ member function. Ugh. No more crap where we split the implementation into half C export and half C++ member.
ControlCore and ControlInteractivity require something that looks like TerminalSettings. The WPF control only supports the most basic settings, in
struct TerminalTheme, and so I've added a bridge WinRT type to adapt TerminalTheme toIXxxSettings. Most of the values it returns are our "defaults", and some of them are overridden.
Core requires an
ITerminalConnection, but the WPF API uses a pair of "write output" and "send input" callbacks. Flat C provides an ITerminalConnection that bridges those callbacks. It does not communicate connection state or anythingelse. Resizing is probably going to be a problem.
Core requires a dispatcher queue to ensure that some operations happen on the UI thread. I have stubbed out an implementation of
IDispatcherQueuethat calls a couple function pointers with a context pointer; the WPF code hooks up to thatand bridges it into the WPF dispatcher. Yes, the Core's dispatcher queue is the WPF main UI thread. It has to be, otherwise we'll explode the first time we try to update the scroll bar. Prep work was done in #20713.
To make the above easier, I removed all use of DispatcherTimer (see #20711). Attempts to use it will almost certainly cause the WPF control to implode.
Core needed to be taught a couple tricks to start up targeting an HWND instead of a composition handle. See #20722 for more.
Refs #20710
Refs #20711
Refs #20713
Refs #20722
Required #20721
Refs #20745
Related to #6999