Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce read-only panes #8867

Merged
24 commits merged into from Feb 8, 2021
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
6416347
Introduce read-only panes
Don-Vito Jan 20, 2021
f80fc21
Merge branch 'main' into 6981-readonly-tabs
Don-Vito Jan 23, 2021
46071db
Introduce ReadOnl property for tab
Don-Vito Jan 23, 2021
2861a1b
Document ReadOnly computation in the Pane
Don-Vito Jan 23, 2021
13476f5
Fix close read-only dialog texts
Don-Vito Jan 23, 2021
c4b17c5
Fix race while waiting for the read-only dialog confirmation
Don-Vito Jan 23, 2021
4e2a905
Introduce snapshot to resolve possible races
Don-Vito Jan 24, 2021
3926930
Add toggle read-only pane command to documents
Don-Vito Jan 24, 2021
ed0f30e
Introduce warning when closing a read-only pane
Don-Vito Jan 24, 2021
a69aec6
Merge branch 'main' into 6981-readonly-tabs
Don-Vito Jan 26, 2021
073065e
Merge branch 'main' into 6981-readonly-tabs
Don-Vito Jan 26, 2021
c5ca651
Change the primary button of close readonly dialog to cancel
Don-Vito Jan 26, 2021
4fc0ec6
Make ContainsReadOnly to be const
Don-Vito Jan 26, 2021
c5b75bd
Change the dialog text + fix the button names
Don-Vito Jan 26, 2021
d72ab1e
Rename the action into toggleReadOnlyMode
Don-Vito Jan 26, 2021
83402fa
Fix command name in schema
Don-Vito Jan 26, 2021
1f432c7
Merge branch 'main' into 6981-readonly-tabs
Don-Vito Jan 29, 2021
28f1abf
Convert ReadOnlyChanged into TypedEventHandler
Don-Vito Jan 29, 2021
5fe20c3
Fix double prompt on read-only pane + prevent race closing wrong pane
Don-Vito Jan 29, 2021
cef523c
Make active control const when checking if pane readonly
Don-Vito Feb 1, 2021
f38a8dd
Merge branch 'main' into 6981-readonly-tabs
Don-Vito Feb 4, 2021
3e40287
Fix read-only pane dialog text
Don-Vito Feb 4, 2021
21636f5
Merge remote-tracking branch 'upstream/main' into 6981-readonly-tabs
Don-Vito Feb 8, 2021
e21e352
Remove redundant read-only check
Don-Vito Feb 8, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/cascadia/profiles.schema.json
Expand Up @@ -105,6 +105,7 @@
"toggleFocusMode",
"toggleFullscreen",
"togglePaneZoom",
"toggleReadOnlyMode",
"toggleShaderEffects",
"wt",
"unbound"
Expand Down
34 changes: 23 additions & 11 deletions src/cascadia/TerminalApp/AppActionHandlers.cpp
Expand Up @@ -154,6 +154,17 @@ namespace winrt::TerminalApp::implementation
args.Handled(true);
}

void TerminalPage::_HandleTogglePaneReadOnly(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
if (const auto activeTab{ _GetFocusedTabImpl() })
{
activeTab->TogglePaneReadOnly();
}

args.Handled(true);
}

void TerminalPage::_HandleScrollUpPage(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
Expand Down Expand Up @@ -465,18 +476,20 @@ namespace winrt::TerminalApp::implementation
return;
}

// Remove tabs after the current one
while (_tabs.Size() > index + 1)
// Since _RemoveTab is asynchronous, create a snapshot of the tabs we want to remove
std::vector<winrt::TerminalApp::TabBase> tabsToRemove;
if (index > 0)
{
_RemoveTabViewItemByIndex(_tabs.Size() - 1);
std::copy(begin(_tabs), begin(_tabs) + index, std::back_inserter(tabsToRemove));
}

// Remove all of them leading up to the selected tab
while (_tabs.Size() > 1)
if (index + 1 < _tabs.Size())
{
_RemoveTabViewItemByIndex(0);
std::copy(begin(_tabs) + index + 1, end(_tabs), std::back_inserter(tabsToRemove));
}

_RemoveTabs(tabsToRemove);

actionArgs.Handled(true);
}
}
Expand All @@ -502,11 +515,10 @@ namespace winrt::TerminalApp::implementation
return;
}

// Remove tabs after the current one
while (_tabs.Size() > index + 1)
{
_RemoveTabViewItemByIndex(_tabs.Size() - 1);
}
// Since _RemoveTab is asynchronous, create a snapshot of the tabs we want to remove
std::vector<winrt::TerminalApp::TabBase> tabsToRemove;
std::copy(begin(_tabs) + index + 1, end(_tabs), std::back_inserter(tabsToRemove));
_RemoveTabs(tabsToRemove);

// TODO:GH#7182 For whatever reason, if you run this action
// when the tab that's currently focused is _before_ the `index`
Expand Down
14 changes: 12 additions & 2 deletions src/cascadia/TerminalApp/Pane.cpp
Expand Up @@ -416,8 +416,11 @@ void Pane::_ControlLostFocusHandler(winrt::Windows::Foundation::IInspectable con
// - <none>
void Pane::Close()
{
// Fire our Closed event to tell our parent that we should be removed.
_ClosedHandlers(nullptr, nullptr);
if (!_isClosing.exchange(true))
{
// Fire our Closed event to tell our parent that we should be removed.
_ClosedHandlers(nullptr, nullptr);
}
}

// Method Description:
Expand Down Expand Up @@ -2085,6 +2088,13 @@ std::optional<SplitState> Pane::PreCalculateAutoSplit(const std::shared_ptr<Pane
FAIL_FAST();
}

// Method Description:
// - Returns true if the pane or one of its descendants is read-only
bool Pane::ContainsReadOnly() const
{
return _IsLeaf() ? _control.ReadOnly() : (_firstChild->ContainsReadOnly() || _secondChild->ContainsReadOnly());
}

DEFINE_EVENT(Pane, GotFocus, _GotFocusHandlers, winrt::delegate<std::shared_ptr<Pane>>);
DEFINE_EVENT(Pane, LostFocus, _LostFocusHandlers, winrt::delegate<std::shared_ptr<Pane>>);
DEFINE_EVENT(Pane, PaneRaiseBell, _PaneRaiseBellHandlers, winrt::Windows::Foundation::EventHandler<bool>);
4 changes: 4 additions & 0 deletions src/cascadia/TerminalApp/Pane.h
Expand Up @@ -81,6 +81,8 @@ class Pane : public std::enable_shared_from_this<Pane>
void Id(uint16_t id) noexcept;
void FocusPane(const uint16_t id);

bool ContainsReadOnly() const;

WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler<winrt::Windows::Foundation::IInspectable>);
DECLARE_EVENT(GotFocus, _GotFocusHandlers, winrt::delegate<std::shared_ptr<Pane>>);
DECLARE_EVENT(LostFocus, _LostFocusHandlers, winrt::delegate<std::shared_ptr<Pane>>);
Expand Down Expand Up @@ -118,6 +120,8 @@ class Pane : public std::enable_shared_from_this<Pane>

Borders _borders{ Borders::None };

std::atomic<bool> _isClosing{ false };

bool _zoomed{ false };

bool _IsLeaf() const noexcept;
Expand Down
12 changes: 12 additions & 0 deletions src/cascadia/TerminalApp/Resources/en-US/Resources.resw
Expand Up @@ -404,6 +404,18 @@
<data name="CloseAllDialog.Title" xml:space="preserve">
<value>Do you want to close all tabs?</value>
</data>
<data name="CloseReadOnlyDialog.CloseButtonText" xml:space="preserve">
<value>Close anyway</value>
</data>
<data name="CloseReadOnlyDialog.PrimaryButtonText" xml:space="preserve">
<value>Cancel</value>
</data>
<data name="CloseReadOnlyDialog.Title" xml:space="preserve">
<value>Warning</value>
</data>
<data name="CloseReadOnlyDialog.Content" xml:space="preserve">
<value>You are about to close a read-only terminal. Do you wish to continue?</value>
</data>
<data name="LargePasteDialog.CloseButtonText" xml:space="preserve">
<value>Cancel</value>
</data>
Expand Down
5 changes: 5 additions & 0 deletions src/cascadia/TerminalApp/ShortcutActionDispatch.cpp
Expand Up @@ -256,6 +256,11 @@ namespace winrt::TerminalApp::implementation
_BreakIntoDebuggerHandlers(*this, eventArgs);
break;
}
case ShortcutAction::TogglePaneReadOnly:
{
_TogglePaneReadOnlyHandlers(*this, eventArgs);
break;
}
default:
return false;
}
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/ShortcutActionDispatch.h
Expand Up @@ -65,6 +65,7 @@ namespace winrt::TerminalApp::implementation
TYPED_EVENT(TabSearch, TerminalApp::ShortcutActionDispatch, Microsoft::Terminal::Settings::Model::ActionEventArgs);
TYPED_EVENT(MoveTab, TerminalApp::ShortcutActionDispatch, Microsoft::Terminal::Settings::Model::ActionEventArgs);
TYPED_EVENT(BreakIntoDebugger, TerminalApp::ShortcutActionDispatch, Microsoft::Terminal::Settings::Model::ActionEventArgs);
TYPED_EVENT(TogglePaneReadOnly, TerminalApp::ShortcutActionDispatch, Microsoft::Terminal::Settings::Model::ActionEventArgs);
// clang-format on

private:
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/ShortcutActionDispatch.idl
Expand Up @@ -51,5 +51,6 @@ namespace TerminalApp
event Windows.Foundation.TypedEventHandler<ShortcutActionDispatch, Microsoft.Terminal.Settings.Model.ActionEventArgs> TabSearch;
event Windows.Foundation.TypedEventHandler<ShortcutActionDispatch, Microsoft.Terminal.Settings.Model.ActionEventArgs> MoveTab;
event Windows.Foundation.TypedEventHandler<ShortcutActionDispatch, Microsoft.Terminal.Settings.Model.ActionEventArgs> BreakIntoDebugger;
event Windows.Foundation.TypedEventHandler<ShortcutActionDispatch, Microsoft.Terminal.Settings.Model.ActionEventArgs> TogglePaneReadOnly;
}
}
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/TabBase.h
Expand Up @@ -35,6 +35,7 @@ namespace winrt::TerminalApp::implementation

OBSERVABLE_GETSET_PROPERTY(winrt::hstring, Title, _PropertyChangedHandlers);
OBSERVABLE_GETSET_PROPERTY(winrt::hstring, Icon, _PropertyChangedHandlers);
OBSERVABLE_GETSET_PROPERTY(bool, ReadOnly, _PropertyChangedHandlers, false);
GETSET_PROPERTY(winrt::Microsoft::UI::Xaml::Controls::TabViewItem, TabViewItem, nullptr);

OBSERVABLE_GETSET_PROPERTY(winrt::Windows::UI::Xaml::FrameworkElement, Content, _PropertyChangedHandlers, nullptr);
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/TabBase.idl
Expand Up @@ -8,6 +8,7 @@ namespace TerminalApp
{
String Title { get; };
String Icon { get; };
Boolean ReadOnly { get; };

Microsoft.UI.Xaml.Controls.TabViewItem TabViewItem { get; };
Windows.UI.Xaml.FrameworkElement Content { get; };
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/TabHeaderControl.h
Expand Up @@ -28,6 +28,7 @@ namespace winrt::TerminalApp::implementation
OBSERVABLE_GETSET_PROPERTY(bool, IsProgressRingIndeterminate, _PropertyChangedHandlers);
OBSERVABLE_GETSET_PROPERTY(bool, BellIndicator, _PropertyChangedHandlers);
OBSERVABLE_GETSET_PROPERTY(uint32_t, ProgressValue, _PropertyChangedHandlers);
OBSERVABLE_GETSET_PROPERTY(bool, IsReadOnlyActive, _PropertyChangedHandlers);

private:
bool _receivedKeyDown{ false };
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/TabHeaderControl.idl
Expand Up @@ -14,6 +14,7 @@ namespace TerminalApp
Boolean IsProgressRingIndeterminate { get; set; };
Boolean BellIndicator { get; set; };
UInt32 ProgressValue { get; set; };
Boolean IsReadOnlyActive { get; set; };

TabHeaderControl();
void BeginRename();
Expand Down
6 changes: 6 additions & 0 deletions src/cascadia/TerminalApp/TabHeaderControl.xaml
Expand Up @@ -55,6 +55,12 @@ the MIT License. See LICENSE in the project root for license information. -->
Glyph="&#xE8A3;"
FontSize="12"
Margin="0,0,8,0"/>
<FontIcon x:Name="HeaderLockIcon"
FontFamily="Segoe MDL2 Assets"
Visibility="{x:Bind IsReadOnlyActive, Mode=OneWay}"
Glyph="&#xE72E;"
FontSize="12"
Margin="0,0,8,0"/>
<TextBlock x:Name="HeaderTextBlock"
Visibility="Visible"
Text="{x:Bind Title, Mode=OneWay}"/>
Expand Down
100 changes: 82 additions & 18 deletions src/cascadia/TerminalApp/TerminalPage.cpp
Expand Up @@ -445,6 +445,17 @@ namespace winrt::TerminalApp::implementation
co_return ContentDialogResult::None;
}

// Method Description:
// - Displays a dialog for warnings found while closing the terminal tab marked as read-only
winrt::Windows::Foundation::IAsyncOperation<ContentDialogResult> TerminalPage::_ShowCloseReadOnlyDialog()
{
if (auto presenter{ _dialogPresenter.get() })
{
co_return co_await presenter.ShowDialog(FindName(L"CloseReadOnlyDialog").try_as<WUX::Controls::ContentDialog>());
}
co_return ContentDialogResult::None;
}

// Method Description:
// - Displays a dialog to warn the user about the fact that the text that
// they are trying to paste contains the "new line" character which can
Expand Down Expand Up @@ -1053,6 +1064,7 @@ namespace winrt::TerminalApp::implementation
_actionDispatch->TabSearch({ this, &TerminalPage::_HandleOpenTabSearch });
_actionDispatch->MoveTab({ this, &TerminalPage::_HandleMoveTab });
_actionDispatch->BreakIntoDebugger({ this, &TerminalPage::_HandleBreakIntoDebugger });
_actionDispatch->TogglePaneReadOnly({ this, &TerminalPage::_HandleTogglePaneReadOnly });
}

// Method Description:
Expand Down Expand Up @@ -1163,7 +1175,7 @@ namespace winrt::TerminalApp::implementation

// Method Description:
// - Look for the index of the input tabView in the tabs vector,
// and call _RemoveTabViewItemByIndex
// and call _RemoveTab
// Arguments:
// - tabViewItem: the TabViewItem in the TabView that is being removed.
void TerminalPage::_RemoveTabViewItem(const MUX::Controls::TabViewItem& tabViewItem)
Expand All @@ -1172,16 +1184,35 @@ namespace winrt::TerminalApp::implementation
if (_tabView.TabItems().IndexOf(tabViewItem, tabIndexFromControl))
{
// If IndexOf returns true, we've actually got an index
_RemoveTabViewItemByIndex(tabIndexFromControl);
auto tab{ _tabs.GetAt(tabIndexFromControl) };
_RemoveTab(tab);
}
}

// Method Description:
// - Removes the tab (both TerminalControl and XAML)
// Arguments:
// - tabIndex: the index of the tab to be removed
void TerminalPage::_RemoveTabViewItemByIndex(uint32_t tabIndex)
// - tab: the tab to remove
winrt::Windows::Foundation::IAsyncAction TerminalPage::_RemoveTab(winrt::TerminalApp::TabBase tab)
{
if (tab.ReadOnly())
{
ContentDialogResult warningResult = co_await _ShowCloseReadOnlyDialog();

// The primary action is canceling the removal
if (warningResult == ContentDialogResult::Primary)
Don-Vito marked this conversation as resolved.
Show resolved Hide resolved
{
co_return;
}
}

uint32_t tabIndex{};
if (!_tabs.IndexOf(tab, tabIndex))
{
// The tab is already removed
co_return;
}

// We use _removing flag to suppress _OnTabSelectionChanged events
// that might get triggered while removing
_removing = true;
Expand All @@ -1191,11 +1222,10 @@ namespace winrt::TerminalApp::implementation

// Removing the tab from the collection should destroy its control and disconnect its connection,
// but it doesn't always do so. The UI tree may still be holding the control and preventing its destruction.
auto tab{ _tabs.GetAt(tabIndex) };
tab.Shutdown();

uint32_t mruIndex;
if (_mruTabs.IndexOf(_tabs.GetAt(tabIndex), mruIndex))
uint32_t mruIndex{};
if (_mruTabs.IndexOf(tab, mruIndex))
{
_mruTabs.RemoveAt(mruIndex);
}
Expand Down Expand Up @@ -1264,6 +1294,8 @@ namespace winrt::TerminalApp::implementation
_rearrangeFrom = std::nullopt;
_rearrangeTo = std::nullopt;
}

