Allow ThrottledFunc to work on different types of dispatcher
#10187
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
…ractivity-projection-000
…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
…ractivity-projection-000
…ity-projection-000
ghost
requested review from
carlos-zamora,
DHowett,
leonMSFT,
PankajBhojwani and
lhecker
lhecker
reviewed
Jul 19, 2021
lhecker
reviewed
Jul 19, 2021
Comment on lines
+1158
to
+1160
| // When the buffer's cursor moves, start the throttled func to | ||
| // eventually dispatch a CursorPositionChanged event. | ||
| _tsfTryRedrawCanvas->Run(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
lhecker
approved these changes
Jul 20, 2021
…rolUnitTests are run all together, they're flaky and sometimes crash. This brings sadness
…ork as expected" This reverts commit f9b4216.
|
note to self: Unittests are now failing because |
zadjii-msft
added
AutoMerge
|
Hello @zadjii-msft! Because this pull request has the 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 (
|
This was referenced Aug 26, 2021
ghost
pushed a commit
that referenced
this pull request
Aug 26, 2021
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
DHowett
pushed a commit
that referenced
this pull request
Aug 26, 2021
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)
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of the Pull Request
This updates our
ThrottledFuncs to take a dispatcher parameter. This means that we can use theWindows::UI::Core::CoreDispatcherin theTermControl, where there's always aCoreDispatcher, and use aWindows::System::DispatcherQueueinControlCore/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
TermControlto let it throttle that update anymore.References
PR Checklist
Detailed Description of the Pull Request / Additional comments
Fortunately,
winrt::resume_foregroundworks the same on both aCoreDispatcherand aDispatcherQueue, so this wasn't too hard!Validation Steps Performed
This was validated in
dev/migrie/oop/the-whole-thing(ordev/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.