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

Add a spec for output snapping #2529

Merged
13 commits merged into from
Jul 7, 2020
Merged

Add a spec for output snapping #2529

13 commits merged into from
Jul 7, 2020

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Aug 24, 2019

Summary of the Pull Request

This proposes a change to how Terminal will scroll in response to newly
generated output. The Terminal will scroll upon receiving new output if
the viewport is at the bottom of the scroll history and no selection is
active.

This spec also explores the possibility of making this response
configurable with a snapOnOutput profile setting. It also discusses
the possibility of adding a scroll lock keybinding action.

PR Checklist

@dazinator
Copy link

what happens if snapOnOutput is set to noSelection and the user starts selecting whilst output is being produced, so scrolling pauses, meanwhile output finishes so there are now no new output events, and then the user finishes / clears selection. As output finished during selection, once the user has finished selecting will the view port remain where it is? Or will it perform some check and snap to the end?

@carlos-zamora
Copy link
Member Author

what happens if snapOnOutput is set to noSelection and the user starts selecting whilst output is being produced, so scrolling pauses, meanwhile output finishes so there are now no new output events, and then the user finishes / clears selection. As output finished during selection, once the user has finished selecting will the view port remain where it is? Or will it perform some check and snap to the end?

In clearing the selection, you'd just be turning the event listener on. So the viewport would remain where it is assuming snapOnInput = false. BUT if snapOnInput = true, the key you pressed to clear the input would also trigger snapOnInput and move the viewport to the end.

I think an important thing for me to check in this implementation is that clearing the selection should also count as a trigger for snapOnInput. Otherwise, the user has to clear the selection, then press the button again to snap to the bottom.

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a couple questions

doc/cascadia/SnapOnOutput.md Outdated Show resolved Hide resolved
doc/cascadia/SnapOnOutput.md Outdated Show resolved Hide resolved
doc/cascadia/SnapOnOutput.md Outdated Show resolved Hide resolved
@zadjii-msft zadjii-msft added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Product-Terminal The new Windows Terminal. labels Sep 12, 2019
@kojoru
Copy link
Contributor

kojoru commented Oct 11, 2019

Proposal: can we also have the option to auto scroll only if the last line is visible?

This way if I start scrolling up (even without selecting anything), the console will not auto-scroll anymore and will stay whereever I scrolled to. As soon as I scroll back down to the last line, the auto-scroll can resume.

I think that's how iTerm2 works by default, and that's pretty awesome and intuitively does what I expect.

@j4james
Copy link
Collaborator

j4james commented Dec 7, 2019

FYI, there are a couple of private mode escape sequences that some terminals use to control this kind of thing. Mode 1010 specifies whether to snap to the bottom on output, and mode 1011 specifies whether to snap to the bottom on a keypress. If there's a chance we may also want to support these modes one day, it's worth considering how they might interact with this setting.

@zadjii-msft
Copy link
Member

Hey maybe we should bump this, it probably should make the 1.0 cut

Copy link
Contributor

@leonMSFT leonMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of non-blocking thoughts.

doc/cascadia/SnapOnOutput.md Outdated Show resolved Hide resolved
doc/cascadia/SnapOnOutput.md Outdated Show resolved Hide resolved
@zadjii-msft zadjii-msft added this to the Terminal v1.1 milestone Jan 23, 2020
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mostly want this comment called out explicitly, because I think atBottom is what I want from the default value, but I'm not exactly sure

doc/specs/#980 - SnapOnOutput.md Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 14, 2020
@mnpenner
Copy link

atBottom|noSelection sounds ideal. If I scroll up, I definitely don't want it to pop me back down. If I'm selecting something, I don't want it to pop me back down. If I'm already at the bottom and trying to select something, I also don't want it scrolling around on me. i.e. it should only scroll if I'm at the bottom and not selecting something. Can we have that option?

But atBottom is definitely the more important one because I can scroll up a single row if it keeps scrolling on me when I'm trying to select something.

@DHowett-MSFT
Copy link
Contributor

@mnpenner we don’t often get community input during our spec phase, so I’m so glad you’re here 😄

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 15, 2020
@carlos-zamora
Copy link
Member Author

atBottom|noSelection sounds ideal. If I scroll up, I definitely don't want it to pop me back down. If I'm selecting something, I don't want it to pop me back down. If I'm already at the bottom and trying to select something, I also don't want it scrolling around on me. i.e. it should only scroll if I'm at the bottom and not selecting something. Can we have that option?

But atBottom is definitely the more important one because I can scroll up a single row if it keeps scrolling on me when I'm trying to select something.

Adding support for an array AND a singular value. I do kinda like the idea of atBottom | noSelection being the default. 😊 so I've included that too.

@carlos-zamora
Copy link
Member Author

I need to add the following:

What happens if the user tries to add always or never as a flag?

I think the right behavior here is to not allow for that to happen. So, this would be the schema for accepted values:

  • string: always, never, noSelection, atBottom
  • array:
    • possible values: noSelection, atBottom

This way, if the user tries to add always/never to the array, the parsing fails and we fallback to the default value.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 26, 2020
@carlos-zamora
Copy link
Member Author

New idea

What if we actually don't need to make this configurable. Just change the behavior so that...

  • if the viewport is at the bottom, scroll with the new output
  • if a selection exists, stop scrolling

And on top of that, introduce a scrollLock keybinding action.

