From 26d4980117271ee0d156e62806d65e7cc5b903c5 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Sun, 12 Jul 2020 16:55:11 -0700 Subject: [PATCH 1/4] Remove the rowsToScroll setting and just always use the system setting 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 --- doc/cascadia/SettingsSchema.md | 1 - doc/cascadia/profiles.schema.json | 2 +- .../LocalTests_TerminalApp/SettingsTests.cpp | 8 ++------ .../TerminalApp/GlobalAppSettings.cpp | 16 --------------- src/cascadia/TerminalApp/GlobalAppSettings.h | 1 - .../TerminalApp/defaults-universal.json | 1 - src/cascadia/TerminalApp/defaults.json | 1 - src/cascadia/TerminalControl/TermControl.cpp | 20 ++++++++++++++----- src/cascadia/TerminalControl/TermControl.h | 3 ++- .../TerminalSettings/ICoreSettings.idl | 1 - .../TerminalSettings/TerminalSettings.cpp | 20 ------------------- .../TerminalSettings/terminalsettings.h | 6 ------ .../UnitTests_TerminalCore/MockTermSettings.h | 2 -- src/inc/DefaultSettings.h | 1 - 14 files changed, 20 insertions(+), 63 deletions(-) diff --git a/doc/cascadia/SettingsSchema.md b/doc/cascadia/SettingsSchema.md index 8f1ef08e250..9c58edc89f4 100644 --- a/doc/cascadia/SettingsSchema.md +++ b/doc/cascadia/SettingsSchema.md @@ -16,7 +16,6 @@ Properties listed below affect the entire window, regardless of the profile sett | `initialPosition` | Optional | String | `","` | The position of the top left corner of the window upon first load. On a system with multiple displays, these coordinates are relative to the top left of the primary display. If `launchMode` is set to `"maximized"`, the window will be maximized on the monitor specified by those coordinates. | | `initialRows` | _Required_ | Integer | `30` | The number of rows displayed in the window upon first load. | | `launchMode` | Optional | String | `default` | Defines whether the Terminal will launch as maximized or not. Possible values: `"default"`, `"maximized"` | -| `rowsToScroll` | Optional | Integer | `system` | The number of rows to scroll at a time with the mouse wheel. This will override the system setting if the value is not zero or "system". | | `theme` | _Required_ | String | `system` | Sets the theme of the application. Possible values: `"light"`, `"dark"`, `"system"` | | `showTerminalTitleInTitlebar` | _Required_ | Boolean | `true` | When set to `true`, titlebar displays the title of the selected tab. When set to `false`, titlebar displays "Windows Terminal". | | `showTabsInTitlebar` | Optional | Boolean | `true` | When set to `true`, the tabs are moved into the titlebar and the titlebar disappears. When set to `false`, the titlebar sits above the tabs. | diff --git a/doc/cascadia/profiles.schema.json b/doc/cascadia/profiles.schema.json index 799a0c5fe25..dc61973679d 100644 --- a/doc/cascadia/profiles.schema.json +++ b/doc/cascadia/profiles.schema.json @@ -396,7 +396,7 @@ }, "rowsToScroll": { "default": "system", - "description": "The number of rows to scroll at a time with the mouse wheel. This will override the system setting if the value is not zero or \"system\".", + "description": "This parameter has been deprecated.", "maximum": 999, "minimum": 0, "type": [ "integer", "string" ] diff --git a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp index 938ebd291df..8aed7e4525d 100644 --- a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp @@ -466,15 +466,13 @@ namespace TerminalAppLocalTests { "alwaysShowTabs": true, "initialCols" : 120, - "initialRows" : 30, - "rowsToScroll" : 4 + "initialRows" : 30 })" }; const std::string settings1String{ R"( { "showTabsInTitlebar": false, "initialCols" : 240, - "initialRows" : 60, - "rowsToScroll" : 8 + "initialRows" : 60 })" }; const auto settings0Json = VerifyParseSucceeded(settings0String); const auto settings1Json = VerifyParseSucceeded(settings1String); @@ -485,14 +483,12 @@ namespace TerminalAppLocalTests VERIFY_ARE_EQUAL(true, settings._globals._AlwaysShowTabs); VERIFY_ARE_EQUAL(120, settings._globals._InitialCols); VERIFY_ARE_EQUAL(30, settings._globals._InitialRows); - VERIFY_ARE_EQUAL(4, settings._globals._RowsToScroll); VERIFY_ARE_EQUAL(true, settings._globals._ShowTabsInTitlebar); settings.LayerJson(settings1Json); VERIFY_ARE_EQUAL(true, settings._globals._AlwaysShowTabs); VERIFY_ARE_EQUAL(240, settings._globals._InitialCols); VERIFY_ARE_EQUAL(60, settings._globals._InitialRows); - VERIFY_ARE_EQUAL(8, settings._globals._RowsToScroll); VERIFY_ARE_EQUAL(false, settings._globals._ShowTabsInTitlebar); } diff --git a/src/cascadia/TerminalApp/GlobalAppSettings.cpp b/src/cascadia/TerminalApp/GlobalAppSettings.cpp index b1943037766..e623634ae7f 100644 --- a/src/cascadia/TerminalApp/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalApp/GlobalAppSettings.cpp @@ -22,7 +22,6 @@ static constexpr std::string_view DefaultProfileKey{ "defaultProfile" }; static constexpr std::string_view AlwaysShowTabsKey{ "alwaysShowTabs" }; static constexpr std::string_view InitialRowsKey{ "initialRows" }; static constexpr std::string_view InitialColsKey{ "initialCols" }; -static constexpr std::string_view RowsToScrollKey{ "rowsToScroll" }; static constexpr std::string_view InitialPositionKey{ "initialPosition" }; static constexpr std::string_view ShowTitleInTitlebarKey{ "showTerminalTitleInTitlebar" }; static constexpr std::string_view ThemeKey{ "theme" }; @@ -73,7 +72,6 @@ GlobalAppSettings::GlobalAppSettings() : _defaultProfile{}, _InitialRows{ DEFAULT_ROWS }, _InitialCols{ DEFAULT_COLS }, - _RowsToScroll{ DEFAULT_ROWSTOSCROLL }, _WordDelimiters{ DEFAULT_WORD_DELIMITERS }, _DebugFeaturesEnabled{ debugFeaturesDefault } { @@ -127,7 +125,6 @@ void GlobalAppSettings::ApplyToSettings(TerminalSettings& settings) const noexce settings.KeyBindings(GetKeybindings()); settings.InitialRows(_InitialRows); settings.InitialCols(_InitialCols); - settings.RowsToScroll(_RowsToScroll); settings.WordDelimiters(_WordDelimiters); settings.CopyOnSelect(_CopyOnSelect); @@ -164,19 +161,6 @@ void GlobalAppSettings::LayerJson(const Json::Value& json) JsonUtils::GetInt(json, InitialColsKey, _InitialCols); - if (auto rowsToScroll{ json[JsonKey(RowsToScrollKey)] }) - { - //if it's not an int we fall back to setting it to 0, which implies using the system setting. This will be the case if it's set to "system" - if (rowsToScroll.isInt()) - { - _RowsToScroll = rowsToScroll.asInt(); - } - else - { - _RowsToScroll = 0; - } - } - if (auto initialPosition{ json[JsonKey(InitialPositionKey)] }) { _ParseInitialPosition(initialPosition.asString(), _InitialPosition); diff --git a/src/cascadia/TerminalApp/GlobalAppSettings.h b/src/cascadia/TerminalApp/GlobalAppSettings.h index 052dc9827c4..58f8ea5a469 100644 --- a/src/cascadia/TerminalApp/GlobalAppSettings.h +++ b/src/cascadia/TerminalApp/GlobalAppSettings.h @@ -70,7 +70,6 @@ class TerminalApp::GlobalAppSettings final GETSET_PROPERTY(bool, ConfirmCloseAllTabs, true); GETSET_PROPERTY(winrt::Windows::UI::Xaml::ElementTheme, Theme, winrt::Windows::UI::Xaml::ElementTheme::Default); GETSET_PROPERTY(winrt::Microsoft::UI::Xaml::Controls::TabViewWidthMode, TabWidthMode, winrt::Microsoft::UI::Xaml::Controls::TabViewWidthMode::Equal); - GETSET_PROPERTY(int, RowsToScroll); // default value set in constructor GETSET_PROPERTY(bool, ShowTabsInTitlebar, true); GETSET_PROPERTY(std::wstring, WordDelimiters); // default value set in constructor GETSET_PROPERTY(bool, CopyOnSelect, false); diff --git a/src/cascadia/TerminalApp/defaults-universal.json b/src/cascadia/TerminalApp/defaults-universal.json index 0109df599f9..0ac56908499 100644 --- a/src/cascadia/TerminalApp/defaults-universal.json +++ b/src/cascadia/TerminalApp/defaults-universal.json @@ -21,7 +21,6 @@ // Miscellaneous "confirmCloseAllTabs": true, "theme": "system", - "rowsToScroll": "system", "snapToGridOnResize": true, "profiles": diff --git a/src/cascadia/TerminalApp/defaults.json b/src/cascadia/TerminalApp/defaults.json index e5379f4507f..d0072afcb5b 100644 --- a/src/cascadia/TerminalApp/defaults.json +++ b/src/cascadia/TerminalApp/defaults.json @@ -22,7 +22,6 @@ "confirmCloseAllTabs": true, "startOnUserLogin": false, "theme": "system", - "rowsToScroll": "system", "snapToGridOnResize": true, "profiles": diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 2496c72291a..da092f0b7bf 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -361,8 +361,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation ScrollBar().Visibility(Visibility::Visible); } - // set number of rows to scroll at a time - _rowsToScroll = _settings.RowsToScroll(); + _UpdateSystemParameterSettings(); } // Method Description: @@ -1495,8 +1494,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // With one of the precision mouses, one click is always a multiple of 120, // but the "smooth scrolling" mode results in non-int values - - double newValue = (_rowsToScroll * rowDelta) + (currentOffset); + const auto rowsToScroll{ _rowsToScroll == WHEEL_PAGESCROLL ? GetViewHeight() : _rowsToScroll }; + double newValue = (rowsToScroll * rowDelta) + (currentOffset); // The scroll bar's ValueChanged handler will actually move the viewport // for us. @@ -1697,7 +1696,18 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation _terminal->SetCursorOn(true); _cursorTimer.value().Start(); } - _rowsToScroll = _settings.RowsToScroll(); + + _UpdateSystemParameterSettings(); + } + + // Method Description + // - Updates internal params based on system parameters + void TermControl::_UpdateSystemParameterSettings() noexcept + { + if (!SystemParametersInfoW(SPI_GETWHEELSCROLLLINES, 0, &_rowsToScroll, 0)) + { + _rowsToScroll = 3; + } } // Method Description: diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index 6ab5b19d36b..3879c4b3835 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -150,7 +150,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation std::shared_ptr> _updateScrollBar; bool _isInternalScrollBarUpdate; - int _rowsToScroll; + unsigned int _rowsToScroll; // Auto scroll occurs when user, while selecting, drags cursor outside viewport. View is then scrolled to 'follow' the cursor. double _autoScrollVelocity; @@ -185,6 +185,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation winrt::Windows::UI::Xaml::Controls::SwapChainPanel::LayoutUpdated_revoker _layoutUpdatedRevoker; void _ApplyUISettings(); + void _UpdateSystemParameterSettings() noexcept; void _InitializeBackgroundBrush(); winrt::fire_and_forget _BackgroundColorChanged(const COLORREF color); bool _InitializeTerminal(); diff --git a/src/cascadia/TerminalSettings/ICoreSettings.idl b/src/cascadia/TerminalSettings/ICoreSettings.idl index f1c055cc490..3bc4834592d 100644 --- a/src/cascadia/TerminalSettings/ICoreSettings.idl +++ b/src/cascadia/TerminalSettings/ICoreSettings.idl @@ -22,7 +22,6 @@ namespace Microsoft.Terminal.Settings Int32 HistorySize; Int32 InitialRows; Int32 InitialCols; - Int32 RowsToScroll; Boolean SnapOnInput; Boolean AltGrAliasing; diff --git a/src/cascadia/TerminalSettings/TerminalSettings.cpp b/src/cascadia/TerminalSettings/TerminalSettings.cpp index e811ddf2a07..02d8f452e8f 100644 --- a/src/cascadia/TerminalSettings/TerminalSettings.cpp +++ b/src/cascadia/TerminalSettings/TerminalSettings.cpp @@ -19,24 +19,4 @@ namespace winrt::Microsoft::Terminal::Settings::implementation THROW_HR_IF(E_INVALIDARG, index >= colorTableCount); _colorTable.at(index) = value; } - - int32_t TerminalSettings::RowsToScroll() noexcept - { - if (_rowsToScroll != 0) - { - return _rowsToScroll; - } - int systemRowsToScroll; - if (!SystemParametersInfo(SPI_GETWHEELSCROLLLINES, 0, &systemRowsToScroll, 0)) - { - systemRowsToScroll = 4; - } - return systemRowsToScroll; - } - - void TerminalSettings::RowsToScroll(int32_t value) noexcept - { - _rowsToScroll = value; - } - } diff --git a/src/cascadia/TerminalSettings/terminalsettings.h b/src/cascadia/TerminalSettings/terminalsettings.h index e32a3682f98..1bd1aad98ae 100644 --- a/src/cascadia/TerminalSettings/terminalsettings.h +++ b/src/cascadia/TerminalSettings/terminalsettings.h @@ -39,11 +39,6 @@ namespace winrt::Microsoft::Terminal::Settings::implementation uint32_t GetColorTableEntry(int32_t index) const noexcept; void SetColorTableEntry(int32_t index, uint32_t value); - // the RowsToScroll getter needs to be implemented manually, so it can - // default to SPI_GETWHEELSCROLLLINES - int32_t RowsToScroll() noexcept; - void RowsToScroll(int32_t value) noexcept; - GETSET_PROPERTY(uint32_t, DefaultForeground, DEFAULT_FOREGROUND_WITH_ALPHA); GETSET_PROPERTY(uint32_t, DefaultBackground, DEFAULT_BACKGROUND_WITH_ALPHA); GETSET_PROPERTY(uint32_t, SelectionBackground, DEFAULT_FOREGROUND); @@ -104,7 +99,6 @@ namespace winrt::Microsoft::Terminal::Settings::implementation private: std::array _colorTable{}; - int32_t _rowsToScroll{ 0 }; }; } diff --git a/src/cascadia/UnitTests_TerminalCore/MockTermSettings.h b/src/cascadia/UnitTests_TerminalCore/MockTermSettings.h index aa668d1a923..9d91488075c 100644 --- a/src/cascadia/UnitTests_TerminalCore/MockTermSettings.h +++ b/src/cascadia/UnitTests_TerminalCore/MockTermSettings.h @@ -25,7 +25,6 @@ namespace TerminalCoreUnitTests int32_t HistorySize() { return _historySize; } int32_t InitialRows() { return _initialRows; } int32_t InitialCols() { return _initialCols; } - int32_t RowsToScroll() { return 4; } uint32_t DefaultForeground() { return COLOR_WHITE; } uint32_t DefaultBackground() { return COLOR_BLACK; } bool SnapOnInput() { return false; } @@ -47,7 +46,6 @@ namespace TerminalCoreUnitTests void HistorySize(int32_t) {} void InitialRows(int32_t) {} void InitialCols(int32_t) {} - void RowsToScroll(int32_t) {} void DefaultForeground(uint32_t) {} void DefaultBackground(uint32_t) {} void SnapOnInput(bool) {} diff --git a/src/inc/DefaultSettings.h b/src/inc/DefaultSettings.h index 397c33666ee..6d86a51be7c 100644 --- a/src/inc/DefaultSettings.h +++ b/src/inc/DefaultSettings.h @@ -34,7 +34,6 @@ constexpr uint16_t DEFAULT_FONT_WEIGHT = 400; // normal constexpr int DEFAULT_ROWS = 30; constexpr int DEFAULT_COLS = 120; -constexpr int DEFAULT_ROWSTOSCROLL = 0; const std::wstring DEFAULT_PADDING{ L"8, 8, 8, 8" }; const std::wstring DEFAULT_STARTING_DIRECTORY{ L"%USERPROFILE%" }; From fc39c7d4731eb012396389ad1ecefb8330b34502 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Sun, 12 Jul 2020 17:11:45 -0700 Subject: [PATCH 2/4] Add PAGESCROLL to APIs --- .github/actions/spell-check/dictionary/apis.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/actions/spell-check/dictionary/apis.txt b/.github/actions/spell-check/dictionary/apis.txt index 3b425550541..fdb6a2dcd05 100644 --- a/.github/actions/spell-check/dictionary/apis.txt +++ b/.github/actions/spell-check/dictionary/apis.txt @@ -8,8 +8,8 @@ EXPCMDSTATE fullkbd href IAsync -IBox IBind +IBox IClass IComparable ICustom @@ -28,6 +28,7 @@ NOAGGREGATION NOREDIRECTIONBITMAP oaidl ocidl +PAGESCROLL RETURNCMD rfind roundf From db2b736a93f7bdb7bd10b62c7c303c45e98f7d56 Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Mon, 13 Jul 2020 11:46:10 -0700 Subject: [PATCH 3/4] Document some of the constants --- src/cascadia/TerminalControl/TermControl.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index da092f0b7bf..9578166b14d 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -1490,10 +1490,13 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // negative = down, positive = up // However, for us, the signs are flipped. + // With one of the precision mice, one click is always a multiple of 120 (WHEEL_DELTA), + // but the "smooth scrolling" mode results in non-int values const auto rowDelta = mouseDelta / (-1.0 * WHEEL_DELTA); - // With one of the precision mouses, one click is always a multiple of 120, - // but the "smooth scrolling" mode results in non-int values + // WHEEL_PAGESCROLL is a Win32 constant that represents the "scroll one page + // at a time" setting. If we ignore it, we will scroll a truly absurd number + // of rows. const auto rowsToScroll{ _rowsToScroll == WHEEL_PAGESCROLL ? GetViewHeight() : _rowsToScroll }; double newValue = (rowsToScroll * rowDelta) + (currentOffset); @@ -1706,6 +1709,9 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation { if (!SystemParametersInfoW(SPI_GETWHEELSCROLLLINES, 0, &_rowsToScroll, 0)) { + LOG_LAST_ERROR(); + // If SystemParametersInfoW fails, which it shouldn't, fall back to + // Windows' default value. _rowsToScroll = 3; } } From 0cf3aa223d7e7ce516b62eb3cad3495946ee755e Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Mon, 13 Jul 2020 12:07:05 -0700 Subject: [PATCH 4/4] Update schema to 2019-09, add deprecation for rowsToScroll --- doc/cascadia/profiles.schema.json | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/doc/cascadia/profiles.schema.json b/doc/cascadia/profiles.schema.json index dc61973679d..663a85e4f4c 100644 --- a/doc/cascadia/profiles.schema.json +++ b/doc/cascadia/profiles.schema.json @@ -1,6 +1,6 @@ { "$id": "https://github.com/microsoft/terminal/blob/master/doc/cascadia/profiles.schema.json", - "$schema": "http://json-schema.org/draft-07/schema#", + "$schema": "https://json-schema.org/draft/2019-09/schema#", "title": "Microsoft's Windows Terminal Settings Profile Schema", "definitions": { "KeyChordSegment": { @@ -396,10 +396,11 @@ }, "rowsToScroll": { "default": "system", - "description": "This parameter has been deprecated.", + "description": "This parameter once allowed you to override the systemwide \"choose how many lines to scroll at one time\" setting. It no longer does so.", "maximum": 999, "minimum": 0, - "type": [ "integer", "string" ] + "type": [ "integer", "string" ], + "deprecated": true }, "keybindings": { "description": "Properties are specific to each custom key binding.",