add rowsToScroll to scrollUp/Down w/ fallback to system default #7924

Don-Vito · 2020-10-15T02:08:24Z

Summary of the Pull Request

The number of lines to move upon scroll up scroll down can be defined
in ScrollUp and ScrollDown commands (parameter is called
"rowsToScroll").
If the number are not provided, use the system default (the one we are
using for mouse scrolls), rather than 1 line.

Validation Steps Performed

Manual testing
Added custom bindings for scroll commands with different values,
verified they and the default appear and behave as expected
Checked that invalid values are not allowed

…lArgs

zadjii-msft

Re-using rowsToScroll like that is really quite clever, I like it. This is again one of those things where it might be hard to test the interaction between the TermControl and the TerminalPage, so it's probably fine without a unit test.

<PM hat> Make sure to update the schema and docs as well</PM hat>, but I think this looks good to go!

src/cascadia/TerminalApp/AppActionHandlers.cpp-a6ed63ee

Don-Vito · 2020-10-15T11:49:24Z

Re-using rowsToScroll like that is really quite clever, I like it. This is again one of those things where it might be hard to test the interaction between the TermControl and the TerminalPage, so it's probably fine without a unit test.

Make sure to update the schema and docs as well</PM hat>, but I think this looks good to go!

Will be done - wanted to hear the initial feedback!

Unrelated question - I see that linter github action is failing on the lines that were not modified. Is there a planned effort to lint the entire code base?

zadjii-msft · 2020-10-15T12:31:36Z

I see that linter github action is failing on the lines that were not modified. Is there a planned effort to lint the entire code base?

Sorry about that. Looks like it's configured incorrectly, I'm hoping that we'll get it sorted out later today. You shouldn't have to change anything for this PR, the clang-format formatting should be good enough for the C++ files. Sorry for the delay 😕

Don-Vito · 2020-10-15T12:54:24Z

I see that linter github action is failing on the lines that were not modified. Is there a planned effort to lint the entire code base?

Sorry about that. Looks like it's configured incorrectly, I'm hoping that we'll get it sorted out later today. You shouldn't have to change anything for this PR, the clang-format formatting should be good enough for the C++ files. Sorry for the delay 😕

@zadjii-msft - I am not sure it is misconfigured: it looks there are some white-space alignment issues in the code.
Another question is regarding the schema in docs. Is it linted usually? While working on the documentation I noticed that:

for some reason "ctrl+shift+up" is not considered a valid binding by the regex in the schema
CloseOtherTabsAction, CloseTabsAfterAction define the the type to be either integer or null, where I believe it should be "null" rather than null. At least my linter is angry about it.

zadjii-msft · 2020-10-15T13:09:38Z

Well, the linter shouldn't be running for C++ files. It actually doesn't know how to lint them, but then erroneously falls back to EDITORCONFIG as a language, and then immediately explodes. I've got a fix over in #7930.

The schema wasn't formally linted, though it might be now that #7637 has merged.

I think there's actually an issue tracking updating the regex in the schema floating around somewhere...
I'm pretty sure that null is what we want here, not the string "null". What error is your linter giving you?

Don-Vito · 2020-10-15T13:24:20Z

Well, the linter shouldn't be running for C++ files. It actually doesn't know how to lint them, but then erroneously falls back to EDITORCONFIG as a language, and then immediately explodes. I've got a fix over in #7930.

The schema wasn't formally linted, though it might be now that #7637 has merged.
* I think there's actually an issue tracking updating the regex in the schema floating around somewhere...

* I'm pretty sure that `null` is what we want here, _not_ the string `"null"`. What error is your linter giving you?

@zadjii-msft - thanks for the clarification!
Regarding the error I am getting:

For whatever reason, the super linter seems to think that any file it doesn't recognize is an EDITORCONFIG file. That means all our `cpp`, `hpp`, `h`, `resw`, `xaml`, etc files are going to get linted with different rules than the clang-format ones we already use. This PR disables the EDITORCONFIG linter, and has a minimal change to a cpp file to ensure that it's no longer linted by the action. See also: * #7637 added this * #7799 is blocked by this * #7924 is blocked by this

WSLUser · 2020-10-16T15:53:23Z

The linter should pass now if you rebase on top of master. It runs every time a new commit is entered and with the updated changes, the ccp files should no longer show any errors. I'm not sure how resw should be treated but most of them passed anyways.

src/cascadia/TerminalApp/TerminalPage.h

miniksa

Great, thanks! Just get that rebase/merge relative to master in here so the linter chills out and we'll be good to go.

…n using a hack with negative enum value

Don-Vito · 2020-10-16T21:31:35Z

