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

hannesne · 2019-11-14T03:56:11Z

The terminal will use the system setting to determine the number of lines to scroll at a time.
This can be overridden by adding rowsToScroll to app global settings file.
terminal will use the system setting if the app setting is 0, or not specified. No restart is needed to reflect setting changes in system or the settings file.

The default was hardcoded to 4 in the code with a todo comment. 1 works better on precision touchpads, where 4 scrolls too fast.

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

Validation Steps Performed

Launch terminal
cat a large file so that there are many lines to scroll in between.
Launch settings editor.
Add rowsToScroll: 1 to global settings, and save the file.
Scroll the page, it should now go nice and slow.
Change the setting to 20, and save the file.
Scrolling should now be super fast.
Restart the terminal, and verify that scrolling is still super fast.
Set rowsToScroll to 0 in settings and save the file.
Verify that scrolling happens at system setting.
Change system setting to max and switch focus back to terminal to see scrolling happen very fast.
Change system setting to minimum and see scrolling happen very slow.

Example of profiles.json file:

// To view the default settings, hold "alt" while clicking on the "Settings" button.
// For documentation on these settings, see: https://aka.ms/terminal-documentation

{
    "$schema": "https://aka.ms/terminal-profiles-schema",

    "defaultProfile": "{574e775e-4f2a-5b96-ac1e-a2962a402336}",
    "rowsToScroll": 1,

    "profiles":
    [
        {
            // Make changes here to the powershell.exe profile
            "guid": "{61c54bbd-c2c6-5271-96e7-009a87ff44bf}",
            "name": "Windows PowerShell",
            "commandline": "powershell.exe",
            "hidden": false
        },
        {
            // Make changes here to the cmd.exe profile
            "guid": "{0caa0dad-35be-5f56-a8ff-afceeeaa6101}",
            "name": "cmd",
            "commandline": "cmd.exe",
            "hidden": false
        },
        {
            "guid": "{574e775e-4f2a-5b96-ac1e-a2962a402336}",
            "hidden": false,
            "name": "PowerShell Core",
            "source": "Windows.Terminal.PowershellCore"
        },
        {
            "guid": "{c6eaf9f4-32a7-5fdc-b5cf-066e8a4b1e40}",
            "hidden": false,
            "name": "Ubuntu-18.04",
            "source": "Windows.Terminal.Wsl"
        }
    ],

    // Add custom color schemes to this array
    "schemes": [],

    // Add any keybinding overrides to this array.
    // To unbind a default keybinding, set the command to "unbound"
    "keybindings": []
}

…ber of rows scrolled at a time. Default was hardcoded to 4. 1 works better on precision touchpads.

…nnesne/configure_rows_scrolled

…/hannesne/terminal into hannesne/configure_rows_scrolled

mdtauk · 2019-12-02T23:06:23Z

The default should probably match the Windows settings for scrolling, with a default option as well as overriding with custom values.

…'s not overridden in settings.

hannesne · 2019-12-04T05:48:28Z

I've done it! The terminal will now use the system setting for lines scrolled. This can be overridden by specifying a value in the app settings file. If the value specified is 0, terminal will use the system setting. cc @mdtauk

… focus, not on every scroll event. More efficient.

DHowett-MSFT · 2019-12-06T18:50:53Z

