Display connection state in tab and add context menu entry for restarting connection

[mpela81]

Summary of the Pull Request

When a connection is Closed, show an indicator in the respective tab.
When the active pane's connection is Closed, show a "Restart Connection" action in the right-click context menu and in the tab context menu.

Validation Steps Performed

• Force close a connection, check the indicator is shown in the tab.
• Right-click on pane shows the Restart Connection action if its connection is closed
• Right-click on tab shows the Restart Connection action if the active pane's connection is closed
• Indicator is cleared after connection is restarted (no panes in closed state)

PR Checklist

• Closes Display connection state in tab & add tab context menu entry for restarting connection #14909
• Tests added/passed
• Documentation updated
• Schema updated (if necessary)

[mpela81]

Please suggest better font icons if any 😄

[zadjii-msft]

Drive by notes:

1. This is slick
2. Let's make sure this works even in the case that the user right-clicks on an inactive tab to restart it - we don't want another "Move Tab to New Window" context menu item uses focused tab #15734 situation
3. Icon is definitely okay, but you're right, maybe we should take another trawl through the segoe fluent icons and see if there's anything better (I doubt it though)

[lhecker]

Further options:

• E871:
  
• E8CD:
  
• EA14:
  

[mpela81]

2. Let's make sure this works even in the case that the user right-clicks on an inactive tab to restart it - we don't want another ["Move Tab to New Window" context menu item uses focused tab #15734](https://github.com/microsoft/terminal/issues/15734) situation

@zadjii-msft Indeed we have the same problem here. Do you have an agreed way to solve this kind of issue?

Actually, I've also noticed another sort of race condition here. When you right-click on a non-active pane, it becomes active, but the context menu may be displayed before this happens (I suppose these events are asynchronous), thus showing the Restart Connection item based the wrong pane's status. Not sure how to fix it either.

[zadjii-msft]

Do you have an agreed way to solve this kind of issue

I'm literally working on that right now.

I believe the code we have right now just manually tries to focus the tab that requested the action, then does the action. Something like

terminal/src/cascadia/TerminalApp/TabManagement.cpp

Lines 191 to 201 in d70794a

	newTabImpl->MoveTabToNewWindowRequested([weakTab, weakThis{ get_weak() }]() {
	auto page{ weakThis.get() };
	auto tab{ weakTab.get() };
	if (page && tab)
	{
	MoveTabArgs args{ hstring{ L"new" }, MoveTabDirection::Forward };
	page->_SetFocusedTab(*tab);
	page->_MoveTab(args);
	}
	});

That seems... unnecessarily wacky.

I'm trying to rewrite bits of the TerminalTab so we don't always have to be raising events requesting the page to then do something with the tab. I want to just be able to fire off the ActionAndArgs, bundled with the sender, and have the handlers figure out who to call it on.

[DHowett]

don't let the dev lead design icons

However: this is a rough sketch of what I envision for the disconnected state icon. We can insert custom SVG, just like we did for the "Case Sensitive" button in Search, if we choose to go somewhere other than Segoe MDL.

(It could use some refinement to match the stroke sizes of the segoe icons, and also to be good)

[DHowett]

(I am still wasting time on this!)

[zadjii-msft]

FWIW #15773 should merge after CI runs, and that should make this a little easier ☺️

[mpela81]

There is still this small race condition I don't know how to fix

When you right-click on a non-active pane, it becomes active, but the context menu may be displayed before this happens, thus showing the Restart Connection item based the wrong pane's status.

[mpela81]

When you right-click on a non-active pane, it becomes active, but the context menu may be displayed before this happens, thus showing the Restart Connection item based the wrong pane's status.

As far as I can see, when a pane is (right)clicked:

1. If unfocused, Focus is called. This goes through the GotFocus handler which eventually calls tab->_UpdateActivePane(sender);
2. PointerPressed is raised which eventually shows the context menu

The first point is done asynchronously, so may update the active pane too late when the menu is already displayed (despite both end up in the UI thread).

terminal/src/cascadia/TerminalControl/TermControl.cpp

Lines 1472 to 1520 in 8daf89e

	void TermControl::_PointerPressedHandler(const Windows::Foundation::IInspectable& sender,
	const Input::PointerRoutedEventArgs& args)
	{
	if (_IsClosing())
	{
	return;
	}
	_RestorePointerCursorHandlers(*this, nullptr);
	_CapturePointer(sender, args);
	const auto ptr = args.Pointer();
	const auto point = args.GetCurrentPoint(*this);
	const auto type = ptr.PointerDeviceType();
	// We also TryShow in GotFocusHandler, but this call is specifically
	// for the case where the Terminal is in focus but the user closed the
	// on-screen keyboard. This lets the user simply tap on the terminal
	// again to bring it up.
	InputPane::GetForCurrentView().TryShow();
	if (!_focused)
	{
	Focus(FocusState::Pointer);
	}
	// Mark that this pointer event actually started within our bounds.
	// We'll need this later, for PointerMoved events.
	_pointerPressedInBounds = true;
	if (type == Windows::Devices::Input::PointerDeviceType::Touch)
	{
	const auto contactRect = point.Properties().ContactRect();
	auto anchor = til::point{ til::math::rounding, contactRect.X, contactRect.Y };
	_interactivity.TouchPressed(anchor.to_core_point());
	}
	else
	{
	const auto cursorPosition = point.Position();
	_interactivity.PointerPressed(TermControl::GetPressedMouseButtons(point),
	TermControl::GetPointerUpdateKind(point),
	point.Timestamp(),
	ControlKeyStates{ args.KeyModifiers() },
	_toTerminalOrigin(cursorPosition).to_core_point());
	}
	args.Handled(true);
	}

@zadjii-msft @lhecker Free to comment if you see how we could resolve this, otherwise if you feel that it would require quite a big change for this PR I can just leave it to you 😄

[zadjii-msft]

How do we feel about

diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp
index f7edf6397..59ad78e23 100644
--- a/src/cascadia/TerminalApp/TerminalPage.cpp
+++ b/src/cascadia/TerminalApp/TerminalPage.cpp
@@ -1661,8 +1661,18 @@ namespace winrt::TerminalApp::implementation
             term.CompletionsChanged({ get_weak(), &TerminalPage::_ControlCompletionsChangedHandler });
         }