@miniksa -Addressed the comment and the linter is better, but now the CI is failing on something that I am not sure related to my changes:

StartGroup: OutputTests::WinPtyWrite#metadataSet1
TAEF: Data[method]: 1
TAEF: Data[selection]: true
Verify: SUCCEEDED(TestData::TryGetValue(L"method", method)): Get which function mode we should use
Verify: SUCCEEDED(TestData::TryGetValue(L"selection", selection)): Get whether we should use selection.
Error: Verify: IsTrue(ret && actual == strlen(buf)): ERROR: WriteFile returned 0: actual=0 LastError=6 (select)
 [File: D:\a\1\s\src\host\ft_host\API_OutputTests.cpp, Function: WinPtyDoWriteTest, Line: 1030]
EndGroup: OutputTests::WinPtyWrite#metadataSet1 [Failed]

Any ideas on how to proceed? 😄

…mount

zadjii-msft · 2020-10-21T08:05:33Z

@Don-Vito Sometimes the CI is just flaky. Looks like it's passed on re-run.

I'll give Dustin the day to block if he has any last concerns, but I think this is good to go IMO.

zadjii-msft · 2020-10-21T08:06:39Z

@msftbot merge this in 14 hours

ghost · 2020-10-21T08:06:43Z

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Wed, 21 Oct 2020 22:06:42 GMT, which is in 14 hours

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

DHowett

I don't love that we're exposing TermControl's "RowsToScroll" as part of its public API, but there's definitely worse things in our codebase.

I just realized, writing this comment, that RowsToScroll has a magic value of -1 (WHEEL_PAGESCROLL) that it can receive from the win32 system parameters API.

It might be better to remove the dependency on TermControl's rows and calculate it using the same mechanism as TC instead, so that we can keep track of the places we're using the system parameter and where we should have to care about WHEEL_PAGESCROLL.

Don-Vito · 2020-10-22T16:18:21Z

I don't love that we're exposing TermControl's "RowsToScroll" as part of its public API, but there's definitely worse things in our codebase.

I just realized, writing this comment, that RowsToScroll has a magic value of -1 (WHEEL_PAGESCROLL) that it can receive from the win32 system parameters API.

It might be better to remove the dependency on TermControl's rows and calculate it using the same mechanism as TC instead, so that we can keep track of the places we're using the system parameter and where we should have to care about WHEEL_PAGESCROLL.

Thanks for the feedback! 😄 I will fix it. The original idea to use the same value to provide a more consistent behavior with mouse scroll speed.

Don-Vito · 2020-10-22T18:31:09Z

@DHowett - when doing the change you suggested, I have few questions:

The value of RowsToScroll is recomputed today in TC. If I move to my own computation, I guess we will need some recomputation mechanism as well.
As long as I call the same API as TC, I will still need to handle the WHEEL_PAGESCROLL return value. So while it will probably be more explicit the fact that we taking this into account persists.

…ontrol

Don-Vito · 2020-10-22T19:57:41Z

@DHowett - I am not sure I got everything correctly 😄. In any case I reverted the change in the TermControl API and compute the default rows to scroll in TerminalPage itself using the same mechanics. To preserve some level of reloading this value I do it upon settings load (in TermControl it is updated upon getting focus as well).

Don-Vito · 2020-10-26T18:20:38Z

@DHowett - anything I can do to unblock this one? I am ready to give some extra to ensure we have hacktoberfest tree planted 😄

DHowett

Love it. Thanks so much!

ghost · 2020-11-11T18:14:08Z

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

Handy links:

Don-Vito added 2 commits October 15, 2020 03:20

5078: use system settings when scrolling

18ba314

5078: add command line parameters support

93a3106

ghost added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Oct 15, 2020

Don-Vito added 3 commits October 15, 2020 05:21

5078: do not convert reference to optional + protect against null rea…

6944fca

…lArgs

Merge branch 'master' into fet/5078-scroll-amount

436a930

5078: remove junk file

7f6fb19

zadjii-msft approved these changes Oct 15, 2020

View reviewed changes

src/cascadia/TerminalApp/AppActionHandlers.cpp-a6ed63ee Outdated Show resolved Hide resolved

5078: add ut for command parameters parsing

a7795d8

zadjii-msft mentioned this pull request Oct 15, 2020

Fix the linter for C++ files #7930

Merged

5078: add schema to docs

0a41a1a

Don-Vito marked this pull request as ready for review October 15, 2020 13:49

DHowett added the hacktoberfest-accepted label Oct 15, 2020

miniksa reviewed Oct 16, 2020

View reviewed changes

src/cascadia/TerminalApp/TerminalPage.h Outdated Show resolved Hide resolved

