Allow `ThrottledFunc` to work on different types of dispatcher by zadjii-msft · Pull Request #10187 · microsoft/terminal

zadjii-msft · 2021-05-25T21:55:04Z

⚠️ targets #10051

Summary of the Pull Request

This updates our ThrottledFuncs to take a dispatcher parameter. This means that we can use the Windows::UI::Core::CoreDispatcher in the TermControl, where there's always a CoreDispatcher, and use a Windows::System::DispatcherQueue in ControlCore/ControlInteractivity. When running in-proc, these are always the same thing. However, out-of-proc, the core needs a dispatcher queue that's not tied to a UI thread (because the content proces doesn't have a UI thread!).

This lets us get rid of the output event, because we don't need to bubble that event out to the TermControl to let it throttle that update anymore.

References

PR Checklist

This is a part of Add support for tab tearoff and tab merge #1256
I work here
[n/a] Tests added/passed
[n/a] Requires documentation to be updated

Detailed Description of the Pull Request / Additional comments

Fortunately, winrt::resume_foreground works the same on both a CoreDispatcher and a DispatcherQueue, so this wasn't too hard!

Validation Steps Performed

This was validated in dev/migrie/oop/the-whole-thing (or dev/migrie/oop/connection-factory, I forget which), and I made sure that it worked both in-proc and x-proc. Not only that, it wasn't any slower!This reverts commit 04b751f.

Now we have an InteractivityAutomationPeer, which acts like the implementation for TermControlAutomationPeer. TermControlAutomationPeer is still needed though, because it implements FrameworkElementAutomationPeer, which is needed to hook up the AP to the actual UIA tree. So we split up the work into two halves, the bit that should be done by the core (the ITextProvider, the IControlAccessibilityInfo), vs the stuff that needs to be in the control (FrameworkElementAutomationPeer). IUiaEventDispatcher should probably be in the control as well, but that's a problem for future us.

… remain.

This reverts commit c113b65. That commit reverted 30b8335

…ractivity-projection-000

…ity-projection-000 # Conflicts: # src/cascadia/TerminalControl/ControlInteractivity.cpp # src/cascadia/TerminalControl/ControlInteractivity.h # src/cascadia/TerminalControl/TermControl.cpp # src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp

…s are still wrong

…dcomp-handle

…ractivity-projection-000

…ity-projection-000

…led-funcs

src/cascadia/TerminalControl/ControlCore.h

lhecker · 2021-07-19T18:53:17Z

src/cascadia/TerminalControl/ControlCore.cpp

+        // When the buffer's cursor moves, start the throttled func to
+        // eventually dispatch a CursorPositionChanged event.
+        _tsfTryRedrawCanvas->Run();


So if I understand it correctly, this starts a throttled func to throw an CursorPositionChanged event every 100ms, right? But TermControl also got a _tsfTryRedrawCanvas which only triggers the update every 100ms. Now it's only updating every 200-300ms (depending on the race between the two timers). Shouldn't we remove either of the two throttled funcs then?

Okay so you have a point here. I'm gonna keep the throttling on the core side, not the control side. The Core will want to throttle how often it hops across the COM boundary to tell the UI to update. The UI doesn't care so much - it should just update whenever the Core tells it to.

…led-funcs

…xpected

…rolUnitTests are run all together, they're flaky and sometimes crash. This brings sadness

…ork as expected" This reverts commit f9b4216.

zadjii-msft · 2021-08-09T13:18:44Z

note to self:

Unittests are now failing because ControlCore::_terminalScrollPositionChanged triggers a throttledfunc to fire the ScrollPositionChanged callback. In real life, this is good - the user won't notice the delay. In the tests, this is bad - we want the callback firing instantly so we can check the value matches what we'd expect. Might need to introduce a delay in the tests to account for this. Unless I can think of a more clever solution.

ghost · 2021-08-09T15:21:44Z

Hello @zadjii-msft!

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.

This is on me. When I got rid of the `_updatePatternLocations` `ThrottledFunc` in the `TermControl`, I didn't add a matching call to `_updatePatternLocations->Run()` in this method. In #9820, in `TermControl::_ScrollPositionChanged`, there was still a call to `_updatePatternLocations->Run();`. (TermControl.cpp:1655 on the right) https://github.com/microsoft/terminal/pull/9820/files#diff-c10bb023995e88dac6c1d786129284c454c2df739ea547ce462129dc86dc2697R1654 #10051 didn't change this In #10187 I moved the `_updatePatternLocations` throttled func from termcontrol to controlcore. Places it existed before: * [x] `TermControl::_coreReceivedOutput`: already matched by ControlCore::_connectionOutputHandler * [x] `TermControl::_ScrollbarChangeHandler` -> added in c20eb9d * [x] `TermControl::_ScrollPositionChanged` -> `ControlCore::_terminalScrollPositionChanged` ## Validation Steps Performed Print a URL, scroll the wheel: it still works. Closes #11055

This is on me. When I got rid of the `_updatePatternLocations` `ThrottledFunc` in the `TermControl`, I didn't add a matching call to `_updatePatternLocations->Run()` in this method. In #9820, in `TermControl::_ScrollPositionChanged`, there was still a call to `_updatePatternLocations->Run();`. (TermControl.cpp:1655 on the right) https://github.com/microsoft/terminal/pull/9820/files#diff-c10bb023995e88dac6c1d786129284c454c2df739ea547ce462129dc86dc2697R1654 #10051 didn't change this In #10187 I moved the `_updatePatternLocations` throttled func from termcontrol to controlcore. Places it existed before: * [x] `TermControl::_coreReceivedOutput`: already matched by ControlCore::_connectionOutputHandler * [x] `TermControl::_ScrollbarChangeHandler` -> added in c20eb9d * [x] `TermControl::_ScrollPositionChanged` -> `ControlCore::_terminalScrollPositionChanged` ## Validation Steps Performed Print a URL, scroll the wheel: it still works. Closes #11055 (cherry picked from commit 7423734)

zadjii-msft added 30 commits April 29, 2021 12:03

Only access ControlInteractivity through the projection

e4478ee

Only 24 more errors to go

400b35f

some comments that should have been in the previous commit. 16 errors…

4578c13

… remain.

GetHoveredCell/UpdateHoveredCell. 12 remain.

d8cc047

Revert "Revert "Use DComp surface handle for Swap Chain management.""

21a97b6

This reverts commit c113b65. That commit reverted 30b8335

some minor cleanup

2e861d8

Merge branch 'dev/migrie/f/oop/use-dcomp-handle' into dev/migrie/inte…

e20caae

…ractivity-projection-000

Use the HANDLE for the swapchainin the core projection

d667f3b

it builds and runs so I guess it's done now

62cbf30

Do the delayload dance

445bdf6

Merge branch 'dev/migrie/f/oop/use-dcomp-handle' into dev/migrie/inte…

1376721

…ractivity-projection-000

What a ridiculous hack

a1ce7cb

fix accessibility

03fb764

I want to get the padding out of interactivity, but the control bound…

6e031e5

…s are still wrong

now this works!

47410c9

Some cleanup

0869456

a bit more cleanup

dace0c4

Merge remote-tracking branch 'origin/main' into dev/migrie/f/oop/use-…

12b78f5

…dcomp-handle

Merge branch 'dev/migrie/f/oop/use-dcomp-handle' into dev/migrie/inte…

e340fe9

…ractivity-projection-000

god I knew I left these behind

b7b23a8

Merge remote-tracking branch 'origin/main' into dev/migrie/interactiv…

086710a

…ity-projection-000

fix the tests

21a6370

This fixes nvda, so nowwe just have to polish

4efdf39

minor typo cleanup

59e911d

some consts

6f07004

this is almost exclusively nits

ff3b808

simple nits

559d1de

this one's easy too

804a114

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Jul 19, 2021

ghost requested review from DHowett, PankajBhojwani, carlos-zamora, leonMSFT and lhecker July 19, 2021 17:12

Merge remote-tracking branch 'origin/main' into dev/migrie/oop/thrott…

cf15df7

…led-funcs

lhecker reviewed Jul 19, 2021

View reviewed changes

src/cascadia/TerminalControl/ControlCore.h Show resolved Hide resolved

lhecker reviewed Jul 19, 2021

View reviewed changes

fix tets

bcce581

lhecker approved these changes Jul 20, 2021

View reviewed changes

zadjii-msft added 7 commits August 4, 2021 10:09

Merge remote-tracking branch 'origin/main' into dev/migrie/oop/thrott…

a6099fb

…led-funcs

Remove one of the unnecessary throttling layers, IMEs still work as e…

f9b4216

…xpected

this prevents the tests from just immediately crashing, but when Cont…

f43f897

…rolUnitTests are run all together, they're flaky and sometimes crash. This brings sadness

these ones are a little volatile, run them in isolation

122b9a3

commentary

a7ff38f

Revert "Remove one of the unnecessary throttling layers, IMEs still w…

3dbac4a

…ork as expected" This reverts commit f9b4216.

frick

b8fa11a

zadjii-msft added 2 commits August 9, 2021 09:20

Well, this makes the tests pass, but it feels stupid

a93b10b

get rid of this nonsense

8baa4f4

zadjii-msft added AutoMerge Marked for automatic merge by the bot when requirements are met and removed Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) labels Aug 9, 2021

ghost merged commit 9f2d406 into main Aug 9, 2021

ghost deleted the dev/migrie/oop/throttled-funcs branch August 9, 2021 15:22

This was referenced Aug 26, 2021

1.11: scrolling at all breaks the URL pattern buffer (no URLs work) #11055

Closed

Update pattern locations again after scrolling #11059

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow `ThrottledFunc` to work on different types of dispatcher#10187

Allow `ThrottledFunc` to work on different types of dispatcher#10187
60 commits merged intomainfrom
dev/migrie/oop/throttled-funcs

zadjii-msft commented May 25, 2021

Uh oh!

Uh oh!

lhecker Jul 19, 2021 •

edited

Loading

Uh oh!

zadjii-msft Aug 4, 2021

Uh oh!

zadjii-msft commented Aug 9, 2021

Uh oh!

ghost commented Aug 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

zadjii-msft commented May 25, 2021

⚠️ targets #10051

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

Uh oh!

lhecker Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

zadjii-msft commented Aug 9, 2021

Uh oh!

ghost commented Aug 9, 2021

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.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lhecker Jul 19, 2021 •

edited

Loading

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.