Remove WinRT composition (inheritance) from PaletteItem #19132
Merged
+322
−367
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Right now, we construct **two objects** for every palette item: the derived type and the base type. It's unnnecessary. 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.
|
The tests are probably exploded rigth now. |
|
Yes, we should cache the name. |
|
(Specifically: it is called 2386 times for a small repertoire of only 110 or so commands...) |
DHowett
requested review from
carlos-zamora and
lhecker
and removed request for
carlos-zamora
lhecker
approved these changes
Jul 16, 2025
| { | ||
| return NestedItemTemplate(); | ||
| } | ||
| __fallthrough; |
There was a problem hiding this comment.
[[fallthrough]], I believe. But why not just do a break instead?
Comment on lines
16
to
19
| const auto icon = Microsoft::Terminal::UI::IconPathConverter::IconWUX(static_cast<T*>(this)->Icon()); | ||
| icon.Width(16); | ||
| icon.Height(16); | ||
| return icon; |
There was a problem hiding this comment.
Is this also called hundreds of times? It looks immutable at first glance.
carlos-zamora
approved these changes
Jul 16, 2025
Comment on lines
+106
to
+108
| if (auto tab = _tab.get()) | ||
| { | ||
| if (auto terminalTab = tab.try_as<winrt::TerminalApp::TerminalTab>()) |
There was a problem hiding this comment.
Suggested change
| if (auto tab = _tab.get()) | |
| { | |
| if (auto terminalTab = tab.try_as<winrt::TerminalApp::TerminalTab>()) | |
| if (const auto tab = _tab.get()) | |
| { | |
| if (const auto terminalTab = tab.try_as<winrt::TerminalApp::TerminalTab>()) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 outthat something calls
Nameroughly 17 times per command and havingthe generator running that often is a serious waste of CPU.
Validation Steps