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

When the profile icon is set to null, fall back to the icon of the commandline #15843

Merged
merged 15 commits into from
Feb 26, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
7 changes: 4 additions & 3 deletions src/cascadia/TerminalApp/TabManagement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -229,9 +229,10 @@ namespace winrt::TerminalApp::implementation
// Set this tab's icon to the icon from the user's profile
if (const auto profile{ newTabImpl->GetFocusedProfile() })
{
if (!profile.Icon().empty())
const auto& icon = profile.EvaluatedIcon();
if (!icon.empty())
{
newTabImpl->UpdateIcon(profile.Icon());
newTabImpl->UpdateIcon(icon);
}
}

Expand Down Expand Up @@ -296,7 +297,7 @@ namespace winrt::TerminalApp::implementation
{
if (const auto profile = tab.GetFocusedProfile())
{
tab.UpdateIcon(profile.Icon());
tab.UpdateIcon(profile.EvaluatedIcon());
}
}

Expand Down
5 changes: 3 additions & 2 deletions src/cascadia/TerminalApp/TerminalPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1012,9 +1012,10 @@ namespace winrt::TerminalApp::implementation

// If there's an icon set for this profile, set it as the icon for
// this flyout item
if (!profile.Icon().empty())
const auto& iconPath = profile.EvaluatedIcon();
if (!iconPath.empty())
{
const auto icon = _CreateNewTabFlyoutIcon(profile.Icon());
const auto icon = _CreateNewTabFlyoutIcon(iconPath);
profileMenuItem.Icon(icon);
}

Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalSettingsEditor/MainPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
MUX::Controls::NavigationViewItem profileNavItem;
profileNavItem.Content(box_value(profile.Name()));
profileNavItem.Tag(box_value<Editor::ProfileViewModel>(profile));
profileNavItem.Icon(IconPathConverter::IconWUX(profile.Icon()));
profileNavItem.Icon(IconPathConverter::IconWUX(profile.EvaluatedIcon()));

// Update the menu item when the icon/name changes
auto weakMenuItem{ make_weak(profileNavItem) };
Expand Down
5 changes: 5 additions & 0 deletions src/cascadia/TerminalSettingsEditor/ProfileViewModel.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
Padding(to_hstring(value));
}

winrt::hstring EvaluatedIcon() const
{
return _profile.EvaluatedIcon();
}

// starting directory
bool UseParentProcessDirectory();
void UseParentProcessDirectory(const bool useParent);
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalSettingsEditor/ProfileViewModel.idl
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ namespace Microsoft.Terminal.Settings.Editor
AppearanceViewModel UnfocusedAppearance { get; };
Boolean VtPassthroughAvailable { get; };

String EvaluatedIcon { get; };

void CreateUnfocusedAppearance();
void DeleteUnfocusedAppearance();

Expand Down
8 changes: 6 additions & 2 deletions src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -515,8 +515,12 @@ void CascadiaSettings::_validateMediaResources()
}
}

// Anything longer than 2 wchar_t's _isn't_ an emoji or symbol,
// so treat it as an invalid path.
// Anything longer than 2 wchar_t's _isn't_ an emoji or symbol, so treat
// it as an invalid path.
//
// Explicitly just use the Icon here, not the EvaluatedIcon. We don't
// want to blow up if we fell back to the commandline and the
// commandline _isn't an icon_.
if (const auto icon = profile.Icon(); icon.size() > 2)
{
const auto iconPath{ wil::ExpandEnvironmentStringsW<std::wstring>(icon.c_str()) };
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalSettingsModel/Command.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation

// - Escape the profile name for JSON appropriately
auto escapedProfileName = _escapeForJson(til::u16u8(p.Name()));
auto escapedProfileIcon = _escapeForJson(til::u16u8(p.Icon()));
auto escapedProfileIcon = _escapeForJson(til::u16u8(p.EvaluatedIcon()));
auto newJsonString = til::replace_needle_in_haystack(oldJsonString,
ProfileNameToken,
escapedProfileName);
Expand Down
25 changes: 25 additions & 0 deletions src/cascadia/TerminalSettingsModel/Profile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -336,3 +336,28 @@ Json::Value Profile::ToJson() const

return json;
}
winrt::hstring Profile::EvaluatedIcon() const
{
// If the profile has an icon, return it.
if (!Icon().empty())
Copy link
Member

Choose a reason for hiding this comment

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

can we not use our own member here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah, we can't because profile is layered so _Icon isn't the actual icon necessarily

{
return Icon();
}

// Otherwise, return the first word of the commandline - that should be the executable name.
std::wstring_view cmdline{ Commandline() };
if (cmdline.empty())
{
return {};
}
Copy link
Member

Choose a reason for hiding this comment

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

FWIW you can remove this check. An empty wstring still returns a valid string via .c_str() ("") and so all the code below should still work.


auto firstSpace = cmdline.find_first_of(L" ");
if (firstSpace == std::wstring::npos)
{
return winrt::hstring{ cmdline };
}
else
{
return winrt::hstring{ cmdline.substr(0, firstSpace) };
}
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
}
2 changes: 2 additions & 0 deletions src/cascadia/TerminalSettingsModel/Profile.h
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
Model::IAppearanceConfig DefaultAppearance();
Model::FontConfig FontInfo();

winrt::hstring EvaluatedIcon() const;

void _FinalizeInheritance() override;

// Special fields
Expand Down
4 changes: 4 additions & 0 deletions src/cascadia/TerminalSettingsModel/Profile.idl
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ namespace Microsoft.Terminal.Settings.Model
Boolean Deleted { get; };
OriginTag Origin { get; };

// Helper for magically using a commandline for an icon for a profile
// without an explicit icon.
String EvaluatedIcon { get; };

INHERITABLE_PROFILE_SETTING(Guid, Guid);
INHERITABLE_PROFILE_SETTING(String, Name);
INHERITABLE_PROFILE_SETTING(String, Source);
Expand Down
Loading