-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Improve Profile.Icon UI in settings #17965
base: dev/cazamor/SUI/nullable-color-picker
Are you sure you want to change the base?
Changes from all commits
6bdace7
9b03fd9
941a792
bb32896
39059c4
f407ab1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
#include <LibraryResources.h> | ||
#include "../WinRTUtils/inc/Utils.h" | ||
#include "../../renderer/base/FontCache.h" | ||
#include "SegoeFluentIconList.h" | ||
|
||
using namespace winrt::Windows::UI::Text; | ||
using namespace winrt::Windows::UI::Xaml; | ||
|
@@ -26,6 +27,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation | |
|
||
Windows::Foundation::Collections::IObservableVector<Editor::Font> ProfileViewModel::_MonospaceFontList{ nullptr }; | ||
Windows::Foundation::Collections::IObservableVector<Editor::Font> ProfileViewModel::_FontList{ nullptr }; | ||
Windows::Foundation::Collections::IVector<IInspectable> ProfileViewModel::_BuiltInIcons{ nullptr }; | ||
|
||
static constexpr std::wstring_view HideIconValue{ L"none" }; | ||
|
||
|
@@ -40,6 +42,14 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation | |
INITIALIZE_BINDABLE_ENUM_SETTING_REVERSE_ORDER(CloseOnExitMode, CloseOnExitMode, winrt::Microsoft::Terminal::Settings::Model::CloseOnExitMode, L"Profile_CloseOnExit", L"Content"); | ||
INITIALIZE_BINDABLE_ENUM_SETTING(ScrollState, ScrollbarState, winrt::Microsoft::Terminal::Control::ScrollbarState, L"Profile_ScrollbarVisibility", L"Content"); | ||
|
||
// set up IconTypes | ||
_IconTypes = winrt::single_threaded_vector<IInspectable>(); | ||
_IconTypes.Append(make<EnumEntry>(RS_(L"Profile_IconTypeNone"), box_value(IconType::None))); | ||
_IconTypes.Append(make<EnumEntry>(RS_(L"Profile_IconTypeFontIcon"), box_value(IconType::FontIcon))); | ||
_IconTypes.Append(make<EnumEntry>(RS_(L"Profile_IconTypeEmoji"), box_value(IconType::Emoji))); | ||
_IconTypes.Append(make<EnumEntry>(RS_(L"Profile_IconTypeImage"), box_value(IconType::Image))); | ||
_DeduceCurrentIconType(); | ||
|
||
// Add a property changed handler to our own property changed event. | ||
// This propagates changes from the settings model to anybody listening to our | ||
// unique view model members. | ||
|
@@ -74,7 +84,20 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation | |
} | ||
else if (viewModelProperty == L"Icon") | ||
{ | ||
_NotifyChanges(L"HideIcon"); | ||
_DeduceCurrentIconType(); | ||
_NotifyChanges(L"LocalizedIcon", L"EvaluatedIcon", L"IconPreview"); | ||
} | ||
else if (viewModelProperty == L"CurrentIconType") | ||
{ | ||
_NotifyChanges(L"UsingNoIcon", L"UsingBuiltInIcon", L"UsingEmojiIcon", L"UsingImageIcon"); | ||
} | ||
else if (viewModelProperty == L"CurrentBuiltInIcon") | ||
{ | ||
Icon(unbox_value<hstring>(_CurrentBuiltInIcon.as<Editor::EnumEntry>().EnumValue())); | ||
} | ||
else if (viewModelProperty == L"CurrentEmojiIcon") | ||
{ | ||
Icon(CurrentEmojiIcon()); | ||
} | ||
}); | ||
|
||
|
@@ -90,6 +113,12 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation | |
UpdateFontList(); | ||
} | ||
|
||
if (!_BuiltInIcons) | ||
{ | ||
_UpdateBuiltInIcons(); | ||
} | ||
_DeduceCurrentBuiltInIcon(); | ||
|
||
if (profile.HasUnfocusedAppearance()) | ||
{ | ||
_unfocusedAppearanceViewModel = winrt::make<implementation::AppearanceViewModel>(profile.UnfocusedAppearance().try_as<AppearanceConfig>()); | ||
|
@@ -98,6 +127,58 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation | |
_defaultAppearanceViewModel.IsDefault(true); | ||
} | ||
|
||
void ProfileViewModel::_UpdateBuiltInIcons() | ||
{ | ||
_BuiltInIcons = single_threaded_vector<IInspectable>(); | ||
for (auto& [val, name] : s_SegoeFluentIcons) | ||
{ | ||
_BuiltInIcons.Append(make<EnumEntry>(hstring{ name }, box_value(val))); | ||
} | ||
Comment on lines
+133
to
+136
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd recommend the "first construct a BTW it's a funny thought but technically it's completely 100% possible to create a custom HSTRING that does not the need a heap allocation. The HSTRING struct is part of the ABI. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
|
||
void ProfileViewModel::_DeduceCurrentIconType() | ||
{ | ||
const auto& profileIcon = _profile.Icon(); | ||
if (profileIcon == HideIconValue) | ||
{ | ||
CurrentIconType(_IconTypes.GetAt(0)); | ||
} | ||
else if (L"\uE700" <= profileIcon && profileIcon <= L"\uF8B3") | ||
{ | ||
CurrentIconType(_IconTypes.GetAt(1)); | ||
_DeduceCurrentBuiltInIcon(); | ||
} | ||
else if (profileIcon.size() <= 2) | ||
{ | ||
// We already did a range check for MDL2 Assets in the previous one, | ||
// so if we're out of that range but still short, assume we're an emoji | ||
CurrentIconType(_IconTypes.GetAt(2)); | ||
} | ||
else | ||
{ | ||
CurrentIconType(_IconTypes.GetAt(3)); | ||
} | ||
} | ||
|
||
void ProfileViewModel::_DeduceCurrentBuiltInIcon() | ||
{ | ||
if (!_BuiltInIcons) | ||
{ | ||
_UpdateBuiltInIcons(); | ||
} | ||
const auto& profileIcon = Icon(); | ||
for (uint32_t i = 0; i < _BuiltInIcons.Size(); i++) | ||
{ | ||
const auto& builtIn = _BuiltInIcons.GetAt(i); | ||
if (profileIcon == unbox_value<hstring>(builtIn.as<Editor::EnumEntry>().EnumValue())) | ||
{ | ||
_CurrentBuiltInIcon = builtIn; | ||
return; | ||
} | ||
} | ||
_CurrentBuiltInIcon = _BuiltInIcons.GetAt(0); | ||
} | ||
|
||
Model::TerminalSettings ProfileViewModel::TermSettings() const | ||
{ | ||
return Model::TerminalSettings::CreateForPreview(_appSettings, _profile); | ||
|
@@ -328,24 +409,97 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation | |
} | ||
} | ||
|
||
bool ProfileViewModel::HideIcon() | ||
{ | ||
return Icon() == HideIconValue; | ||
} | ||
void ProfileViewModel::HideIcon(const bool hide) | ||
winrt::hstring ProfileViewModel::LocalizedIcon() const | ||
{ | ||
if (hide) | ||
if (_currentIconType && unbox_value<IconType>(_currentIconType.as<Editor::EnumEntry>().EnumValue()) == IconType::None) | ||
{ | ||
// Stash the current value of Icon. If the user | ||
// checks and un-checks the "Hide Icon" checkbox, we want | ||
// the path that we display in the text box to remain unchanged. | ||
_lastIcon = Icon(); | ||
Icon(HideIconValue); | ||
return RS_(L"Profile_IconTypeNone"); | ||
} | ||
else | ||
return Icon(); | ||
} | ||
|
||
Windows::UI::Xaml::Controls::IconElement ProfileViewModel::IconPreview() const | ||
{ | ||
// IconWUX sets the icon width/height to 32 by default | ||
auto icon = Microsoft::Terminal::UI::IconPathConverter::IconWUX(EvaluatedIcon()); | ||
icon.Width(16); | ||
icon.Height(16); | ||
return icon; | ||
} | ||
|
||
void ProfileViewModel::CurrentIconType(const Windows::Foundation::IInspectable& value) | ||
{ | ||
if (_currentIconType != value) | ||
{ | ||
Icon(_lastIcon); | ||
// Switching from... | ||
if (_currentIconType && unbox_value<IconType>(_currentIconType.as<Editor::EnumEntry>().EnumValue()) == IconType::Image) | ||
{ | ||
// Stash the current value of Icon. If the user | ||
// switches out of then back to IconType::Image, we want | ||
// the path that we display in the text box to remain unchanged. | ||
_lastIconPath = Icon(); | ||
} | ||
|
||
// Set the member here instead of after setting Icon() below! | ||
// We have an Icon property changed handler defined for when we discard changes. | ||
// Inadvertently, that means that we call this setter again. | ||
// Setting the member here means that we early exit at the beginning of the function | ||
// because _currentIconType == value. | ||
_currentIconType = value; | ||
|
||
// Switched to... | ||
switch (unbox_value<IconType>(value.as<Editor::EnumEntry>().EnumValue())) | ||
{ | ||
case IconType::None: | ||
{ | ||
Icon(HideIconValue); | ||
carlos-zamora marked this conversation as resolved.
Show resolved
Hide resolved
|
||
break; | ||
} | ||
case IconType::Image: | ||
{ | ||
if (!_lastIconPath.empty()) | ||
{ | ||
// Conversely, if we switch to Image, | ||
// retrieve that saved value and apply it | ||
Icon(_lastIconPath); | ||
} | ||
break; | ||
} | ||
case IconType::FontIcon: | ||
{ | ||
if (_CurrentBuiltInIcon) | ||
{ | ||
Icon(unbox_value<hstring>(_CurrentBuiltInIcon.as<Editor::EnumEntry>().EnumValue())); | ||
} | ||
break; | ||
} | ||
case IconType::Emoji: | ||
{ | ||
CurrentEmojiIcon({}); | ||
} | ||
} | ||
_NotifyChanges(L"CurrentIconType"); | ||
} | ||
}; | ||
|
||
bool ProfileViewModel::UsingNoIcon() const | ||
{ | ||
return _currentIconType == _IconTypes.GetAt(0); | ||
} | ||
|
||
bool ProfileViewModel::UsingBuiltInIcon() const | ||
{ | ||
return _currentIconType == _IconTypes.GetAt(1); | ||
} | ||
|
||
bool ProfileViewModel::UsingEmojiIcon() const | ||
{ | ||
return _currentIconType == _IconTypes.GetAt(2); | ||
} | ||
|
||
bool ProfileViewModel::UsingImageIcon() const | ||
{ | ||
return _currentIconType == _IconTypes.GetAt(3); | ||
} | ||
|
||
bool ProfileViewModel::IsBellStyleFlagSet(const uint32_t flag) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw right now we do this once per profile when we could do it once per settings UI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah,
_BuiltInIcons
is static so it's a shared instance across all ProfileVMs.And we only call it twice:
terminal/src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp
Lines 116 to 119 in bb32896
terminal/src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp
Lines 165 to 168 in bb32896
Both of which are wrapped in a null-check.
Confirmed w/ breakpoint in
_UpdateBuiltInIcons()
and poking around SUI (and closing/reopening SUI).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh jeez you're a step ahead of me!