(Sorry for the silence here! We've been trying to stay on top of our PR backlog, but with the November-December holiday seasons here in the US our velocities aren't as great as we'd like.)

carlos-zamora

Personally don't like that 0 or null are considered the system setting. I'd prefer something like "system" for the system setting or a number for the number. We should probably just invalidate the profile if 0 or null are passed in. Then throw a warning? @DHowett-MSFT @cinnamon-msft thoughts?

Also, please update the following too:

src/cascadia/TerminalControl/TermControl.cpp

src/cascadia/TerminalApp/defaults.json

hannesne · 2019-12-12T00:05:14Z

Personally don't like that 0 or null are considered the system setting. I'd prefer something like "system" for the system setting or a number for the number. We should probably just invalidate the profile if 0 or null are passed in. Then throw a warning? @DHowett-MSFT @cinnamon-msft

@carlos-zamora
Supporting this requires changing the setting type to string (currently it's integer). It adds additional complexity by then requiring specialised parsing in the code. Given that the real default is to not specify it, I'm not sure if it's worth the effort at this point?

I've implemented all the other requested changes.

zadjii-msft

Largely I really like this - I just think we need to move the SystemParametersInfo call to where we're parsing instead of in TermControl.

I'd similarly prefer that we use "system" to indicate using the system scroll amount, as opposed to null/0, but that can always be done in a follow-up PR.

src/cascadia/TerminalControl/TermControl.cpp

src/cascadia/TerminalApp/defaults.json

src/cascadia/TerminalApp/GlobalAppSettings.cpp

src/inc/DefaultSettings.h

…e of 'system' for default in settings.

hannesne · 2019-12-13T01:35:07Z

I didn't like calling the system for the setting in the termcontrol either, so I've changed it now so that the globablappsettings will assign 0 to the internal value if anything other than a non-zero int value is in the settings when parsing the settings file (the default is now "system"). The terminal settings will then in the accessor determine if a valid setting is provided, falling back to the system setting. The terminal control will update it's internal value using that accessor when settings change and when it receives focus so that you don't have to restart the terminal when you change the system setting. @zadjii-msft

miniksa · 2019-12-30T23:01:37Z

Not going to override @carlos-zamora because I don't see an answer to the thing he specifically asked of @DHowett-MSFT and @cinnamon-msft regarding using the 0/null state for "get system default".

hannesne · 2020-01-04T21:32:02Z

@carlos-zamora requested changes have been addressed by making "system" the default option in the settings file. Any value that is not 0 or an integer (null, "system", or not specified) will default to using the system setting. cc @miniksa

miniksa · 2020-01-06T17:38:07Z

@carlos-zamora Carlos Zamora FTE requested changes have been addressed by making "system" the default option in the settings file. Any value that is not 0 or an integer (null, "system", or not specified) will default to using the system setting. cc @miniksa Michael Niksa FTE

Ah alright. Sorry, that wasn't clear to me sooner.

Also, I checked the calendar and @carlos-zamora isn't due back for another week, so I'm going to clear his review and merge this.

He's OOF and his issues have been addressed, as far as I can discern.

hannesne · 2020-01-07T08:02:27Z

Thank you for the support @miniksa ! :)

j4james · 2020-01-07T16:03:14Z

src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp

            "globals": {
                "alwaysShowTabs": true,
                "initialCols" : 120,
-                "initialRows" : 30
+                "initialRows" : 30,
+                "rowsToScroll" :  4,
            }


I'm not sure why this wasn't picked up by the build bot, but this test now fails for me when run locally because of the trailing comma (that's not valid JSON).

zadjii-msft · 2020-01-07T16:49:16Z

@j4james the tests all defined in LocalTests_TerminalApp don't actually run in CI - it's been my monthslong hell to try and make that work ;___;

See #3838 for the most recent incarnation of fixing them.

I'll move your comment to it's own issue, cause that should be fixed.

ghost · 2020-01-14T17:16:19Z

🎉Windows Terminal Preview v0.8.10091.0 has been released which incorporates this pull request.:tada:

Handy links:

realdadfish · 2020-01-17T09:41:47Z

I'm running v0.8.10091.0, but the setting is not visible in the default (generated) settings JSON. Was it forgotten to be added there or is the auto-generation of the default setting value not implemented...?

Basically, people don't know the setting exists until they stumble upon this PR or the accompanying issue.

DHowett-MSFT · 2020-02-21T18:31:20Z

Hey @hannesne! It looks like somebody came through and fixed the scroll wheel deltas, which obviates the need for rowsToScroll. It feels really great in the default configuration, but right now if you set rowsToScroll to 1, it'll have somewhat of an adverse effect on your ability to scroll 😦

I can send you a release build from master if you want to give it a test.

Should we revert this feature, or change the key name so users who set 1 aren't left surprised when their scrolling is suddenly 3x slower than it used to be?

yogan · 2020-11-29T11:27:15Z

I'm confused here. The v1.5.3142.0 release notes said:

The scrollUp and scrollDown actions have learned about rowsToScroll (thanks @Don-Vito!) (#7924)

But the config schema docs tell me that rowsToScroll is no longer a thing?

Unfortunately the config docs seem to be right. No matter what (numeric) value I set, I only get three lines of
scrolling (my System setting). I don't want to change that, it's fine for most apps and would also be fine for
mouse wheel scrolling in WT, but I'd like to get usable keyboard scrolling, and for that 3 is way to low for me,
that means I have to smash those keys like a maniac. ;-)

Running v1.5.3242.0 here.

Don-Vito · 2020-11-29T11:31:08Z

@yogan - you can now set rowsToScroll as a part of the binding, something like:

{ "command": {"action": "scrollUp", "rowsToScroll":16}, "keys": "ctrl+shift+up" },

yogan · 2020-11-29T15:19:17Z

Perfect! Just what I was looking for. Thanks @Don-Vito!

Maybe add a hint like that to the docs of the global rowsToScroll option instead or in addition to that deprecation notice to let others find about it as well.

Hannes Nel and others added 6 commits November 14, 2019 16:41

Added 'rowsToScroll' setting in global settings, to configure the num…

7bc8d46

…ber of rows scrolled at a time. Default was hardcoded to 4. 1 works better on precision touchpads.

Merge branch 'master' of https://github.com/hannesne/terminal into ha…

0e3c5d2

…nnesne/configure_rows_scrolled

Merge branch 'master' into hannesne/configure_rows_scrolled

0e3f086

Ran Invoke-CodeFormat

b7dd11b

Merge branch 'hannesne/configure_rows_scrolled' of https://github.com…

ad6f3ac

…/hannesne/terminal into hannesne/configure_rows_scrolled

Run code format

20c8af0

hannesne mentioned this pull request Nov 27, 2019

Touchpad scrolling too fast -- should act in precision mode #1066

Closed

Hannes Nel added 2 commits December 2, 2019 17:10

Fixed whitespace in file for better diff in PR.

7336711

Fixed whitespace in file for better diff in PR.

37b408d

Terminal now uses system settings for number of lines to scroll if it…

5a3cd44

…'s not overridden in settings.

Terminal control will only grab system setting for lines to scroll on…

d9bbe0d

… focus, not on every scroll event. More efficient.

hannesne changed the title ~~Added setting in global settings to configure the number of rows scrolled at a time~~ Terminal uses system default for number of rows scrolled at a time. Can be overridden in settings file. Dec 4, 2019

carlos-zamora previously requested changes Dec 11, 2019

View reviewed changes

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/defaults.json Outdated Show resolved Hide resolved

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Dec 11, 2019

Hannes Nel added 4 commits December 12, 2019 12:06

Merge with Master

b231532

Updated settings schema and addressed feedback.

7a4ff11

Updated settings schema docs.

b89d7f7

Performed code format

892265d

zadjii-msft requested changes Dec 12, 2019

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Dec 12, 2019

Moved determination of system scrolllines to settings, and enabled us…

0e97d79

…e of 'system' for default in settings.

hannesne requested review from zadjii-msft and carlos-zamora December 13, 2019 09:46

zadjii-msft added the Product-Terminal The new Windows Terminal. label Dec 13, 2019

ghost requested review from miniksa and DHowett-MSFT December 13, 2019 13:27

Merged to master

d211165

zadjii-msft added this to the Terminal-2001 milestone Dec 30, 2019

miniksa approved these changes Dec 30, 2019

View reviewed changes

Hannes Nel added 3 commits January 5, 2020 09:05

Merged from Master

d146ca3

Merged from master

b54b8da

Added noexcept to RowsToScroll properties.

069fc50

miniksa merged commit a60ed52 into microsoft:master Jan 6, 2020

hannesne deleted the hannesne/configure_rows_scrolled branch January 7, 2020 08:05

j4james reviewed Jan 7, 2020

View reviewed changes

ghost removed the Needs-Second It's a PR that needs another sign-off label Jan 7, 2020

This was referenced Jan 7, 2020

LocalTests were broken in #3575 #4137

Closed

Fix a test that regressed during #3575 #4143

Merged

DHowett-MSFT pushed a commit that referenced this pull request Jan 7, 2020

Fix a test that regressed during #3575 (#4143)

911a9dd

iamakulov mentioned this pull request Jan 23, 2020

Allow string values (like "system") in the rowsToScroll schema #4342

Merged

5 tasks

Don-Vito mentioned this pull request Nov 29, 2020

schema: make rowsToScroll deprecation message mention bindings #8439

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hannesne commented Nov 14, 2019 •

edited

Loading

mdtauk commented Dec 2, 2019

hannesne commented Dec 4, 2019 •

edited

Loading

DHowett-MSFT commented Dec 6, 2019

carlos-zamora left a comment

hannesne commented Dec 12, 2019

zadjii-msft left a comment

hannesne commented Dec 13, 2019 •

edited

Loading

miniksa commented Dec 30, 2019

hannesne commented Jan 4, 2020

miniksa commented Jan 6, 2020

hannesne commented Jan 7, 2020

j4james Jan 7, 2020

zadjii-msft commented Jan 7, 2020

ghost commented Jan 14, 2020

realdadfish commented Jan 17, 2020

DHowett-MSFT commented Feb 21, 2020

yogan commented Nov 29, 2020

Don-Vito commented Nov 29, 2020

yogan commented Nov 29, 2020

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

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

Conversation

hannesne commented Nov 14, 2019 • edited Loading

Validation Steps Performed

mdtauk commented Dec 2, 2019

hannesne commented Dec 4, 2019 • edited Loading

DHowett-MSFT commented Dec 6, 2019

carlos-zamora left a comment

Choose a reason for hiding this comment

hannesne commented Dec 12, 2019

zadjii-msft left a comment

Choose a reason for hiding this comment

hannesne commented Dec 13, 2019 • edited Loading

miniksa commented Dec 30, 2019

hannesne commented Jan 4, 2020

miniksa commented Jan 6, 2020

hannesne commented Jan 7, 2020

j4james Jan 7, 2020

Choose a reason for hiding this comment

zadjii-msft commented Jan 7, 2020

ghost commented Jan 14, 2020

realdadfish commented Jan 17, 2020

DHowett-MSFT commented Feb 21, 2020

yogan commented Nov 29, 2020

Don-Vito commented Nov 29, 2020

yogan commented Nov 29, 2020

hannesne commented Nov 14, 2019 •

edited

Loading

hannesne commented Dec 4, 2019 •

edited

Loading

hannesne commented Dec 13, 2019 •

edited

Loading