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.

Sign up for GitHub

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

Jump to bottom

Move Tab Switcher mode handling into CommandPalette #8656

Merged
4 commits merged into from
Jan 19, 2021
+105 −114
Merged

Move Tab Switcher mode handling into CommandPalette #8656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
61b47b2
Move Tab Switcher mode handling into CommandPalette
Don-Vito Dec 26, 2020
4476cdd
Merge branch 'main' into 8415-move-tabswitcher-mode
Don-Vito Jan 4, 2021
cb04967
Merge branch 'main' into 8415-move-tabswitcher-mode
Don-Vito Jan 11, 2021
988caa0
Use std::copy for better readability
Don-Vito Jan 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions src/cascadia/TerminalApp/AppActionHandlers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,13 +521,8 @@ namespace winrt::TerminalApp::implementation
void TerminalPage::_HandleOpenTabSearch(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
// Tab search is always in-order.
CommandPalette().SetTabs(_tabs, true);

auto opt = _GetFocusedTabIndex();
uint32_t startIdx = opt.value_or(0);

CommandPalette().EnableTabSwitcherMode(true, startIdx);
CommandPalette().SetTabs(_tabs, _mruTabs);
CommandPalette().EnableTabSearchMode();
CommandPalette().Visibility(Visibility::Visible);

args.Handled(true);
Expand Down
136 changes: 74 additions & 62 deletions src/cascadia/TerminalApp/CommandPalette.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ namespace winrt::TerminalApp::implementation
_currentNestedCommands = winrt::single_threaded_vector<winrt::TerminalApp::FilteredCommand>();
_allCommands = winrt::single_threaded_vector<winrt::TerminalApp::FilteredCommand>();
_tabActions = winrt::single_threaded_vector<winrt::TerminalApp::FilteredCommand>();
_mruTabActions = winrt::single_threaded_vector<winrt::TerminalApp::FilteredCommand>();
_commandLineHistory = winrt::single_threaded_vector<winrt::TerminalApp::FilteredCommand>();

_switchToMode(CommandPaletteMode::ActionMode);
Expand All @@ -51,6 +52,12 @@ namespace winrt::TerminalApp::implementation
RegisterPropertyChangedCallback(UIElement::VisibilityProperty(), [this](auto&&, auto&&) {
if (Visibility() == Visibility::Visible)
{
if (_filteredActionsView().Items().Size() == 0 && _filteredActions.Size() > 0)
{
// Force immediate binding update so we can select an item
Bindings->Update();
}

if (_currentMode == CommandPaletteMode::TabSwitchMode)
{
_searchBox().Visibility(Visibility::Collapsed);
Expand All @@ -66,7 +73,6 @@ namespace winrt::TerminalApp::implementation
{
_filteredActionsView().SelectedIndex(0);
_searchBox().Focus(FocusState::Programmatic);
_updateFilteredActions();
}

TraceLoggingWrite(
Expand Down Expand Up @@ -112,7 +118,7 @@ namespace winrt::TerminalApp::implementation
// - <none>
void CommandPalette::SelectNextItem(const bool moveDown)
{
const auto selected = _filteredActionsView().SelectedIndex();
auto selected = _filteredActionsView().SelectedIndex();
const int numItems = ::base::saturated_cast<int>(_filteredActionsView().Items().Size());

// Do not try to select an item if
Expand Down Expand Up @@ -561,7 +567,7 @@ namespace winrt::TerminalApp::implementation
case CommandPaletteMode::TabSearchMode:
return _tabActions;
case CommandPaletteMode::TabSwitchMode:
return _tabActions;
return _tabSwitcherMode == TabSwitcherMode::MostRecentlyUsed ? _mruTabActions : _tabActions;
case CommandPaletteMode::CommandlineMode:
return _commandLineHistory;
default:
Expand Down Expand Up @@ -854,31 +860,40 @@ namespace winrt::TerminalApp::implementation
_allCommands.Append(filteredCommand);
}

_updateFilteredActions();
if (Visibility() == Visibility::Visible && _currentMode == CommandPaletteMode::ActionMode)
{
_updateFilteredActions();
}
}

void CommandPalette::SetTabs(Collections::IVector<TabBase> const& tabs, const bool clearList)
// Method Description:
// - Replaces a list of filtered commands in the target collection with new
// commands based on the tabs in the source collection.
// Although the source observable we still don't register on it,
// so the palette user will need to reset the binding manually every time
// the source collection changes
// Arguments:
// - source: the tabs to use for creation filtered commands
// - target: the collection to store newly created filtered commands
// Return Value:
// - <none>
void CommandPalette::_bindTabs(
Windows::Foundation::Collections::IObservableVector<winrt::TerminalApp::TabBase> const& source,
Windows::Foundation::Collections::IVector<winrt::TerminalApp::FilteredCommand> const& target)
{
_tabActions.Clear();
for (const auto& tab : tabs)
target.Clear();
for (const auto& tab : source)
{
auto tabPaletteItem{ winrt::make<winrt::TerminalApp::implementation::TabPaletteItem>(tab) };
auto filteredCommand{ winrt::make<FilteredCommand>(tabPaletteItem) };
_tabActions.Append(filteredCommand);
target.Append(filteredCommand);
}
}

// The smooth remove/add animations that happen during
// UpdateFilteredActions don't work very well with changing the tab
// order, because of the sheer amount of remove/adds. So, let's just
// clear & rebuild the list when we change the set of tabs.
//
// Some callers might actually want smooth updating, like when the list
// of tabs changes.
Comment on lines -875 to -876
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that now, if a tab is removed which the switcher is open, that the entire list will re-animate into existence, rather than the smooth update it does now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now if the tab is removed our ephemeral palette gets closed immediately 😊

Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with this

Copy link
Member

Choose a reason for hiding this comment

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

I believe that's because of the earlier change to dispatch bindings from TerminalPage::PreviewKeyDown, right? That makes sense to me.

But... does this also hold if a user (trying to break terminal :P) issues a sleep 5; exit and then opens up the palette and waits for it to blow up?

Copy link
Contributor Author

@Don-Vito Don-Vito Jan 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@DHowett - what a nasty user 😊 I believe that the problem you describe exists in the original code as well.

From my understanding, If the user triggers delayed sleep and then starts switching, what will happen right now is that we will still have TabPaletteItem populated in the switcher, but switching to it will have no action (as weak ref is empty). Not perfect, not terrible (or however they said this in Chernobyl). To resolve this we need to have the fully fledged binding that I planned for the next PR (where we do follow the changes in _tabs, _mruTabs that are observable collections).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DHowett - ignore the previous comment. What happens now, if the tab is closed by sleep 5; exit we actually dismiss the switcher. This was introduced as a part of the ephemeral palette work (every change in tabs collection closes the palette) to ensure consistency. If the user tries to break terminal this way, the only impact that the palette gets closed.

if (clearList && _currentMode == CommandPaletteMode::TabSwitchMode)
{
_filteredActions.Clear();
}
_updateFilteredActions();
void CommandPalette::SetTabs(Collections::IObservableVector<TabBase> const& tabs, Collections::IObservableVector<TabBase> const& mruTabs)
{
_bindTabs(tabs, _tabActions);
_bindTabs(mruTabs, _mruTabActions);
}

void CommandPalette::EnableCommandPaletteMode(CommandPaletteLaunchMode const launchMode)
Expand All @@ -888,26 +903,11 @@ namespace winrt::TerminalApp::implementation
CommandPaletteMode::ActionMode;

_switchToMode(mode);
_updateFilteredActions();
}

void CommandPalette::_switchToMode(CommandPaletteMode mode)
{
// The smooth remove/add animations that happen during
// UpdateFilteredActions don't work very well when switching between
// modes because of the sheer amount of remove/adds. So, let's just
// clear + append when switching between modes.
if (mode != _currentMode)
{
_currentMode = mode;
_filteredActions.Clear();
auto commandsToFilter = _commandsToFilter();

for (auto action : commandsToFilter)
{
_filteredActions.Append(action);
}
}
_currentMode = mode;

ParsedCommandLineText(L"");
_searchBox().Text(L"");
Expand Down Expand Up @@ -940,6 +940,13 @@ namespace winrt::TerminalApp::implementation
PrefixCharacter(L">");
break;
}

// The smooth remove/add animations that happen during
// UpdateFilteredActions don't work very well when switching between
// modes because of the sheer amount of remove/adds. So, let's just
// clear + append when switching between modes.
_filteredActions.Clear();
_updateFilteredActions();
}

// Method Description:
Expand All @@ -956,27 +963,34 @@ namespace winrt::TerminalApp::implementation
winrt::hstring searchText{ _getTrimmedInput() };

auto commandsToFilter = _commandsToFilter();
for (const auto& action : commandsToFilter)
{
// Update filter for all commands
// This will modify the highlighting but will also lead to re-computation of weight (and consequently sorting).
// Pay attention that it already updates the highlighting in the UI
action.UpdateFilter(searchText);

// if there is active search we skip commands with 0 weight
if (searchText.empty() || action.Weight() > 0)
if (_currentMode == CommandPaletteMode::TabSwitchMode)
{
std::copy(begin(commandsToFilter), end(commandsToFilter), std::back_inserter(actions));
}
else if (_currentMode == CommandPaletteMode::TabSearchMode || _currentMode == CommandPaletteMode::ActionMode || _currentMode == CommandPaletteMode::CommandlineMode)
{
for (const auto& action : commandsToFilter)
{
actions.push_back(action);
// Update filter for all commands
// This will modify the highlighting but will also lead to re-computation of weight (and consequently sorting).
// Pay attention that it already updates the highlighting in the UI
action.UpdateFilter(searchText);

// if there is active search we skip commands with 0 weight
if (searchText.empty() || action.Weight() > 0)
{
actions.push_back(action);
}
}
}

// We want to present the commands sorted,
// unless we are in the TabSwitcherMode and TabSearchMode,
// in which we want to preserve the original order (to be aligned with the tab view)
if (_currentMode != CommandPaletteMode::TabSearchMode && _currentMode != CommandPaletteMode::TabSwitchMode && _currentMode != CommandPaletteMode::CommandlineMode)
// We want to present the commands sorted
if (_currentMode == CommandPaletteMode::ActionMode)
{
std::sort(actions.begin(), actions.end(), FilteredCommand::Compare);
}

return actions;
}

Expand Down Expand Up @@ -1051,20 +1065,18 @@ namespace winrt::TerminalApp::implementation
_currentNestedCommands.Clear();
}

void CommandPalette::EnableTabSwitcherMode(const bool searchMode, const uint32_t startIdx)
void CommandPalette::EnableTabSwitcherMode(const uint32_t startIdx, TabSwitcherMode tabSwitcherMode)
{
_switcherStartIdx = startIdx;

if (searchMode)
{
_switchToMode(CommandPaletteMode::TabSearchMode);
}
else
{
_switchToMode(CommandPaletteMode::TabSwitchMode);
}

_updateFilteredActions();
// The _switcherStartIdx field allows us to select the current tab
// We need to take it into account only in the in-order mode,
// as an MRU mode the current tab is on top by definition.
_switcherStartIdx = tabSwitcherMode == TabSwitcherMode::InOrder ? startIdx : 0;
_tabSwitcherMode = tabSwitcherMode;
_switchToMode(CommandPaletteMode::TabSwitchMode);
}

void CommandPalette::EnableTabSearchMode()
{
_switchToMode(CommandPaletteMode::TabSearchMode);
}
}
13 changes: 8 additions & 5 deletions src/cascadia/TerminalApp/CommandPalette.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,9 @@ namespace winrt::TerminalApp::implementation
Windows::Foundation::Collections::IObservableVector<winrt::TerminalApp::FilteredCommand> FilteredActions();

void SetCommands(Windows::Foundation::Collections::IVector<Microsoft::Terminal::Settings::Model::Command> const& actions);
void SetTabs(Windows::Foundation::Collections::IVector<winrt::TerminalApp::TabBase> const& tabs, const bool clearList);
void SetTabs(Windows::Foundation::Collections::IObservableVector<winrt::TerminalApp::TabBase> const& tabs, Windows::Foundation::Collections::IObservableVector<winrt::TerminalApp::TabBase> const& mruTabs);
void SetKeyMap(const Microsoft::Terminal::Settings::Model::KeyMapping& keymap);

void EnableCommandPaletteMode(Microsoft::Terminal::Settings::Model::CommandPaletteLaunchMode const launchMode);

bool OnDirectKeyEvent(const uint32_t vkey, const uint8_t scanCode, const bool down);

void SelectNextItem(const bool moveDown);
Expand All @@ -45,8 +43,9 @@ namespace winrt::TerminalApp::implementation
void ScrollToTop();
void ScrollToBottom();

// Tab Switcher
void EnableTabSwitcherMode(const bool searchMode, const uint32_t startIdx);
void EnableCommandPaletteMode(Microsoft::Terminal::Settings::Model::CommandPaletteLaunchMode const launchMode);
void EnableTabSwitcherMode(const uint32_t startIdx, Microsoft::Terminal::Settings::Model::TabSwitcherMode tabSwitcherMode);
void EnableTabSearchMode();

WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler);
OBSERVABLE_GETSET_PROPERTY(winrt::hstring, NoMatchesText, _PropertyChangedHandlers);
Expand Down Expand Up @@ -112,7 +111,11 @@ namespace winrt::TerminalApp::implementation

