Fix Reverse Walking in AttrRowIterator (#3566)

(cherry picked from commit 5bbf7e2650)

doc/cascadia/profiles.schema.json

@@ -100,8 +100,8 @@
             "action": { "type": "string", "pattern": "copy" },
             "trimWhitespace": {
               "type": "boolean",
-              "default": false,
+              "default": true,
-              "description": "If true, will trim whitespace from the end of the line on copy."
+              "description": "If true, whitespace is removed and newlines are maintained. If false, newlines are removed and whitespace is maintained."
             }
           }
         }

src/buffer/out/AttrRowIterator.cpp

@@ -16,20 +16,22 @@ AttrRowIterator AttrRowIterator::CreateEndIterator(const ATTR_ROW* const attrRow
 AttrRowIterator::AttrRowIterator(const ATTR_ROW* const attrRow) noexcept :
     _pAttrRow{ attrRow },
     _run{ attrRow->_list.cbegin() },
-    _currentAttributeIndex{ 0 }
+    _currentAttributeIndex{ 0 },
+    _exceeded{ false }
 {
 }
 
 AttrRowIterator::operator bool() const
 {
-    return _run < _pAttrRow->_list.cend();
+    return !_exceeded && _run < _pAttrRow->_list.cend();
 }
 
 bool AttrRowIterator::operator==(const AttrRowIterator& it) const
 {
     return (_pAttrRow == it._pAttrRow &&
             _run == it._run &&
-            _currentAttributeIndex == it._currentAttributeIndex);
+            _currentAttributeIndex == it._currentAttributeIndex &&
+            _exceeded == it._exceeded);
 }
 
 bool AttrRowIterator::operator!=(const AttrRowIterator& it) const
@@ -52,6 +54,8 @@ AttrRowIterator AttrRowIterator::operator++(int)
 
 AttrRowIterator& AttrRowIterator::operator+=(const ptrdiff_t& movement)
 {
+    if (!_exceeded)
+    {
         if (movement >= 0)
         {
             _increment(gsl::narrow<size_t>(movement));
@@ -60,6 +64,7 @@ AttrRowIterator& AttrRowIterator::operator+=(const ptrdiff_t& movement)
         {
             _decrement(gsl::narrow<size_t>(-movement));
         }
+    }
 
     return *this;
 }
@@ -84,11 +89,13 @@ AttrRowIterator AttrRowIterator::operator--(int)
 
 const TextAttribute* AttrRowIterator::operator->() const
 {
+    THROW_HR_IF(E_BOUNDS, _exceeded);
     return &_run->GetAttributes();
 }
 
 const TextAttribute& AttrRowIterator::operator*() const
 {
+    THROW_HR_IF(E_BOUNDS, _exceeded);
     return _run->GetAttributes();
 }
 
@@ -123,14 +130,23 @@ void AttrRowIterator::_decrement(size_t count)
 {
     while (count > 0)
     {
+        // If there's still space within this color attribute to move left, do so.
         if (count <= _currentAttributeIndex)
         {
             _currentAttributeIndex -= count;
             return;
         }
+        // If there's not space, move to the previous attribute run
+        // We'll walk through above on the if branch to move left further (if necessary)
         else
         {
-            count -= _currentAttributeIndex;
+            // make sure we don't go out of bounds
+            if (_run == _pAttrRow->_list.cbegin())
+            {
+                _exceeded = true;
+                return;
+            }
+            count -= _currentAttributeIndex + 1;
             --_run;
             _currentAttributeIndex = _run->GetLength() - 1;
         }

src/buffer/out/AttrRowIterator.hpp

@@ -54,6 +54,7 @@ private:
     std::vector<TextAttributeRun>::const_iterator _run;
     const ATTR_ROW* _pAttrRow;
     size_t _currentAttributeIndex; // index of TextAttribute within the current TextAttributeRun
+    bool _exceeded;
 
     void _increment(size_t count);
     void _decrement(size_t count);

src/cascadia/CascadiaPackage/CascadiaPackage.wapproj

@@ -111,12 +111,12 @@
   </Target>
 
   <!-- This is required to get the package dependency in the AppXManifest. -->
-  <Import Project="..\..\..\packages\Microsoft.UI.Xaml.2.2.191021001-prerelease\build\native\Microsoft.UI.Xaml.targets" Condition="Exists('..\..\..\packages\Microsoft.UI.Xaml.2.2.191021001-prerelease\build\native\Microsoft.UI.Xaml.targets')" />
+  <Import Project="..\..\..\packages\Microsoft.UI.Xaml.2.2.191203001-prerelease\build\native\Microsoft.UI.Xaml.targets" Condition="Exists('..\..\..\packages\Microsoft.UI.Xaml.2.2.191203001-prerelease\build\native\Microsoft.UI.Xaml.targets')" />
   <Target Name="EnsureNuGetPackageBuildImports" BeforeTargets="PrepareForBuild">
     <PropertyGroup>
       <ErrorText>This project references NuGet package(s) that are missing on this computer. Use NuGet Package Restore to download them.  For more information, see http://go.microsoft.com/fwlink/?LinkID=322105. The missing file is {0}.</ErrorText>
     </PropertyGroup>
-    <Error Condition="!Exists('..\..\..\packages\Microsoft.UI.Xaml.2.2.191021001-prerelease\build\native\Microsoft.UI.Xaml.targets')" Text="$([System.String]::Format('$(ErrorText)', '..\..\..\packages\Microsoft.UI.Xaml.2.2.191021001-prerelease\build\native\Microsoft.UI.Xaml.targets'))" />
+    <Error Condition="!Exists('..\..\..\packages\Microsoft.UI.Xaml.2.2.191203001-prerelease\build\native\Microsoft.UI.Xaml.targets')" Text="$([System.String]::Format('$(ErrorText)', '..\..\..\packages\Microsoft.UI.Xaml.2.2.191203001-prerelease\build\native\Microsoft.UI.Xaml.targets'))" />
   </Target>
 
 </Project>

src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp

@@ -213,24 +213,24 @@ namespace TerminalAppLocalTests
 
         {
             Log::Comment(NoThrowString().Format(
-                L"Verify that `copy` without args parses as Copy(TrimWhitespace=false)"));
+                L"Verify that `copy` without args parses as Copy(TrimWhitespace=true)"));
             KeyChord kc{ true, false, false, static_cast<int32_t>('C') };
             auto actionAndArgs = GetActionAndArgs(*appKeyBindings, kc);
             const auto& realArgs = actionAndArgs.Args().try_as<CopyTextArgs>();
             VERIFY_IS_NOT_NULL(realArgs);
             // Verify the args have the expected value
-            VERIFY_IS_FALSE(realArgs.TrimWhitespace());
+            VERIFY_IS_TRUE(realArgs.TrimWhitespace());
         }
 
         {
             Log::Comment(NoThrowString().Format(
-                L"Verify that `copyTextWithoutNewlines` parses as Copy(TrimWhitespace=true)"));
+                L"Verify that `copyTextWithoutNewlines` parses as Copy(TrimWhitespace=false)"));
             KeyChord kc{ false, true, false, static_cast<int32_t>('C') };
             auto actionAndArgs = GetActionAndArgs(*appKeyBindings, kc);
             const auto& realArgs = actionAndArgs.Args().try_as<CopyTextArgs>();
             VERIFY_IS_NOT_NULL(realArgs);
             // Verify the args have the expected value
