Skip to content
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

5366: delay scrolling / clear selection until the input sequence is created #7896

Closed

Conversation

Don-Vito
Copy link
Contributor

@Don-Vito Don-Vito commented Oct 12, 2020

Summary of the Pull Request

Right now the decisions to clear text selection or to scroll down is triggered too early, before we even decided to handle the event at all. This PR is an attempt to delay this decision until TermControl gets notified about input sequence.

References

PR Checklist

  • Closes #Media and other keys autoscroll the terminal #5366
  • CLA signed.
  • Tests added/passed - not added yet
  • Documentation updated - irrelevant
  • Schema updated - irrelevant
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

So this one should be very simple: don't invoke selection clearing / scrolling immediately, but rather in the TermControl once a sequence received from the callback.

What confused me was that delaying the decision was not enough: there are sequences that we do send to the connection that actually don't require scrolling / clearing selection. For instance Print Screen key is sent to the connection, but no action should be taken on Terminal side.

So I still had to preserve the safeguards that exist today. The problem is that to preserve these safeguards, TermControl should be aware of the stuff like was it key-up / key-down, etc. Though this information can be decoded from the sequence I decided to propagate the IInputEvent all the way through. This required some massive change of function prototypes.

Validation Steps Performed

  • Manual tests only by now

@Don-Vito Don-Vito changed the title 5366: delay scrolling / clear selection until the input is reported t… 5366: delay scrolling / clear selection until the input sequence is created Oct 12, 2020
@zadjii-msft zadjii-msft self-assigned this Oct 12, 2020
@lhecker
Copy link
Member

lhecker commented Oct 13, 2020

Just an idea - and please strap yourself in, because it's a fugly one - but...
Under the assumption that SendKeyEvent is synchronous and that we intent do refactor the input handling in the long term anyways, you could achieve the same functionality within this PR, by adding a bool _clearSelectionOnSend member to TermControl.
In _TrySendKeyEvent, before calling SendKeyEvent, you add the following code:

_clearSelectionOnSend = _terminal->IsSelectionActive() && !KeyEvent::IsModifierKey(vkey));
auto cleanup = wil::scope_exit([&]() noexcept {
    _clearSelectionOnSend = false;
});

And in _UpdateTerminalOnInput you do:

if (_clearSelectionOnSend)
{
    _clearSelectionOnSend = false;
    _terminal->ClearSelection();
    _renderer->TriggerSelection();
}

Pro: Less changes. Con: Fugly.
Pro2: If we do refactor the input handling next year (or somesuch) this code will probably get changed again anyways.

@Don-Vito
Copy link
Contributor Author

@lhecker - I absolutely considered this one. And I can see how it makes sense if we really rewrite the logic soon 😄 .
The only difference is that I was thinking of storing a _keyEvent rather than just _clearSelectionOnSend. This would leave place for some more flexibility.

I will create a separate PR with the suggested approach and then we can throw the responsibility on @zadjii-msft to choose which one the team likes more 😄

@Don-Vito
Copy link
Contributor Author

@zadjii-msft, @lhecker - probably don't waste too much time on this one yet. I believe that while dogfooding I found some weird regression: the text selection does not always get cleared when I type. It is weird because in exactly the same scenario it gets cleared if scrolling down (even for a single line) is required as well. I mean that in debugger I do see _UpdateTerminalOnInput is called, ClearSelection and TriggerSelection are invoked, selection becomes empty, but for some reason the re-rendering does not happen. Right now I am quite clueless.

@microsoft-github-updates microsoft-github-updates bot changed the base branch from master to main October 22, 2020 00:27
@Don-Vito
Copy link
Contributor Author

Don-Vito commented Nov 2, 2020

@zadjii-msft - I am abandoning this by now. I have a plan to rewrite it.. also want to focus you on other 5 PRs that are still pending 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants