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

Remove the rowsToScroll setting and just always use the system setting #6891

Merged
4 commits merged into from
Jul 14, 2020

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Jul 13, 2020

This parameter was added as a workaround for our fast trackpad
scrolling. Since that was fixed before 1.0 shipped, in #4554, it has
been largely vestigial. There is no reason for us to keep it around any
longer.

It was also the only "logic" in TerminalSettings, which is otherwise a
library that only transits data between two other libraries.

I have not removed it from the schema, as I do not want to mark folks'
settings files invalid to a strict schema parser.

While I was in the area, I added support for "scroll one screen at a
time" (which is represented by the API returning WHEEL_PAGESCROLL),
fixing #5610. We were also storing it in an int (whoops) instead of a
uint.

Fixes #5610

This parameter was added as a workaround for our fast trackpad
scrolling. Since that was fixed before 1.0 shipped, in #4554, it has
been largely vestigial. There is no reason for us to keep it around any
longer.

It was also the only "logic" in TerminalSettings, which is otherwise a
library that only transits data between two other libraries.

I have not removed it from the schema, as I do not want to mark folks'
settings files invalid to a strict schema parser.

While I was in the area, I added support for "scroll one screen at a
time" (which is represented by the API returning WHEEL_PAGESCROLL),
fixing #5610. We were also storing it in an int (whoops) instead of a
uint.

Fixes #5610
@ghost ghost added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Jul 13, 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.

Really just blocking over the 3 question

src/cascadia/TerminalControl/TermControl.cpp Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp 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 Jul 13, 2020
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

+1 to Mike's comments

Don't forget about the docs repo

doc/cascadia/profiles.schema.json Outdated Show resolved Hide resolved
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 13, 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.

thanks for the additional comments - no one in their right mind would think that 3 was an intentional value.

DHowett added a commit to MicrosoftDocs/terminal that referenced this pull request Jul 14, 2020
@DHowett
Copy link
Member Author

DHowett commented Jul 14, 2020

@carlos-zamora doc discussion ongoing in MicrosoftDocs/terminal#87

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 14, 2020
@ghost
Copy link

ghost commented Jul 14, 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 06b50b4 into master Jul 14, 2020
@ghost ghost deleted the dev/duhowett/kill-rowsToScroll branch July 14, 2020 01:38
@ghost
Copy link

ghost commented Jul 22, 2020

🎉Windows Terminal Preview v1.2.2022.0 has been released which incorporates this pull request.:tada:

Handy links:

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-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mouse wheel scrolls backwards when set to "one screen at a time"
3 participants