// Tab Switcher
Windows::Foundation::Collections::IVector<winrt::TerminalApp::FilteredCommand> _tabActions{ nullptr };
Windows::Foundation::Collections::IVector<winrt::TerminalApp::FilteredCommand> _mruTabActions{ nullptr };
Microsoft::Terminal::Settings::Model::TabSwitcherMode _tabSwitcherMode;
uint32_t _switcherStartIdx;

void _bindTabs(Windows::Foundation::Collections::IObservableVector<winrt::TerminalApp::TabBase> const& source, Windows::Foundation::Collections::IVector<winrt::TerminalApp::FilteredCommand> const& target);
void _anchorKeyUpHandler();

winrt::Windows::UI::Xaml::Controls::ListView::SizeChanged_revoker _sizeChangedRevoker;
Expand Down
9 changes: 6 additions & 3 deletions src/cascadia/TerminalApp/CommandPalette.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,16 @@ namespace TerminalApp
Windows.Foundation.Collections.IObservableVector<FilteredCommand> FilteredActions { get; };

void SetCommands(Windows.Foundation.Collections.IVector<Microsoft.Terminal.Settings.Model.Command> actions);
void SetTabs(Windows.Foundation.Collections.IVector<TabBase> tabs, Boolean clearList);

void SetTabs(Windows.Foundation.Collections.IObservableVector<TabBase> tabs, Windows.Foundation.Collections.IObservableVector<TabBase> mruTabs);

