Skip to content

Commit e7939bb

Browse files
DHowettDHowett
authored
Remove WinRT composition (inheritance) from PaletteItem (#19132)
Right now, we construct **two objects** for every palette item: the derived type and the base type. It's unnnecessary. This pull request replaces WinRT composition with a good old-fashioned `enum ThingType`. This also removes many of our palette items from our IDL. The only ones that are necessary to expose via our WinRT API surface are the ones that are used in XAML documents. I originally removed the caching for `Command.Name`, but it turns out that something calls `Name` roughly 17 times **per command** and having the generator running that often is a serious waste of CPU. ## Validation Steps - [x] Tab Switcher still live-updates when it is in use - [x] Command searching still works - [x] Commandline mode still works - [x] Suggestions control still works
1 parent bdf4432 commit e7939bb

25 files changed

+322
-367
lines changed

‎src/cascadia/LocalTests_TerminalApp/FilteredCommandTests.cpp‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@
22
// Licensed under the MIT license.
33

44
#include "pch.h"
5-
#include "../TerminalApp/CommandLinePaletteItem.h"
65
#include "../TerminalApp/CommandPalette.h"
6+
#include "../TerminalApp/BasePaletteItem.h"
77
#include "CppWinrtTailored.h"
88

99
using namespace Microsoft::Console;
@@ -15,6 +15,19 @@ using namespace winrt::Microsoft::Terminal::Control;
1515

1616
namespace TerminalAppLocalTests
1717
{
18+
struct StringPaletteItem : winrt::implements<StringPaletteItem, winrt::TerminalApp::IPaletteItem, winrt::Windows::UI::Xaml::Data::INotifyPropertyChanged>, winrt::TerminalApp::implementation::BasePaletteItem<StringPaletteItem, winrt::TerminalApp::PaletteItemType::CommandLine>
19+
{
20+
StringPaletteItem(std::wstring_view value) :
21+
_value{ value } {}
22+
23+
winrt::hstring Name() { return _value; }
24+
winrt::hstring KeyChordText() { return {}; }
25+
winrt::hstring Icon() { return {}; }
26+
27+
private:
28+
winrt::hstring _value;
29+
};
30+
1831
class FilteredCommandTests
1932
{
2033
BEGIN_TEST_CLASS(FilteredCommandTests)
@@ -40,7 +53,7 @@ namespace TerminalAppLocalTests
4053
auto result = RunOnUIThread([]() {
4154
const WEX::TestExecution::DisableVerifyExceptions disableExceptionsScope;
4255

43-
const auto paletteItem{ winrt::make<winrt::TerminalApp::implementation::CommandLinePaletteItem>(L"AAAAAABBBBBBCCC") };
56+
const auto paletteItem{ winrt::make<StringPaletteItem>(L"AAAAAABBBBBBCCC") };
4457
const auto filteredCommand = winrt::make_self<winrt::TerminalApp::implementation::FilteredCommand>(paletteItem);
4558

4659
{
@@ -112,7 +125,7 @@ namespace TerminalAppLocalTests
112125
void FilteredCommandTests::VerifyWeight()
113126
{
114127
auto result = RunOnUIThread([]() {
115-
const auto paletteItem{ winrt::make<winrt::TerminalApp::implementation::CommandLinePaletteItem>(L"AAAAAABBBBBBCCC") };
128+
const auto paletteItem{ winrt::make<StringPaletteItem>(L"AAAAAABBBBBBCCC") };
116129
const auto filteredCommand = winrt::make_self<winrt::TerminalApp::implementation::FilteredCommand>(paletteItem);
117130

118131
const auto weigh = [&](const wchar_t* str) {
@@ -152,8 +165,8 @@ namespace TerminalAppLocalTests
152165
void FilteredCommandTests::VerifyCompare()
153166
{
154167
auto result = RunOnUIThread([]() {
155-
const auto paletteItem{ winrt::make<winrt::TerminalApp::implementation::CommandLinePaletteItem>(L"AAAAAABBBBBBCCC") };
156-
const auto paletteItem2{ winrt::make<winrt::TerminalApp::implementation::CommandLinePaletteItem>(L"BBBBBCCC") };
168+
const auto paletteItem{ winrt::make<StringPaletteItem>(L"AAAAAABBBBBBCCC") };
169+
const auto paletteItem2{ winrt::make<StringPaletteItem>(L"BBBBBCCC") };
157170
{
158171
Log::Comment(L"Testing comparison of commands with no filter");
159172
const auto filteredCommand = winrt::make_self<winrt::TerminalApp::implementation::FilteredCommand>(paletteItem);
@@ -192,8 +205,8 @@ namespace TerminalAppLocalTests
192205
void FilteredCommandTests::VerifyCompareIgnoreCase()
193206
{
194207
auto result = RunOnUIThread([]() {
195-
const auto paletteItem{ winrt::make<winrt::TerminalApp::implementation::CommandLinePaletteItem>(L"a") };
196-
const auto paletteItem2{ winrt::make<winrt::TerminalApp::implementation::CommandLinePaletteItem>(L"B") };
208+
const auto paletteItem{ winrt::make<StringPaletteItem>(L"a") };
209+
const auto paletteItem2{ winrt::make<StringPaletteItem>(L"B") };
197210
{
198211
const auto filteredCommand = winrt::make_self<winrt::TerminalApp::implementation::FilteredCommand>(paletteItem);
199212
const auto filteredCommand2 = winrt::make_self<winrt::TerminalApp::implementation::FilteredCommand>(paletteItem2);

‎src/cascadia/TerminalApp/ActionPaletteItem.cpp‎

Lines changed: 0 additions & 28 deletions
This file was deleted.

‎src/cascadia/TerminalApp/ActionPaletteItem.h‎

Lines changed: 0 additions & 26 deletions
This file was deleted.

‎src/cascadia/TerminalApp/ActionPaletteItem.idl‎

Lines changed: 0 additions & 14 deletions
This file was deleted.

‎src/cascadia/TerminalApp/BasePaletteItem.h‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
// Copyright (c) Microsoft Corporation.
2+
// Licensed under the MIT license.
3+
4+
#pragma once
5+
6+
namespace winrt::TerminalApp::implementation
7+
{
8+
template<typename T, winrt::TerminalApp::PaletteItemType Ty>
9+
struct BasePaletteItem
10+
{
11+
public:
12+
winrt::TerminalApp::PaletteItemType Type() { return Ty; }
13+
14+
Windows::UI::Xaml::Controls::IconElement ResolvedIcon()
15+
{
16+
const auto icon{ static_cast<T*>(this)->Icon() };
17+
if (!_resolvedIcon && !icon.empty())
18+
{
19+
const auto resolvedIcon{ Microsoft::Terminal::UI::IconPathConverter::IconWUX(icon) };
20+
resolvedIcon.Width(16);
21+
resolvedIcon.Height(16);
22+
_resolvedIcon = resolvedIcon;
23+
}
24+
return _resolvedIcon;
25+
}
26+
27+
til::property_changed_event PropertyChanged;
28+
29+
protected:
30+
void BaseRaisePropertyChanged(wil::zwstring_view property)
31+
{
32+
PropertyChanged.raise(*static_cast<T*>(this), winrt::Windows::UI::Xaml::Data::PropertyChangedEventArgs{ property });
33+
}
34+
35+
void InvalidateResolvedIcon()
36+
{
37+
_resolvedIcon = nullptr;
38+
BaseRaisePropertyChanged(L"ResolvedIcon");
39+
}
40+
41+
private:
42+
Windows::UI::Xaml::Controls::IconElement _resolvedIcon{ nullptr };
43+
};
44+
}

‎src/cascadia/TerminalApp/CommandLinePaletteItem.cpp‎

Lines changed: 0 additions & 26 deletions
This file was deleted.

‎src/cascadia/TerminalApp/CommandLinePaletteItem.h‎

Lines changed: 0 additions & 23 deletions
This file was deleted.

‎src/cascadia/TerminalApp/CommandLinePaletteItem.idl‎

Lines changed: 0 additions & 12 deletions
This file was deleted.

‎src/cascadia/TerminalApp/CommandPalette.cpp‎

Lines changed: 38 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,8 @@
22
// Licensed under the MIT license.
33

44
#include "pch.h"
5-
#include "ActionPaletteItem.h"
6-
#include "TabPaletteItem.h"
7-
#include "CommandLinePaletteItem.h"
85
#include "CommandPalette.h"
6+
#include "CommandPaletteItems.h"
97
#include <LibraryResources.h>
108

119
#include "CommandPalette.g.cpp"
@@ -233,9 +231,11 @@ namespace winrt::TerminalApp::implementation
233231
}
234232
else if (_currentMode == CommandPaletteMode::ActionMode && filteredCommand != nullptr)
235233
{
236-
if (const auto actionPaletteItem{ filteredCommand.Item().try_as<winrt::TerminalApp::ActionPaletteItem>() })
234+
const auto item{ filteredCommand.Item() };
235+
if (item.Type() == PaletteItemType::Action)
237236
{
238-
PreviewAction.raise(*this, actionPaletteItem.Command());
237+
const auto actionPaletteItem{ winrt::get_self<ActionPaletteItem>(item) };
238+
PreviewAction.raise(*this, actionPaletteItem->Command());
239239
}
240240
}
241241
else if (_currentMode == CommandPaletteMode::CommandlineMode)
@@ -555,10 +555,11 @@ namespace winrt::TerminalApp::implementation
555555
const auto enteredItem = listViewItem.Content();
556556
if (const auto filteredCommand{ enteredItem.try_as<winrt::TerminalApp::FilteredCommand>() })
557557
{
558-
if (const auto actionPaletteItem{ filteredCommand.Item().try_as<winrt::TerminalApp::ActionPaletteItem>() })
558+
const auto item{ filteredCommand.Item() };
559+
if (item.Type() == PaletteItemType::Action)
559560
{
560-
// immediately preview the hovered command
561-
PreviewAction.raise(*this, actionPaletteItem.Command());
561+
const auto actionPaletteItem{ winrt::get_self<ActionPaletteItem>(item) };
562+
PreviewAction.raise(*this, actionPaletteItem->Command());
562563
}
563564
}
564565
}
@@ -589,9 +590,11 @@ namespace winrt::TerminalApp::implementation
589590
{
590591
if (_currentMode == CommandPaletteMode::ActionMode && filteredCommand)
591592
{
592-
if (const auto actionPaletteItem{ filteredCommand.Item().try_as<winrt::TerminalApp::ActionPaletteItem>() })
593+
const auto item{ filteredCommand.Item() };
594+
if (item.Type() == PaletteItemType::Action)
593595
{
594-
PreviewAction.raise(*this, actionPaletteItem.Command());
596+
const auto actionPaletteItem{ winrt::get_self<ActionPaletteItem>(item) };
597+
PreviewAction.raise(*this, actionPaletteItem->Command());
595598
}
596599
}
597600
}
@@ -617,7 +620,7 @@ namespace winrt::TerminalApp::implementation
617620
const auto selectedCommand = selectedList.GetAt(0);
618621
if (const auto filteredCmd = selectedCommand.try_as<TerminalApp::FilteredCommand>())
619622
{
620-
if (const auto paletteItem = filteredCmd.Item().try_as<TerminalApp::PaletteItem>())
623+
if (const auto paletteItem = filteredCmd.Item())
621624
{
622625
automationPeer.RaiseNotificationEvent(
623626
Automation::Peers::AutomationNotificationKind::ItemAdded,
@@ -652,10 +655,13 @@ namespace winrt::TerminalApp::implementation
652655
if (_nestedActionStack.Size() > 0)
653656
{
654657
const auto newPreviousAction{ _nestedActionStack.GetAt(_nestedActionStack.Size() - 1) };
655-
const auto actionPaletteItem{ newPreviousAction.Item().try_as<winrt::TerminalApp::ActionPaletteItem>() };
656-
657-
ParentCommandName(actionPaletteItem.Command().Name());
658-
_updateCurrentNestedCommands(actionPaletteItem.Command());
658+
const auto item{ newPreviousAction.Item() };
659+
if (item.Type() == PaletteItemType::Action)
660+
{
661+
const auto actionPaletteItem{ winrt::get_self<ActionPaletteItem>(item) };
662+
ParentCommandName(actionPaletteItem->Command().Name());
663+
_updateCurrentNestedCommands(actionPaletteItem->Command());
664+
}
659665
}
660666
else
661667
{
@@ -757,16 +763,19 @@ namespace winrt::TerminalApp::implementation
757763
}
758764
else if (filteredCommand)
759765
{
760-
if (const auto actionPaletteItem{ filteredCommand.Item().try_as<winrt::TerminalApp::ActionPaletteItem>() })
766+
auto item{ filteredCommand.Item() };
767+
if (item.Type() == PaletteItemType::Action)
761768
{
762-
if (actionPaletteItem.Command().HasNestedCommands())
769+
const auto actionPaletteItem{ winrt::get_self<ActionPaletteItem>(item) };
770+
auto command{ actionPaletteItem->Command() };
771+
if (command.HasNestedCommands())
763772
{
764773
// If this Command had subcommands, then don't dispatch the
765774
// action. Instead, display a new list of commands for the user
766775
// to pick from.
767776
_nestedActionStack.Append(filteredCommand);
768-
ParentCommandName(actionPaletteItem.Command().Name());
769-
_updateCurrentNestedCommands(actionPaletteItem.Command());
777+
ParentCommandName(command.Name());
778+
_updateCurrentNestedCommands(command);
770779

771780
_updateUIForStackChange();
772781
}
@@ -785,9 +794,9 @@ namespace winrt::TerminalApp::implementation
785794
// But make an exception for the Toggle Command Palette action: we don't want the dispatch
786795
// make the command palette - that was just closed - visible again.
787796
// All other actions can just be dispatched.
788-
if (actionPaletteItem.Command().ActionAndArgs().Action() != ShortcutAction::ToggleCommandPalette)
797+
if (command.ActionAndArgs().Action() != ShortcutAction::ToggleCommandPalette)
789798
{
790-
DispatchCommandRequested.raise(*this, actionPaletteItem.Command());
799+
DispatchCommandRequested.raise(*this, command);
791800
}
792801

793802
TraceLoggingWrite(
@@ -837,9 +846,11 @@ namespace winrt::TerminalApp::implementation
837846
{
838847
if (filteredCommand)
839848
{
840-
if (const auto tabPaletteItem{ filteredCommand.Item().try_as<winrt::TerminalApp::TabPaletteItem>() })
849+
const auto item{ filteredCommand.Item() };
850+
if (item.Type() == PaletteItemType::Tab)
841851
{
842-
if (const auto tab{ tabPaletteItem.Tab() })
852+
const auto tabPaletteItem{ winrt::get_self<TabPaletteItem>(item) };
853+
if (const auto tab{ tabPaletteItem->Tab() })
843854
{
844855
SwitchToTabRequested.raise(*this, tab);
845856
}
@@ -867,9 +878,11 @@ namespace winrt::TerminalApp::implementation
867878
TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES),
868879
TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage));
869880

870-
if (const auto commandLinePaletteItem{ filteredCommand.value().Item().try_as<winrt::TerminalApp::CommandLinePaletteItem>() })
881+
const auto item{ filteredCommand->Item() };
882+
if (item.Type() == PaletteItemType::CommandLine)
871883
{
872-
CommandLineExecutionRequested.raise(*this, commandLinePaletteItem.CommandLine());
884+
const auto commandLinePaletteItem{ winrt::get_self<CommandLinePaletteItem>(item) };
885+
CommandLineExecutionRequested.raise(*this, commandLinePaletteItem->CommandLine());
873886
_close();
874887
}
875888
}

0 commit comments

Comments
 (0)