-            VERIFY_IS_TRUE(realArgs.TrimWhitespace());
+            VERIFY_IS_FALSE(realArgs.TrimWhitespace());
         }
 
         {
@@ -325,7 +325,7 @@ namespace TerminalAppLocalTests
             const auto& realArgs = actionAndArgs.Args().try_as<CopyTextArgs>();
             VERIFY_IS_NOT_NULL(realArgs);
             // Verify the args have the expected value
-            VERIFY_IS_FALSE(realArgs.TrimWhitespace());
+            VERIFY_IS_TRUE(realArgs.TrimWhitespace());
         }
 
         {
@@ -337,7 +337,7 @@ namespace TerminalAppLocalTests
             const auto& realArgs = actionAndArgs.Args().try_as<CopyTextArgs>();
             VERIFY_IS_NOT_NULL(realArgs);
             // Verify the args have the expected value
-            VERIFY_IS_FALSE(realArgs.TrimWhitespace());
+            VERIFY_IS_TRUE(realArgs.TrimWhitespace());
         }
 
         {

src/cascadia/LocalTests_TerminalApp/TerminalApp.LocalTests.vcxproj

@@ -93,11 +93,11 @@
     <!-- From Microsoft.UI.Xaml.targets -->
     <Native-Platform Condition="'$(Platform)' == 'Win32'">x86</Native-Platform>
     <Native-Platform Condition="'$(Platform)' != 'Win32'">$(Platform)</Native-Platform>
-    <_MUXBinRoot>&quot;$(OpenConsoleDir)packages\Microsoft.UI.Xaml.2.2.191021001-prerelease\runtimes\win10-$(Native-Platform)\native\&quot;</_MUXBinRoot>
+    <_MUXBinRoot>&quot;$(OpenConsoleDir)packages\Microsoft.UI.Xaml.2.2.191203001-prerelease\runtimes\win10-$(Native-Platform)\native\&quot;</_MUXBinRoot>
   </PropertyGroup>
 
 <!-- We actually can just straight up reference MUX here, it's fine -->
-  <Import Project="..\..\..\packages\Microsoft.UI.Xaml.2.2.191021001-prerelease\build\native\Microsoft.UI.Xaml.targets" Condition="Exists('..\..\..\packages\Microsoft.UI.Xaml.2.2.191021001-prerelease\build\native\Microsoft.UI.Xaml.targets')" />
+  <Import Project="..\..\..\packages\Microsoft.UI.Xaml.2.2.191203001-prerelease\build\native\Microsoft.UI.Xaml.targets" Condition="Exists('..\..\..\packages\Microsoft.UI.Xaml.2.2.191203001-prerelease\build\native\Microsoft.UI.Xaml.targets')" />
   <Import Project="..\..\..\packages\Microsoft.Toolkit.Win32.UI.XamlApplication.6.0.0-preview6.2\build\native\Microsoft.Toolkit.Win32.UI.XamlApplication.targets" Condition="Exists('..\..\..\packages\Microsoft.Toolkit.Win32.UI.XamlApplication.6.0.0-preview6.2\build\native\Microsoft.Toolkit.Win32.UI.XamlApplication.targets')" />
 
   <!-- This project will generate individual sxs manifests for each of our winrt libraries -->

src/cascadia/TerminalApp/ActionArgs.h

@@ -36,7 +36,7 @@ namespace winrt::TerminalApp::implementation
     struct CopyTextArgs : public CopyTextArgsT<CopyTextArgs>
     {
         CopyTextArgs() = default;
-        GETSET_PROPERTY(bool, TrimWhitespace, false);
+        GETSET_PROPERTY(bool, TrimWhitespace, true);
 
         static constexpr std::string_view TrimWhitespaceKey{ "trimWhitespace" };
 

src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp

@@ -237,7 +237,7 @@ std::function<IActionArgs(const Json::Value&)> LegacyParseSwitchToTabArgs(int in
 IActionArgs LegacyParseCopyTextWithoutNewlinesArgs(const Json::Value& /*json*/)
 {
     auto args = winrt::make_self<winrt::TerminalApp::implementation::CopyTextArgs>();
-    args->TrimWhitespace(true);
+    args->TrimWhitespace(false);
     return *args;
 };
 

src/cascadia/TerminalApp/MinMaxCloseControl.xaml

@@ -26,7 +26,7 @@ the MIT License. See LICENSE in the project root for license information. -->
                     <StaticResource x:Key="CaptionButtonStrokePointerOver" ResourceKey="SystemControlForegroundBaseHighBrush"/>
                     <StaticResource x:Key="CaptionButtonStrokePressed" ResourceKey="SystemControlForegroundBaseHighBrush"/>
                     <SolidColorBrush x:Key="CaptionButtonBackground" Color="Transparent" />
-                    <SolidColorBrush x:Key="CloseButtonBackgroundPointerOver" Color="Red"/>
+                    <SolidColorBrush x:Key="CloseButtonBackgroundPointerOver" Color="#e81123"/>
                     <SolidColorBrush x:Key="CloseButtonStrokePointerOver" Color="White"/>
                     <SolidColorBrush x:Key="CloseButtonBackgroundPressed" Color="#f1707a"/>
                     <SolidColorBrush x:Key="CloseButtonStrokePressed" Color="Black"/>
@@ -39,7 +39,7 @@ the MIT License. See LICENSE in the project root for license information. -->
                     <StaticResource x:Key="CaptionButtonStrokePointerOver" ResourceKey="SystemControlForegroundBaseHighBrush"/>
                     <StaticResource x:Key="CaptionButtonStrokePressed" ResourceKey="SystemControlForegroundBaseHighBrush"/>
                     <SolidColorBrush x:Key="CaptionButtonBackground" Color="Transparent" />
-                    <SolidColorBrush x:Key="CloseButtonBackgroundPointerOver" Color="Red"/>
+                    <SolidColorBrush x:Key="CloseButtonBackgroundPointerOver" Color="#e81123"/>
                     <SolidColorBrush x:Key="CloseButtonStrokePointerOver" Color="White"/>
                     <SolidColorBrush x:Key="CloseButtonBackgroundPressed" Color="#f1707a"/>
                     <SolidColorBrush x:Key="CloseButtonStrokePressed" Color="Black"/>
@@ -131,7 +131,7 @@ the MIT License. See LICENSE in the project root for license information. -->
         </ResourceDictionary>
     </StackPanel.Resources>
 
-    <Button Height="36.0" MinWidth="45.0" Width="45.0" x:Name="MinimizeButton" Style="{StaticResource CaptionButton}" Click="_MinimizeClick"
+    <Button Height="36.0" MinWidth="46.0" Width="46.0" x:Name="MinimizeButton" Style="{StaticResource CaptionButton}" Click="_MinimizeClick"
             AutomationProperties.Name="Minimize">
         <Button.Resources>
             <ResourceDictionary>
@@ -139,7 +139,7 @@ the MIT License. See LICENSE in the project root for license information. -->
             </ResourceDictionary>
         </Button.Resources>
     </Button>
-    <Button Height="36.0" MinWidth="45.0" Width="45.0" x:Name="MaximizeButton" Style="{StaticResource CaptionButton}" Click="_MaximizeClick"
+    <Button Height="36.0" MinWidth="46.0" Width="46.0" x:Name="MaximizeButton" Style="{StaticResource CaptionButton}" Click="_MaximizeClick"
             AutomationProperties.Name="Maximize">
         <Button.Resources>
             <ResourceDictionary>
@@ -148,7 +148,7 @@ the MIT License. See LICENSE in the project root for license information. -->
             </ResourceDictionary>
         </Button.Resources>
     </Button>
-    <Button Height="36.0" MinWidth="45.0" Width="45.0" x:Name="CloseButton" Style="{StaticResource CaptionButton}" Click="_CloseClick"
+    <Button Height="36.0" MinWidth="46.0" Width="46.0" x:Name="CloseButton" Style="{StaticResource CaptionButton}" Click="_CloseClick"
             AutomationProperties.Name="Close">
         <Button.Resources>
             <ResourceDictionary>

src/cascadia/TerminalApp/Tab.cpp

@@ -25,10 +25,6 @@ Tab::Tab(const GUID& profile, const TermControl& control)
 
     _activePane = _rootPane;
 
-    _AttachEventHandlersToPane(_rootPane);
-
-    _AttachEventHandlersToControl(control);
-
     _MakeTabViewItem();
 }
 
@@ -108,6 +104,19 @@ std::optional<GUID> Tab::GetFocusedProfile() const noexcept
     return _activePane->GetFocusedProfile();
 }
 
+// Method Description:
+// - Called after construction of a Tab object to bind event handlers to its
+//   associated Pane and TermControl object
+// Arguments:
+// - control: reference to the TermControl object to bind event to
+// Return Value:
+// - <none>
+void Tab::BindEventHandlers(const TermControl& control) noexcept
+{
+    _AttachEventHandlersToPane(_rootPane);
+    _AttachEventHandlersToControl(control);
+}
+
 // Method Description:
 // - Attempts to update the settings of this tab's tree of panes.
 // Arguments:
@@ -147,8 +156,13 @@ void Tab::UpdateIcon(const winrt::hstring iconPath)
 
     _lastIconPath = iconPath;
 
-    _tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this]() {
+    std::weak_ptr<Tab> weakThis{ shared_from_this() };
-        _tabViewItem.IconSource(GetColoredIcon<winrt::MUX::Controls::IconSource>(_lastIconPath));
+
+    _tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [weakThis]() {
+        if (auto tab{ weakThis.lock() })
+        {
+            tab->_tabViewItem.IconSource(GetColoredIcon<winrt::MUX::Controls::IconSource>(tab->_lastIconPath));
+        }
     });
 }
 