-        term.ContextMenu().Opening({ this, &TerminalPage::_ContextMenuOpened });
-        term.SelectionContextMenu().Opening({ this, &TerminalPage::_SelectionMenuOpened });
+        term.ContextMenu().Opening([weak = get_weak(), weakTerm = term.get_weak()](auto&& sender, auto&& args) {
+            if (const auto& page{ weak.get() })
+            {
+                _PopulateContextMenu(weakTerm.get(), sender.try_as<MUX::Controls::CommandBarFlyout>(), false);
+            }
+        });
+        term.SelectionContextMenu().Opening([weak = get_weak(), weakTerm = term.get_weak()](auto&& sender, auto&& args) {
+            if (const auto& page{ weak.get() })
+            {
+                _PopulateContextMenu(weakTerm.get(), sender.try_as<MUX::Controls::CommandBarFlyout>(), true);
+            }
+        });
     }

     // Method Description:
@@ -4801,25 +4811,14 @@ namespace winrt::TerminalApp::implementation
                    characterSize.Height);
     }

-    void TerminalPage::_ContextMenuOpened(const IInspectable& sender,
-                                          const IInspectable& /*args*/)
-    {
-        _PopulateContextMenu(sender, false /*withSelection*/);
-    }
-    void TerminalPage::_SelectionMenuOpened(const IInspectable& sender,
-                                            const IInspectable& /*args*/)
-    {
-        _PopulateContextMenu(sender, true /*withSelection*/);
-    }
-
-    void TerminalPage::_PopulateContextMenu(const IInspectable& sender,
+    void TerminalPage::_PopulateContextMenu(const TermControl& control,
+                                            const MUX::Controls::CommandBarFlyout& menu,
                                             const bool withSelection)
     {
         // withSelection can be used to add actions that only appear if there's
         // selected text, like "search the web"

-        const auto& menu{ sender.try_as<MUX::Controls::CommandBarFlyout>() };
-        if (!menu)
+        if (!control || !menu)
         {
             return;
         }
@@ -4869,12 +4868,9 @@ namespace winrt::TerminalApp::implementation
             makeItem(RS_(L"PaneClose"), L"\xE89F", ActionAndArgs{ ShortcutAction::ClosePane, nullptr });
         }

-        if (const auto pane{ _GetFocusedTabImpl()->GetActivePane() })
+        if (control.ConnectionState() >= ConnectionState::Closed)
         {
-            if (pane->IsConnectionClosed())
-            {
-                makeItem(RS_(L"RestartConnectionText"), L"\xE72C", ActionAndArgs{ ShortcutAction::RestartConnection, nullptr });
-            }
+            makeItem(RS_(L"RestartConnectionText"), L"\xE72C", ActionAndArgs{ ShortcutAction::RestartConnection, nullptr });
         }

         if (withSelection)
diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h
index fd63f7cc6..d70d09ded 100644
--- a/src/cascadia/TerminalApp/TerminalPage.h
+++ b/src/cascadia/TerminalApp/TerminalPage.h
@@ -535,9 +535,7 @@ namespace winrt::TerminalApp::implementation
                           const std::optional<til::point>& dragPoint = std::nullopt);
         void _sendDraggedTabToWindow(const winrt::hstring& windowId, const uint32_t tabIndex, std::optional<til::point> dragPoint);

-        void _ContextMenuOpened(const IInspectable& sender, const IInspectable& args);
-        void _SelectionMenuOpened(const IInspectable& sender, const IInspectable& args);
-        void _PopulateContextMenu(const IInspectable& sender, const bool withSelection);
+        void _PopulateContextMenu(const Microsoft::Terminal::Control::TermControl& control, const Microsoft::UI::Xaml::Controls::CommandBarFlyout& sender, const bool withSelection);
         winrt::Windows::UI::Xaml::Controls::MenuFlyout _CreateRunAsAdminFlyout(int profileIndex);

         winrt::Microsoft::Terminal::Control::TermControl _senderOrActiveControl(const winrt::Windows::Foundation::IInspectable& sender);

(not tested)

We plumb the control that the context menu was opened for all the way through to where the event is actually handled (in _PopulateContextMenu)

[zadjii-msft]

In the interest of getting this in to cut a 1.19 build: 2450775

If you don't have time, we can just merge this as-is tomorrow morning, and add that commit in post. Whatever you feel is best ☺️

[mpela81]

In the interest of getting this in to cut a 1.19 build: 2450775

If you don't have time, we can just merge this as-is tomorrow morning, and add that commit in post. Whatever you feel is best ☺️

Probably I won't be able to work on it today, feel free to go ahead in the interest of the product 💪