co_return;
}

// Method Description:
Expand Down Expand Up @@ -1539,26 +1571,54 @@ namespace winrt::TerminalApp::implementation
{
if (auto index{ _GetFocusedTabIndex() })
{
_RemoveTabViewItemByIndex(*index);
auto tab{ _tabs.GetAt(*index) };
_RemoveTab(tab);
}
}

// Method Description:
// - Close the currently focused pane. If the pane is the last pane in the
// tab, the tab will also be closed. This will happen when we handle the
// tab's Closed event.
void TerminalPage::_CloseFocusedPane()
winrt::fire_and_forget TerminalPage::_CloseFocusedPane()
{
if (const auto terminalTab{ _GetFocusedTabImpl() })
{
_UnZoomIfNeeded();
terminalTab->ClosePane();

auto pane = terminalTab->GetActivePane();
Don-Vito marked this conversation as resolved.
Show resolved Hide resolved

if (const auto pane{ terminalTab->GetActivePane() })
{
if (const auto control{ pane->GetTerminalControl() })
{
if (control.ReadOnly())
{
ContentDialogResult warningResult = co_await _ShowCloseReadOnlyDialog();

// The primary action is canceling the action
if (warningResult == ContentDialogResult::Primary)
{
co_return;
}

// Clean read-only mode to prevent additional prompt if closing the pane triggers closing of a hosting tab
if (control.ReadOnly())
{
control.ToggleReadOnly();
}
}

pane->Close();
Comment on lines +1598 to +1613
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: what happens if we run sleep 5 ; exit, then during the sleep mark a pane as read-only, then try to close the pane, then wait before confirming. We'll get the dialog, then the pane will close on its own, then we'll try to close it again. Is that fine (probably)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea probably, because of the atomic added in Pane

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, this was fine. Nothing to worry about here.

}
}
}
else if (auto index{ _GetFocusedTabIndex() })
{
if (_tabs.GetAt(*index).try_as<TerminalApp::SettingsTab>())
const auto tab{ _tabs.GetAt(*index) };
if (tab.try_as<TerminalApp::SettingsTab>())
{
_RemoveTabViewItemByIndex(*index);
_RemoveTab(tab);
}
}
}
Expand All @@ -1580,17 +1640,21 @@ namespace winrt::TerminalApp::implementation
}
}

_CloseAllTabs();
// Since _RemoveTab is asynchronous, create a snapshot of the tabs we want to remove
std::vector<winrt::TerminalApp::TabBase> tabsToRemove;
std::copy(begin(_tabs), end(_tabs), std::back_inserter(tabsToRemove));
_RemoveTabs(tabsToRemove);
}

// Method Description:
// - Remove all the tabs opened and the terminal will terminate
// on its own when the last tab is closed.
void TerminalPage::_CloseAllTabs()
// - Closes provided tabs one by one
// Arguments:
// - tabs - tabs to remove
winrt::fire_and_forget TerminalPage::_RemoveTabs(const std::vector<winrt::TerminalApp::TabBase> tabs)
{
while (_tabs.Size() != 0)
for (auto& tab : tabs)
Don-Vito marked this conversation as resolved.
Show resolved Hide resolved
{
_RemoveTabViewItemByIndex(0);
co_await _RemoveTab(tab);
}
}

Expand Down