-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #670: Terminal selects a char when bringing window to foreground #856
Conversation
…he terminal window from selecting text.
@@ -75,6 +75,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation | |||
std::unique_ptr<::Microsoft::Console::Render::DxEngine> _renderEngine; | |||
|
|||
Settings::IControlSettings _settings; | |||
bool _focused; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bool _focused; [](start = 8, length = 14)
Could we use _controlRoot.FocusState()
? Click here for the doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I did look into _controlRoot.FocusState(), and the value returned when inside MouseClick event handler is never FocusState::Unfocused. If I had to guess, _controlRoot's focus state is probably changed sometime between the GettingFocus event and the MouseClick event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well that sucks. Thanks for checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me. I'm no XAML wizard, but that feels like it should work. I do echo Carlos's question re: _controlRoot.FocusState
- I'd rather not duplicate state we can look up
} | ||
else | ||
{ | ||
// The user has disabled cursor blinking | ||
_cursorTimer = std::nullopt; | ||
} | ||
|
||
_controlRoot.GotFocus({ this, &TermControl::_GotFocusHandler }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd want to keep this change even if we can use _controlRoot.FocusState
(as Carlos suggested)
{ | ||
args.Handled(true); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed that in CMD/Powershell (outside of WT), an unfocused window can still be scrolled w/ touch. Since we're exiting before line 604, I suspect we won't be able to do that here. Could you double check and fix if that's the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a bit to get the touch simulation working, but I have confirmed that this is the case. A touch and drag will scroll the original CMD and PowerShell terminals without focusing them.
With the focus check at the top of the MouseClick event handler, a touch and drag does not scroll and the window is incorrectly focused.
Moving the focus check inside the PointerDeviceType() == Mouse check does enable scrolling, but the window is still focused.
It looks like a slightly more complicated solution will be necessary. I'll start looking into it tonight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started digging into the issue touch focus/scrolling issue, but I will not have time to properly fix it. I will be out of town for a week starting tomorrow and will be unable to commit any time to this PR.
I did confirm that the touch and drag issue was present before the changes of this PR, and have made a small change that returns the behavior of a touch and drag event to how it originally was while still fixing #670 (in regards to mouse click events).
I'm more than happy to open a new issue for the touch and drag problem. When I return I will be able to devote the time to resolve it. In the mean time, I would hate to see this PR remain open and stagnant for a week because of an already present bug. However it is not my repo, so I'll defer to your preferences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't worry about it. :) Thanks for looking into it. I'll put an issue in (if there isn't one already) and let's get the PR in!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I have a Surface Pro and I've been comparing what your PR has with an admin Powershell instance. I think they're both acting the same way with regards to providing focus. I don't think we'll need an issue for this, then.
…se specific block of code. This lets any touch and drag events scroll the terminal's contents.
My primary concern here is the focus state getting out of sync across window transitions, but I have to trust that the TermControl loses focus when the window loses focus. For our UI toolkit to do otherwise would be utter insanity. |
Thanks for the contribution! |
Summary of the Pull Request
This fix prevents text from being selected when the user clicks within the active terminal to re-focus the window. This brings the new terminal's behavior inline with the original powershell and cmd terminals.
Fixes #670
I tested this fix's behavior manually, but did ensure all unit tests are passing.
PR Checklist
Detailed Description of the Pull Request / Additional comments
The proposed changes may be a naive way of fixing the issue. I simply added a bool called _focused to the TermControl class which is set and unset during the respective GotFocus and LostFocus events. That bool is then checked whenever a new MouseClick event is encountered. If _focused is false during the MouseClick event, the MouseClick event handler immediately returns instead of handling the event normally.
I did try to fix this issue using the GettingFocus event, but I could not find a way to stop the MouseClick event from the GettingFocus event handler. I am pretty new to UWP events, so I am more than happy to revise this fix if a better method exists.