void SetKeyMap(Microsoft.Terminal.Settings.Model.KeyMapping keymap);
void EnableCommandPaletteMode(Microsoft.Terminal.Settings.Model.CommandPaletteLaunchMode launchMode);

void SelectNextItem(Boolean moveDown);

void EnableTabSwitcherMode(Boolean searchMode, UInt32 startIdx);
void EnableCommandPaletteMode(Microsoft.Terminal.Settings.Model.CommandPaletteLaunchMode launchMode);
void EnableTabSwitcherMode(UInt32 startIdx, Microsoft.Terminal.Settings.Model.TabSwitcherMode tabSwitcherMode);
void EnableTabSearchMode();

event Windows.Foundation.TypedEventHandler<CommandPalette, TabBase> SwitchToTabRequested;
event Windows.Foundation.TypedEventHandler<CommandPalette, Microsoft.Terminal.Settings.Model.Command> DispatchCommandRequested;
Expand Down
50 changes: 14 additions & 36 deletions src/cascadia/TerminalApp/TerminalPage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ namespace winrt::TerminalApp::implementation
{
TerminalPage::TerminalPage() :
_tabs{ winrt::single_threaded_observable_vector<TerminalApp::TabBase>() },
_mruTabs{ winrt::single_threaded_vector<TerminalApp::TabBase>() },
_mruTabs{ winrt::single_threaded_observable_vector<TerminalApp::TabBase>() },
_startupActions{ winrt::single_threaded_vector<ActionAndArgs>() },
_hostingHwnd{}
{
Expand Down Expand Up @@ -1322,49 +1322,27 @@ namespace winrt::TerminalApp::implementation
// - Sets focus to the tab to the right or left the currently selected tab.
void TerminalPage::_SelectNextTab(const bool bMoveRight)
{
const auto index{ _GetFocusedTabIndex().value_or(0) };
const auto tabSwitchMode = _settings.GlobalSettings().TabSwitcherMode();
const bool useInOrderTabIndex = tabSwitchMode != TabSwitcherMode::MostRecentlyUsed;

// First, determine what the index of the newly selected tab should be.
// This changes if we're doing an in-order traversal vs a MRU traversal.
auto newTabIndex = 0;
if (useInOrderTabIndex)
{
// Determine what the next in-order tab index is
if (auto index{ _GetFocusedTabIndex() })
{
uint32_t tabCount = _tabs.Size();
// Wraparound math. By adding tabCount and then calculating
// modulo tabCount, we clamp the values to the range [0,
// tabCount) while still supporting moving leftward from 0 to
// tabCount - 1.
newTabIndex = ((tabCount + *index + (bMoveRight ? 1 : -1)) % tabCount);
}
if (tabSwitchMode == TabSwitcherMode::Disabled)
{
uint32_t tabCount = _tabs.Size();
// Wraparound math. By adding tabCount and then calculating
// modulo tabCount, we clamp the values to the range [0,
// tabCount) while still supporting moving leftward from 0 to
// tabCount - 1.
const auto newTabIndex = ((tabCount + index + (bMoveRight ? 1 : -1)) % tabCount);
_SelectTab(newTabIndex);
}
else
{
// Determine what the next "most recently used" index is.
// In this case, our focused tab index (in the MRU ordering) is
// always 0. So, going next should go to index 1, and going prev
// should wrap to the end.
uint32_t tabCount = _mruTabs.Size();
newTabIndex = ((tabCount + (bMoveRight ? 1 : -1)) % tabCount);
}

const bool useTabSwitcher = tabSwitchMode != TabSwitcherMode::Disabled;

if (useTabSwitcher)
{
CommandPalette().SetTabs(useInOrderTabIndex ? _tabs : _mruTabs, true);
CommandPalette().SetTabs(_tabs, _mruTabs);

// Otherwise, set up the tab switcher in the selected mode, with
// the given ordering, and make it visible.
CommandPalette().EnableTabSwitcherMode(false, newTabIndex);
CommandPalette().EnableTabSwitcherMode(index, tabSwitchMode);
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
CommandPalette().Visibility(Visibility::Visible);
}
else if (auto index{ _GetFocusedTabIndex() })
{
_SelectTab(newTabIndex);
CommandPalette().SelectNextItem(bMoveRight);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/TerminalPage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ namespace winrt::TerminalApp::implementation
Microsoft::Terminal::Settings::Model::CascadiaSettings _settings{ nullptr };

Windows::Foundation::Collections::IObservableVector<TerminalApp::TabBase> _tabs;
Windows::Foundation::Collections::IVector<TerminalApp::TabBase> _mruTabs;
Windows::Foundation::Collections::IObservableVector<TerminalApp::TabBase> _mruTabs;
static winrt::com_ptr<TerminalTab> _GetTerminalTabImpl(const TerminalApp::TabBase& tab);

void _UpdateTabIndices();
Expand Down