@@ -175,8 +189,13 @@ void Tab::SetTabText(const winrt::hstring& text)
 {
     // Copy the hstring, so we don't capture a dead reference
     winrt::hstring textCopy{ text };
-    _tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [text = std::move(textCopy), this]() {
+    std::weak_ptr<Tab> weakThis{ shared_from_this() };
-        _tabViewItem.Header(winrt::box_value(text));
+
+    _tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [text = std::move(textCopy), weakThis]() {
+        if (auto tab{ weakThis.lock() })
+        {
+            tab->_tabViewItem.Header(winrt::box_value(text));
+        }
     });
 }
 
@@ -296,12 +315,16 @@ void Tab::ClosePane()
 // - <none>
 void Tab::_AttachEventHandlersToControl(const TermControl& control)
 {
-    control.TitleChanged([this](auto newTitle) {
+    std::weak_ptr<Tab> weakThis{ shared_from_this() };
-        // The title of the control changed, but not necessarily the title
+
-        // of the tab. Get the title of the active pane of the tab, and set
+    control.TitleChanged([weakThis](auto newTitle) {
-        // the tab's text to the active panes' text.
+        // Check if Tab's lifetime has expired
-        auto newTabTitle = GetActiveTitle();
+        if (auto tab{ weakThis.lock() })
-        SetTabText(newTabTitle);
+        {
+            // The title of the control changed, but not necessarily the title of the tab.
+            // Set the tab's text to the active panes' text.
+            tab->SetTabText(tab->GetActiveTitle());
+        }
     });
 }
 
@@ -316,22 +339,24 @@ void Tab::_AttachEventHandlersToControl(const TermControl& control)
 // - <none>
 void Tab::_AttachEventHandlersToPane(std::shared_ptr<Pane> pane)
 {
-    pane->GotFocus([this](std::shared_ptr<Pane> sender) {
+    std::weak_ptr<Tab> weakThis{ shared_from_this() };
-        // Do nothing if it's the same pane as before.
+
-        if (sender == _activePane)
+    pane->GotFocus([weakThis](std::shared_ptr<Pane> sender) {
+        // Do nothing if the Tab's lifetime is expired or pane isn't new.
+        auto tab{ weakThis.lock() };
+        if (tab && sender != tab->_activePane)
         {
-            return;
-        }
             // Clear the active state of the entire tree, and mark only the sender as active.
-        _rootPane->ClearActive();
+            tab->_rootPane->ClearActive();
-        _activePane = sender;
+            tab->_activePane = sender;
-        _activePane->SetActive();
+            tab->_activePane->SetActive();
 
             // Update our own title text to match the newly-active pane.
-        SetTabText(GetActiveTitle());
+            tab->SetTabText(tab->GetActiveTitle());
 
             // Raise our own ActivePaneChanged event.
-        _ActivePaneChangedHandlers();
+            tab->_ActivePaneChangedHandlers();
+        }
     });
 }
 

src/cascadia/TerminalApp/Tab.h

@@ -5,11 +5,14 @@
 #include <winrt/Microsoft.UI.Xaml.Controls.h>
 #include "Pane.h"
 
-class Tab
+class Tab : public std::enable_shared_from_this<Tab>
 {
 public:
     Tab(const GUID& profile, const winrt::Microsoft::Terminal::TerminalControl::TermControl& control);
 
+    // Called after construction to setup events with weak_ptr
+    void BindEventHandlers(const winrt::Microsoft::Terminal::TerminalControl::TermControl& control) noexcept;
+
     winrt::Microsoft::UI::Xaml::Controls::TabViewItem GetTabViewItem();
     winrt::Windows::UI::Xaml::UIElement GetRootElement();
     winrt::Microsoft::Terminal::TerminalControl::TermControl GetActiveTerminalControl() const;

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -60,23 +60,36 @@ namespace winrt::TerminalApp::implementation
         _tabView = _tabRow.TabView();
         _rearranging = false;
 
-        _tabView.TabDragStarting([this](auto&& /*o*/, auto&& /*a*/) {
+        // weak_ptr to this TerminalPage object lambda capturing
-            _rearranging = true;
+        auto weakThis{ get_weak() };
-            _rearrangeFrom = std::nullopt;
+
-            _rearrangeTo = std::nullopt;
+        _tabView.TabDragStarting([weakThis](auto&& /*o*/, auto&& /*a*/) {
+            if (auto page{ weakThis.get() })
+            {
+                page->_rearranging = true;
+                page->_rearrangeFrom = std::nullopt;
+                page->_rearrangeTo = std::nullopt;
+            }
         });
 
-        _tabView.TabDragCompleted([this](auto&& /*o*/, auto&& /*a*/) {
+        _tabView.TabDragCompleted([weakThis](auto&& /*o*/, auto&& /*a*/) {
-            if (_rearrangeFrom.has_value() && _rearrangeTo.has_value() && _rearrangeTo != _rearrangeFrom)
+            if (auto page{ weakThis.get() })
             {
-                auto tab = _tabs.at(_rearrangeFrom.value());
+                auto& from{ page->_rearrangeFrom };
-                _tabs.erase(_tabs.begin() + _rearrangeFrom.value());
+                auto& to{ page->_rearrangeTo };
-                _tabs.insert(_tabs.begin() + _rearrangeTo.value(), tab);
+
+                if (from.has_value() && to.has_value() && to != from)
+                {
+                    auto& tabs{ page->_tabs };
+                    auto tab = tabs.at(from.value());
+                    tabs.erase(tabs.begin() + from.value());
+                    tabs.insert(tabs.begin() + to.value(), tab);
                 }
 
-            _rearranging = false;
+                page->_rearranging = false;
-            _rearrangeFrom = std::nullopt;
+                from = std::nullopt;
-            _rearrangeTo = std::nullopt;
+                to = std::nullopt;
+            }
         });
 
         auto tabRowImpl = winrt::get_self<implementation::TabRowControl>(_tabRow);
@@ -97,8 +110,11 @@ namespace winrt::TerminalApp::implementation
         }
 
         //Event Bindings (Early)
-        _newTabButton.Click([this](auto&&, auto&&) {
+        _newTabButton.Click([weakThis](auto&&, auto&&) {
-            this->_OpenNewTab(std::nullopt);
+            if (auto page{ weakThis.get() })
+            {
+                page->_OpenNewTab(std::nullopt);
+            }
         });
         _tabView.SelectionChanged({ this, &TerminalPage::_OnTabSelectionChanged });
         _tabView.TabCloseRequested({ this, &TerminalPage::_OnTabCloseRequested });
@@ -298,8 +314,13 @@ namespace winrt::TerminalApp::implementation
                 profileMenuItem.FontWeight(FontWeights::Bold());
             }
 
-            profileMenuItem.Click([this, profileIndex](auto&&, auto&&) {
+            auto weakThis{ get_weak() };
-                this->_OpenNewTab({ profileIndex });
+
+            profileMenuItem.Click([profileIndex, weakThis](auto&&, auto&&) {
+                if (auto page{ weakThis.get() })
+                {
+                    page->_OpenNewTab({ profileIndex });
+                }
             });
             newTabFlyout.Items().Append(profileMenuItem);
         }
@@ -437,17 +458,21 @@ namespace winrt::TerminalApp::implementation
         // Don't capture a strong ref to the tab. If the tab is removed as this
         // is called, we don't really care anymore about handling the event.
         std::weak_ptr<Tab> weakTabPtr = newTab;
+        auto weakThis{ get_weak() };
+
         // When the tab's active pane changes, we'll want to lookup a new icon
         // for it, and possibly propogate the title up to the window.
-        newTab->ActivePaneChanged([this, weakTabPtr]() {
+        newTab->ActivePaneChanged([weakTabPtr, weakThis]() {
-            if (auto tab = weakTabPtr.lock())
+            auto page{ weakThis.get() };
+            auto tab{ weakTabPtr.lock() };
+
+            if (page && tab)
             {
                 // Possibly update the icon of the tab.
-                _UpdateTabIcon(tab);
+                page->_UpdateTabIcon(tab);
-
                 // Possibly update the title of the tab, window to match the newly
                 // focused pane.
-                _UpdateTitle(tab);
+                page->_UpdateTitle(tab);
             }
         });
 
@@ -464,10 +489,16 @@ namespace winrt::TerminalApp::implementation
         tabViewItem.PointerPressed({ this, &TerminalPage::_OnTabClick });
 
         // When the tab is closed, remove it from our list of tabs.
-        newTab->Closed([tabViewItem, this]() {
+        newTab->Closed([tabViewItem, weakThis]() {
-            _tabView.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [tabViewItem, this]() {
+            if (auto page{ weakThis.get() })
-                _RemoveTabViewItem(tabViewItem);
+            {
+                page->_tabView.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [tabViewItem, weakThis]() {
+                    if (auto page{ weakThis.get() })
+                    {
+                        page->_RemoveTabViewItem(tabViewItem);
+                    }
                 });
+            }
         });
 
         // This is one way to set the tab's selected background color.
@@ -750,19 +781,25 @@ namespace winrt::TerminalApp::implementation
         // Add an event handler when the terminal wants to paste data from the Clipboard.
         term.PasteFromClipboard({ this, &TerminalPage::_PasteFromClipboardHandler });
 
+        // Bind Tab events to the TermControl and the Tab's Pane
+        hostingTab->BindEventHandlers(term);
+
         // Don't capture a strong ref to the tab. If the tab is removed as this
         // is called, we don't really care anymore about handling the event.
         std::weak_ptr<Tab> weakTabPtr = hostingTab;
-        term.TitleChanged([this, weakTabPtr](auto newTitle) {
+        auto weakThis{ get_weak() };
-            auto tab = weakTabPtr.lock();
+
-            if (!tab)
+        term.TitleChanged([weakTabPtr, weakThis](auto newTitle) {
+            auto page{ weakThis.get() };
+            auto tab{ weakTabPtr.lock() };
+
+            if (page && tab)
             {
-                return;
-            }
                 // The title of the control changed, but not necessarily the title
                 // of the tab. Get the title of the focused pane of the tab, and set
                 // the tab's text to the focused panes' text.
-            _UpdateTitle(tab);
+                page->_UpdateTitle(tab);
+            }
         });
     }
 
@@ -837,9 +874,12 @@ namespace winrt::TerminalApp::implementation
         // GH#1117: This is a workaround because _tabView.SelectedIndex(tabIndex)
         //          sometimes set focus to an incorrect tab after removing some tabs
         auto tab = _tabs.at(tabIndex);
-        _tabView.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [tab, this]() {
+        auto weakThis{ get_weak() };
-            auto tabViewItem = tab->GetTabViewItem();
+        _tabView.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [tab, weakThis]() {
-            _tabView.SelectedItem(tabViewItem);
+            if (auto page{ weakThis.get() })
+            {
+                page->_tabView.SelectedItem(tab->GetTabViewItem());
+            }
         });
     }
 
@@ -1360,10 +1400,14 @@ namespace winrt::TerminalApp::implementation
             _UpdateTitle(tab);
         }
 
-        this->Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this]() {
+        auto weakThis{ get_weak() };
+        this->Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [weakThis]() {
             // repopulate the new tab button's flyout with entries for each
             // profile, which might have changed
-            _CreateNewTabFlyout();
+            if (auto page{ weakThis.get() })
+            {
+                page->_CreateNewTabFlyout();
+            }
         });
     }
 

src/cascadia/TerminalApp/lib/TerminalAppLib.vcxproj

@@ -286,7 +286,7 @@
   <PropertyGroup>
     <Native-Platform Condition="'$(Platform)' == 'Win32'">x86</Native-Platform>
     <Native-Platform Condition="'$(Platform)' != 'Win32'">$(Platform)</Native-Platform>
-    <_MUXRoot>$(OpenConsoleDir)\packages\Microsoft.UI.Xaml.2.2.191021001-prerelease\</_MUXRoot>
+    <_MUXRoot>$(OpenConsoleDir)\packages\Microsoft.UI.Xaml.2.2.191203001-prerelease\</_MUXRoot>
     <_MUXAppRoot>$(OpenConsoleDir)\packages\Microsoft.Toolkit.Win32.UI.XamlApplication.6.0.0-preview7\</_MUXAppRoot>
   </PropertyGroup>
   <ItemGroup>
@@ -316,7 +316,7 @@
     <PropertyGroup>
       <ErrorText>This project references NuGet package(s) that are missing on this computer. Use NuGet Package Restore to download them.  For more information, see http://go.microsoft.com/fwlink/?LinkID=322105. The missing file is {0}.</ErrorText>
     </PropertyGroup>
-    <Error Condition="!Exists('$(OpenConsoleDir)\packages\Microsoft.UI.Xaml.2.2.191021001-prerelease\build\native\Microsoft.UI.Xaml.targets')" Text="$([System.String]::Format('$(ErrorText)', '$(OpenConsoleDir)\packages\Microsoft.UI.Xaml.2.2.191021001-prerelease\build\native\Microsoft.UI.Xaml.targets'))" />
+    <Error Condition="!Exists('$(OpenConsoleDir)\packages\Microsoft.UI.Xaml.2.2.191203001-prerelease\build\native\Microsoft.UI.Xaml.targets')" Text="$([System.String]::Format('$(ErrorText)', '$(OpenConsoleDir)\packages\Microsoft.UI.Xaml.2.2.191203001-prerelease\build\native\Microsoft.UI.Xaml.targets'))" />
     <Error Condition="!Exists('$(OpenConsoleDir)\packages\Microsoft.Toolkit.Win32.UI.XamlApplication.6.0.0-preview7\build\native\Microsoft.Toolkit.Win32.UI.XamlApplication.targets')" Text="$([System.String]::Format('$(ErrorText)', '$(OpenConsoleDir)\packages\Microsoft.Toolkit.Win32.UI.XamlApplication.6.0.0-preview7\build\native\Microsoft.Toolkit.Win32.UI.XamlApplication.targets'))" />
   </Target>
 

src/cascadia/TerminalApp/packages.config

@@ -1,6 +1,6 @@
 <?xml version="1.0" encoding="utf-8"?>
 <packages>
   <package id="Microsoft.Toolkit.Win32.UI.XamlApplication" version="6.0.0-preview7" targetFramework="native" />
-  <package id="Microsoft.UI.Xaml" version="2.2.191021001-prerelease" targetFramework="native" />
+  <package id="Microsoft.UI.Xaml" version="2.2.191203001-prerelease" targetFramework="native" />
   <package id="Microsoft.Windows.CppWinRT" version="2.0.190730.2" targetFramework="native" />
 </packages>

src/cascadia/TerminalCore/Terminal.cpp

@@ -87,10 +87,16 @@ void Terminal::CreateFromSettings(winrt::Microsoft::Terminal::Settings::ICoreSet
 {
     const COORD viewportSize{ Utils::ClampToShortMax(settings.InitialCols(), 1),
                               Utils::ClampToShortMax(settings.InitialRows(), 1) };
+
     // TODO:MSFT:20642297 - Support infinite scrollback here, if HistorySize is -1
     Create(viewportSize, Utils::ClampToShortMax(settings.HistorySize(), 0), renderTarget);
 
     UpdateSettings(settings);
+
+    if (_suppressApplicationTitle)
+    {
+        _title = _startingTitle;
+    }
 }
 
 // Method Description:

src/cascadia/TerminalCore/TerminalApi.cpp

@@ -365,25 +365,9 @@ bool Terminal::EraseInDisplay(const DispatchTypes::EraseType eraseType)
 
 bool Terminal::SetWindowTitle(std::wstring_view title)
 {
-    // Set the title on Terminal load
+    _title = _suppressApplicationTitle ? _startingTitle : title;
-    if (_title.empty())
-    {
-        _title = title;
-        _pfnTitleChanged(title);
-    }
 
-    _title = title;
-
-    // If this is removed, the tab object assumes the application title is the title
-    if (_suppressApplicationTitle)
-    {
-        _title = _startingTitle;
-    }
-
-    if (_pfnTitleChanged && !_suppressApplicationTitle)
-    {
     _pfnTitleChanged(_title);
-    }
 
     return true;
 }

src/cascadia/TerminalCore/TerminalSelection.cpp

@@ -377,7 +377,7 @@ COORD Terminal::_ExpandDoubleClickSelectionLeft(const COORD position) const
     while (positionWithOffsets.X > bufferViewport.Left() && (_GetDelimiterClass(*bufferIterator) == startedOnDelimiter))
     {
         bufferViewport.DecrementInBounds(positionWithOffsets);
-        bufferIterator--;
+        --bufferIterator;
     }
 
     if (_GetDelimiterClass(*bufferIterator) != startedOnDelimiter)
@@ -415,7 +415,7 @@ COORD Terminal::_ExpandDoubleClickSelectionRight(const COORD position) const
     while (positionWithOffsets.X < bufferViewport.RightInclusive() && (_GetDelimiterClass(*bufferIterator) == startedOnDelimiter))
     {
         bufferViewport.IncrementInBounds(positionWithOffsets);
-        bufferIterator++;
+        ++bufferIterator;
     }
 
     if (_GetDelimiterClass(*bufferIterator) != startedOnDelimiter)

src/cascadia/WinRTUtils/inc/Utils.h

@@ -28,7 +28,7 @@ inline winrt::Windows::UI::Color ColorRefToColor(const COLORREF& colorref)
 // - Rect scaled by scale
 inline winrt::Windows::Foundation::Rect ScaleRect(winrt::Windows::Foundation::Rect rect, double scale)
 {
-    const float scaleLocal = gsl::narrow<float>(scale);
+    const float scaleLocal = gsl::narrow_cast<float>(scale);
     rect.X *= scaleLocal;
     rect.Y *= scaleLocal;
     rect.Width *= scaleLocal;

src/cascadia/WindowsTerminal/WindowsTerminal.vcxproj

@@ -101,14 +101,14 @@
 
   <Import Project="$(OpenConsoleDir)src\cppwinrt.build.post.props" />
 
-  <Import Project="..\..\..\packages\Microsoft.UI.Xaml.2.2.191021001-prerelease\build\native\Microsoft.UI.Xaml.targets" Condition="Exists('..\..\..\packages\Microsoft.UI.Xaml.2.2.191021001-prerelease\build\native\Microsoft.UI.Xaml.targets')" />
+  <Import Project="..\..\..\packages\Microsoft.UI.Xaml.2.2.191203001-prerelease\build\native\Microsoft.UI.Xaml.targets" Condition="Exists('..\..\..\packages\Microsoft.UI.Xaml.2.2.191203001-prerelease\build\native\Microsoft.UI.Xaml.targets')" />
   <Import Project="..\..\..\packages\Microsoft.Toolkit.Win32.UI.XamlApplication.6.0.0-preview7\build\native\Microsoft.Toolkit.Win32.UI.XamlApplication.targets" Condition="Exists('..\..\..\packages\Microsoft.Toolkit.Win32.UI.XamlApplication.6.0.0-preview7\build\native\Microsoft.Toolkit.Win32.UI.XamlApplication.targets')" />
   <Import Project="..\..\..\packages\Microsoft.VCRTForwarders.140.1.0.1-rc\build\native\Microsoft.VCRTForwarders.140.targets" Condition="Exists('..\..\..\packages\Microsoft.VCRTForwarders.140.1.0.1-rc\build\native\Microsoft.VCRTForwarders.140.targets')" />
   <Target Name="EnsureNuGetPackageBuildImports" BeforeTargets="PrepareForBuild">
     <PropertyGroup>
       <ErrorText>This project references NuGet package(s) that are missing on this computer. Use NuGet Package Restore to download them.  For more information, see http://go.microsoft.com/fwlink/?LinkID=322105. The missing file is {0}.</ErrorText>
     </PropertyGroup>
-    <Error Condition="!Exists('..\..\..\packages\Microsoft.UI.Xaml.2.2.191021001-prerelease\build\native\Microsoft.UI.Xaml.targets')" Text="$([System.String]::Format('$(ErrorText)', '..\..\..\packages\Microsoft.UI.Xaml.2.2.191021001-prerelease\build\native\Microsoft.UI.Xaml.targets'))" />
+    <Error Condition="!Exists('..\..\..\packages\Microsoft.UI.Xaml.2.2.191203001-prerelease\build\native\Microsoft.UI.Xaml.targets')" Text="$([System.String]::Format('$(ErrorText)', '..\..\..\packages\Microsoft.UI.Xaml.2.2.191203001-prerelease\build\native\Microsoft.UI.Xaml.targets'))" />
     <Error Condition="!Exists('..\..\..\packages\Microsoft.Toolkit.Win32.UI.XamlApplication.6.0.0-preview7\build\native\Microsoft.Toolkit.Win32.UI.XamlApplication.props')" Text="$([System.String]::Format('$(ErrorText)', '..\..\..\packages\Microsoft.Toolkit.Win32.UI.XamlApplication.6.0.0-preview7\build\native\Microsoft.Toolkit.Win32.UI.XamlApplication.props'))" />
     <Error Condition="!Exists('..\..\..\packages\Microsoft.Toolkit.Win32.UI.XamlApplication.6.0.0-preview7\build\native\Microsoft.Toolkit.Win32.UI.XamlApplication.targets')" Text="$([System.String]::Format('$(ErrorText)', '..\..\..\packages\Microsoft.Toolkit.Win32.UI.XamlApplication.6.0.0-preview7\build\native\Microsoft.Toolkit.Win32.UI.XamlApplication.targets'))" />
     <Error Condition="!Exists('..\..\..\packages\Microsoft.VCRTForwarders.140.1.0.1-rc\build\native\Microsoft.VCRTForwarders.140.targets')" Text="$([System.String]::Format('$(ErrorText)', '..\..\..\packages\Microsoft.VCRTForwarders.140.1.0.1-rc\build\native\Microsoft.VCRTForwarders.140.targets'))" />

src/cascadia/WindowsTerminal/packages.config

@@ -2,6 +2,6 @@
 <packages>
   <package id="Microsoft.Windows.CppWinRT" version="2.0.190730.2" targetFramework="native" />
   <package id="Microsoft.Toolkit.Win32.UI.XamlApplication" version="6.0.0-preview7" targetFramework="native" />
-  <package id="Microsoft.UI.Xaml" version="2.2.191021001-prerelease" targetFramework="native" />
+  <package id="Microsoft.UI.Xaml" version="2.2.191203001-prerelease" targetFramework="native" />
   <package id="Microsoft.VCRTForwarders.140" version="1.0.1-rc" targetFramework="native" />
 </packages>

src/cascadia/ut_app/TerminalApp.UnitTests.vcxproj

@@ -85,7 +85,7 @@
     <!-- From Microsoft.UI.Xaml.targets -->
     <Native-Platform Condition="'$(Platform)' == 'Win32'">x86</Native-Platform>
     <Native-Platform Condition="'$(Platform)' != 'Win32'">$(Platform)</Native-Platform>
-    <_MUXBinRoot>&quot;$(OpenConsoleDir)packages\Microsoft.UI.Xaml.2.2.191021001-prerelease\runtimes\win10-$(Native-Platform)\native\&quot;</_MUXBinRoot>
+    <_MUXBinRoot>&quot;$(OpenConsoleDir)packages\Microsoft.UI.Xaml.2.2.191203001-prerelease\runtimes\win10-$(Native-Platform)\native\&quot;</_MUXBinRoot>
   </PropertyGroup>
 
   <ItemDefinitionGroup>

src/host/ut_host/AttrRowTests.cpp

@@ -498,6 +498,135 @@ class AttrRowTests
         }
     }
 
+    TEST_METHOD(TestReverseIteratorWalkFromMiddle)
+    {
+        // GH #3409, walking backwards through color range runs out of bounds
+        // We're going to create an attribute row with assorted colors and varying lengths
+        // just like the row of text on the Ubuntu prompt line that triggered this bug being found.
+        // Then we're going to walk backwards through the iterator like a selection-expand-to-left
+        // operation and ensure we don't run off the bounds.
+
+        // walk the chain, from index, stepSize at a time
+        // ensure we don't crash
+        auto testWalk = [](ATTR_ROW* chain, size_t index, int stepSize) {
+            // move to starting index
+            auto iter = chain->cbegin();
+            iter += index;
+
+            // Now walk backwards in a loop until 0.
+            while (iter)
+            {
+                iter -= stepSize;
+            }
+
+            Log::Comment(L"We made it through without crashing!");
+        };
+
+        // take one step of size stepSize on the chain
+        // index is where we start from
+        // expectedAttribute is what we expect to read here
+        auto verifyStep = [](ATTR_ROW* chain, size_t index, int stepSize, TextAttribute expectedAttribute) {
+            // move to starting index
+            auto iter = chain->cbegin();
+            iter += index;
+
+            // Now step backwards
+            iter -= stepSize;
+
+            VERIFY_ARE_EQUAL(expectedAttribute, *iter);
+        };
+
+        Log::Comment(L"Reverse iterate through ubuntu prompt");
+        {
+            // Create attr row representing a buffer that's 121 wide.
+            auto chain = std::make_unique<ATTR_ROW>(121, _DefaultAttr);
+
+            // The repro case had 4 chain segments.
+            chain->_list.resize(4);
+
+            // The color 10 went for the first 18.
+            chain->_list[0].SetAttributes(TextAttribute(0xA));
+            chain->_list[0].SetLength(18);
+
+            // Default color for the next 1
+            chain->_list[1].SetAttributes(TextAttribute());
+            chain->_list[1].SetLength(1);
+
+            // Color 12 for the next 29
+            chain->_list[2].SetAttributes(TextAttribute(0xC));
+            chain->_list[2].SetLength(29);
+
+            // Then default color to end the run
+            chain->_list[3].SetAttributes(TextAttribute());
+            chain->_list[3].SetLength(73);
+
+            // The sum of the lengths should be 121.
+            VERIFY_ARE_EQUAL(chain->_cchRowWidth, chain->_list[0]._cchLength + chain->_list[1]._cchLength + chain->_list[2]._cchLength + chain->_list[3]._cchLength);
+
+            auto index = chain->_list[0].GetLength();
+            auto stepSize = 1;
+            testWalk(chain.get(), index, stepSize);
+        }
+
+        Log::Comment(L"Reverse iterate across a text run in the chain");
+        {
+            // Create attr row representing a buffer that's 3 wide.
+            auto chain = std::make_unique<ATTR_ROW>(3, _DefaultAttr);
+
+            // The repro case had 3 chain segments.
+            chain->_list.resize(3);
+
+            // The color 10 went for the first 1.
+            chain->_list[0].SetAttributes(TextAttribute(0xA));
+            chain->_list[0].SetLength(1);
+
+            // The color 11 for the next 1
+            chain->_list[1].SetAttributes(TextAttribute(0xB));
+            chain->_list[1].SetLength(1);
+
+            // Color 12 for the next 1
+            chain->_list[2].SetAttributes(TextAttribute(0xC));
+            chain->_list[2].SetLength(1);
+
+            // The sum of the lengths should be 3.
+            VERIFY_ARE_EQUAL(chain->_cchRowWidth, chain->_list[0]._cchLength + chain->_list[1]._cchLength + chain->_list[2]._cchLength);
+
+            // on 'ABC', step from B to A
+            auto index = 1;
+            auto stepSize = 1;
+            verifyStep(chain.get(), index, stepSize, TextAttribute(0xA));
+        }
+
+        Log::Comment(L"Reverse iterate across two text runs in the chain");
+        {
+            // Create attr row representing a buffer that's 3 wide.
+            auto chain = std::make_unique<ATTR_ROW>(3, _DefaultAttr);
+
+            // The repro case had 3 chain segments.
+            chain->_list.resize(3);
+
+            // The color 10 went for the first 1.
+            chain->_list[0].SetAttributes(TextAttribute(0xA));
+            chain->_list[0].SetLength(1);
+
+            // The color 11 for the next 1
+            chain->_list[1].SetAttributes(TextAttribute(0xB));
+            chain->_list[1].SetLength(1);
+
+            // Color 12 for the next 1
+            chain->_list[2].SetAttributes(TextAttribute(0xC));
+            chain->_list[2].SetLength(1);
+
+            // The sum of the lengths should be 3.
+            VERIFY_ARE_EQUAL(chain->_cchRowWidth, chain->_list[0]._cchLength + chain->_list[1]._cchLength + chain->_list[2]._cchLength);
+
+            // on 'ABC', step from C to A
+            auto index = 2;
+            auto stepSize = 2;
+            verifyStep(chain.get(), index, stepSize, TextAttribute(0xA));
+        }
+    }
+
     TEST_METHOD(TestSetAttrToEnd)
     {
         const WORD wTestAttr = FOREGROUND_BLUE | BACKGROUND_GREEN;

src/renderer/dx/DxRenderer.cpp

@@ -41,7 +41,7 @@ DxEngine::DxEngine() :
     _displaySizePixels{ 0 },
     _foregroundColor{ 0 },
     _backgroundColor{ 0 },
-    _selectionBackground{ DEFAULT_FOREGROUND },
+    _selectionBackground{},
     _glyphCell{ 0 },
     _haveDeviceResources{ false },
     _hwndTarget{ static_cast<HWND>(INVALID_HANDLE_VALUE) },
@@ -57,6 +57,10 @@ DxEngine::DxEngine() :
         DWRITE_FACTORY_TYPE_SHARED,
         __uuidof(_dwriteFactory),
         reinterpret_cast<IUnknown**>(_dwriteFactory.GetAddressOf())));
+
+    // Initialize our default selection color to DEFAULT_FOREGROUND, but make
+    // sure to set to to a D2D1::ColorF
+    SetSelectionBackground(DEFAULT_FOREGROUND);
 }
 
 // Routine Description:

tools/ConsoleTypes.natvis

@@ -12,16 +12,19 @@
         <!-- You can't do too much trickyness inside the DisplayString format
                 string, so we'd have to add entries for each flag if we really
                 wanted them to show up like that. -->
-        <DisplayString Condition="_isBold">{{FG:{_foreground},BG:{_background},{_wAttrLegacy}, Bold}}</DisplayString>
+        <DisplayString>{{FG: {_foreground}, BG: {_background}, Legacy: {_wAttrLegacy}, {_extendedAttrs}}</DisplayString>
-        <DisplayString>{{FG:{_foreground},BG:{_background},{_wAttrLegacy}, Normal}}</DisplayString>
         <Expand>
-            <Item Name="_wAttrLegacy">_wAttrLegacy</Item>
+            <Item Name="Legacy">_wAttrLegacy</Item>
-            <Item Name="_isBold">_isBold</Item>
+            <Item Name="FG">_foreground</Item>
-            <Item Name="_foreground">_foreground</Item>
+            <Item Name="BG">_background</Item>
-            <Item Name="_background">_background</Item>
+            <Item Name="Extended">_extendedAttrs</Item>
         </Expand>
     </Type>
 
+    <Type Name="TextAttributeRun">
+        <DisplayString>Length={_cchLength} Attr={_attributes}</DisplayString>
+    </Type>
+
     <Type Name="Microsoft::Console::Types::Viewport">
         <!-- Can't call functions in here -->
         <DisplayString>{{LT({_sr.Left}, {_sr.Top}) RB({_sr.Right}, {_sr.Bottom}) [{_sr.Right-_sr.Left+1} x { _sr.Bottom-_sr.Top+1}]}}</DisplayString>