miniksa approved these changes Oct 16, 2020

View reviewed changes

Don-Vito added 2 commits October 16, 2020 22:49

Merge branch 'master' into fet/5078-scroll-amount

4f3c840

5078: perform comprehensive computation of viewport delta, rather tha…

2f2ee73

…n using a hack with negative enum value

Don-Vito added 3 commits October 20, 2020 19:27

5078: make compute delta function static

b02f9ec

Merge remote-tracking branch 'upstream/master' into fet/5078-scroll-a…

cfa1242

…mount

5078: add copy method to scroll up/down arguments

4e90d99

Don-Vito requested a review from DHowett October 21, 2020 07:27

This comment has been minimized.

Sign in to view

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 21, 2020

DHowett requested changes Oct 22, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 22, 2020

microsoft-github-updates bot changed the base branch from master to main October 22, 2020 00:27

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 22, 2020

Merge remote-tracking branch 'upstream/main' into fet/5078-scroll-amount

d74e873

ghost removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 22, 2020

5078: fix format destroyed by the merge

a3a27f2

Don-Vito added 2 commits October 22, 2020 22:41

5078: decouple sytem level rows to scroll computation from terminal c…

40087f2

…ontrol

5078: cleanup the changes in the TermControl API

a74922b

Don-Vito requested a review from DHowett October 22, 2020 19:53

DHowett approved these changes Oct 27, 2020

View reviewed changes

DHowett changed the title ~~5078: specify the number of lines to scroll with fallback to system default~~ add rowsToScroll to scrollUp/Down w/ fallback to system default Oct 27, 2020

DHowett merged commit b3aab8c into microsoft:main Oct 27, 2020

cinnamon-msft mentioned this pull request Nov 11, 2020

Add remaining 1.5 release features MicrosoftDocs/terminal#176

Merged

ghost mentioned this pull request Nov 11, 2020

Specify number of lines for scollUp and scrollDown commands (+ follow system setting?) #5078

Closed

yogan mentioned this pull request Nov 29, 2020

Terminal uses system default for number of rows scrolled at a time. Can be overridden in settings file. #3575

Merged

1 task

Don-Vito deleted the fet/5078-scroll-amount branch December 3, 2020 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add rowsToScroll to scrollUp/Down w/ fallback to system default #7924

add rowsToScroll to scrollUp/Down w/ fallback to system default #7924

Don-Vito commented Oct 15, 2020 •

edited by DHowett

Loading

zadjii-msft left a comment

Don-Vito commented Oct 15, 2020

zadjii-msft commented Oct 15, 2020

Don-Vito commented Oct 15, 2020

zadjii-msft commented Oct 15, 2020

Don-Vito commented Oct 15, 2020

WSLUser commented Oct 16, 2020

miniksa left a comment

Don-Vito commented Oct 16, 2020

zadjii-msft commented Oct 21, 2020

This comment has been minimized.

This comment has been minimized.

zadjii-msft commented Oct 21, 2020

ghost commented Oct 21, 2020

DHowett left a comment

Don-Vito commented Oct 22, 2020

Don-Vito commented Oct 22, 2020

Don-Vito commented Oct 22, 2020

Don-Vito commented Oct 26, 2020

DHowett left a comment

ghost commented Nov 11, 2020

add rowsToScroll to scrollUp/Down w/ fallback to system default #7924

add rowsToScroll to scrollUp/Down w/ fallback to system default #7924

Conversation

Don-Vito commented Oct 15, 2020 • edited by DHowett Loading

Summary of the Pull Request

Validation Steps Performed

zadjii-msft left a comment

Choose a reason for hiding this comment

Don-Vito commented Oct 15, 2020

zadjii-msft commented Oct 15, 2020

Don-Vito commented Oct 15, 2020

zadjii-msft commented Oct 15, 2020

Don-Vito commented Oct 15, 2020

WSLUser commented Oct 16, 2020

miniksa left a comment

Choose a reason for hiding this comment

Don-Vito commented Oct 16, 2020

zadjii-msft commented Oct 21, 2020

This comment has been minimized.

This comment has been minimized.

zadjii-msft commented Oct 21, 2020

ghost commented Oct 21, 2020

DHowett left a comment

Choose a reason for hiding this comment

Don-Vito commented Oct 22, 2020

Don-Vito commented Oct 22, 2020

Don-Vito commented Oct 22, 2020

Don-Vito commented Oct 26, 2020

DHowett left a comment

Choose a reason for hiding this comment

ghost commented Nov 11, 2020

Don-Vito commented Oct 15, 2020 •

edited by DHowett

Loading