This covers the following scenarios...

  • I don't want scrolling
    --> use scrollLock (via keybinding or command palette)
    --> scroll up a bit
    --> create a selection
  • I want to auto scroll on new output
    --> scroll to the bottom
    --> use snapOnInput

(include your thoughts below)

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 26, 2020
@carlos-zamora
Copy link
Member Author

For reference, it looks like other terminal emulators don't make this configurable. Took a look at iTerm2 and gnome terminal.

@johnburnett
Copy link

Scrolling up a bit or making a selection both give a very intuitive behavior in my experience. I'd strongly suggest avoiding the scrollLock key, as there's no visual indicator on most keyboards I've seen these days that show what its state is (I'm typing on one now). That would then turn it into an invisible "make scrolling not work right" key (unless there's a real obvious Terminal UI indication that the scroll lock key is toggling this behavior).

@carlos-zamora
Copy link
Member Author

Taking a look at the ScrollLock key in wikipedia. It looks pretty awful. Looks like the key rarely actually does something.

In Unix consoles, Ctrl+S and Ctrl+Q freeze and unfreeze the output respectively. But here's the kicker, the shell is freezing the output, not the terminal. If you try this on PowerShell, for example, Ctrl+S does 'Forward Search History' instead thanks to Powerline.

So scrollLock is put into an interesting situation.

  1. I don't think it's worth it to deal with the ScrollLock key. It's rarely used. It has a different meaning based on whether the lock is on or off. We would have to poll that, and our internal state of "is the scroll locked" has to always align with it.
  2. Adding a scrollLock command that is unbound from the ScrollLock key would require a visual indicator...
    • if the indicator is persistent, it blocks the view
    • if the indicator is not persistent, we get into the problem of an "invisible 'make scrolling not work right' command.

Scrolling up a bit or making a small selection isn't too bad of a solution. So we can just cut the scrollLock keybinding in general, and make the above the default non-configurable behavior (unless/until somebody actually asks for it).

@j4james
Copy link
Collaborator

j4james commented Jun 29, 2020

Just to make things more confusing, note that there is also a Pause key, that pauses the output in the conhost console (pressing any other key will resume scrolling). Not that I'm asking for Windows Terminal to replicate that functionality, but thought it worth mentioning in case anyone else thinks that level of compatibility is worth considering.

@carlos-zamora
Copy link
Member Author

The latest commit introduces a major change. As I mentioned above, what if we made this not configurable. The rest of the spec records the ideas we've discussed here, and provides some reasoning for why not make it configurable.

Because of this, I'm dismissing the past approvals as stale.

@carlos-zamora carlos-zamora dismissed stale reviews from miniksa and zadjii-msft July 6, 2020 23:56

major change

@carlos-zamora carlos-zamora requested a review from a team July 7, 2020 00:01
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think after all this discussion, I'm on board with this. I'll be shocked if anyone really wants "scrollOnOutput": "always" or "pauseScrollingOnSelect": false, but we could always add those in the future if there was huge demand, now that we've enumerated that those settings would work like that.

doc/specs/#980 - SnapOnOutput.md Outdated Show resolved Hide resolved
Co-authored-by: Mike Griese <migrie@microsoft.com>
@DHowett
Copy link
Member

DHowett commented Jul 7, 2020

@leonMSFT can you be the third?

@msftbot make sure @leonMSFT signs off on this

@DHowett DHowett changed the title SnapOnOutput Spec Add a spec for output snapping Jul 7, 2020
@DHowett
Copy link
Member

DHowett commented Jul 7, 2020

Renamed PR to be in imperative mood

@DHowett
Copy link
Member

DHowett commented Jul 7, 2020

Before you merge this, rewrite the description to explain what's really happening.

@carlos-zamora carlos-zamora removed their assignment Jul 7, 2020
@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 7, 2020
@ghost
Copy link

ghost commented Jul 7, 2020

Hello @DHowett!

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.

@ghost ghost merged commit 29f0690 into master Jul 7, 2020
@ghost ghost deleted the dev/cazamor/spec-pause-on-select branch July 7, 2020 21:45
ghost pushed a commit that referenced this pull request Jul 9, 2020
## Summary of the Pull Request
Updates the Terminal's scroll response to new output. The Terminal will not automatically scroll if...
- a selection is active, or
- the viewport is at the bottom of the scroll history

## References
#2529 - Spec
#3863 - Implementation

## PR Checklist
* [X] Closes #980
* [X] Closes #3863
* [ ] Tests added/passed
* [ ] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments
Updates the `_scrollOffset` value properly in TerminalCore when the cursor moves. We calculate a new `_scrollOffset` based on if we are circling the buffer and how far below the mutable bottom is.

We specifically check for if a selection is active and if the viewport is at the bottom, then use that as a condition for deciding if we should update `_scrollOffset` to the new calculated value or 0 (the bottom of the scroll history).

## Validation Steps Performed
Manual testing. Though I should add automated tests.
- [X] new output
- [X] new output when circling
- [X] new output when circling and viewport is at the top
@jstr045329
Copy link

Any progress on this? It's been a couple years and the lack of this feature is quite annoying.

@zadjii-msft
Copy link
Member

@jstr045329 You probably want to go upvote #8879. This thread was a spec review from a few years ago now. The Terminal will "snap on input", but not output. #8879 is tracking that request.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings Issues related to settings and customizability, for console or terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet