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

Option to default to show icon in tab, hide tabicon or make icon in tab monochrome. #15948

Merged
merged 4 commits into from
Sep 19, 2023
Merged

Option to default to show icon in tab, hide tabicon or make icon in tab monochrome. #15948

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0f3c340
Enum IconStyle
bundgaard Sep 10, 2023
193fb60
Adding support for dynamic changing configuration
bundgaard Sep 11, 2023
83bafbd
Code formatting according to PR feedback
bundgaard Sep 19, 2023
03efa7a
removed leftover headers not wanted for this pr
bundgaard Sep 19, 2023
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
5 changes: 3 additions & 2 deletions src/cascadia/LocalTests_SettingsModel/ThemeTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ namespace SettingsModelLocalTests
TEST_METHOD(ParseNullWindowTheme);
TEST_METHOD(ParseThemeWithNullThemeColor);
TEST_METHOD(InvalidCurrentTheme);

static Core::Color rgb(uint8_t r, uint8_t g, uint8_t b) noexcept
{
return Core::Color{ r, g, b, 255 };
Expand All @@ -60,7 +60,8 @@ namespace SettingsModelLocalTests
"tabRow":
{
"background": "#FFFF8800",
"unfocusedBackground": "#FF8844"
"unfocusedBackground": "#FF8844",
"iconStyle": "default"
},
"window":
{
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalApp/TabManagement.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ namespace winrt::TerminalApp::implementation
{
if (!profile.Icon().empty())
{
newTabImpl->UpdateIcon(profile.Icon());
newTabImpl->UpdateIcon(profile.Icon(), _settings.GlobalSettings().CurrentTheme().Tab().IconStyle());
}
}

Expand Down Expand Up @@ -241,7 +241,7 @@ namespace winrt::TerminalApp::implementation
{
if (const auto profile = tab.GetFocusedProfile())
{
tab.UpdateIcon(profile.Icon());
tab.UpdateIcon(profile.Icon(), _settings.GlobalSettings().CurrentTheme().Tab().IconStyle());
}
}

Expand Down
9 changes: 7 additions & 2 deletions src/cascadia/TerminalApp/TerminalTab.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,16 +278,21 @@ namespace winrt::TerminalApp::implementation
// - iconPath: The new path string to use as the IconPath for our TabViewItem
// Return Value:
// - <none>
void TerminalTab::UpdateIcon(const winrt::hstring iconPath)
void TerminalTab::UpdateIcon(const winrt::hstring iconPath, const winrt::Microsoft::Terminal::Settings::Model::IconStyle& iconStyle)
bundgaard marked this conversation as resolved.
Show resolved Hide resolved
{
ASSERT_UI_THREAD();

if (iconStyle == IconStyle::Hidden)
{
// we want to return early here
HideIcon(true);
return;
}
// Don't reload our icon if it hasn't changed.
if (iconPath == _lastIconPath)
{
return;
}

_lastIconPath = iconPath;

// If the icon is currently hidden, just return here (but only after setting _lastIconPath to the new path
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/TerminalTab.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ namespace winrt::TerminalApp::implementation
std::shared_ptr<Pane> newPane);

void ToggleSplitOrientation();
void UpdateIcon(const winrt::hstring iconPath);
void UpdateIcon(const winrt::hstring iconPath, const winrt::Microsoft::Terminal::Settings::Model::IconStyle& iconStyle);
Copy link

@KalleOlaviNiemitalo KalleOlaviNiemitalo Sep 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

IconStyle is an enum type; is it useful to pass that by const &?

Copy link
Contributor Author

@bundgaard bundgaard Sep 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

By passing it const& you indicate that it is a read-only type and it should not copy the value.
You can resolve the comment if you have no further questions.

Copy link

Choose a reason for hiding this comment

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

What's wrong with copying the value though? It's 32-bit so nowadays smaller than a pointer. There are other methods that take bool parameters by value.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @KalleOlaviNiemitalo. const& is most useful when passing RAII types to avoid making costly copies. An integer is much much cheaper to pass by-value.

(It can also be useful under certain circumstances when passing >8 bytes on the Windows x64 ABI due to poor MSVC optimizations, but I'm sure those few cycles are fairly irrelevant in most circumstances.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Member

@lhecker lhecker Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think it's fair to use other instances of this as proof that it isn't a mistake. Otherwise what does this code imply?

terminal/src/types/ModifierKeyState.cpp

Lines 100 to 137 in 9b70a40

// Routine Description:
// - Expands legacy control keys bitsets into a stl set
// Arguments:
// - flags - legacy bitset to expand
// Return Value:
// - set of ModifierKeyState values that represent flags
std::unordered_set<ModifierKeyState> FromConsoleControlKeyFlags(const DWORD flags)
{
std::unordered_set<ModifierKeyState> keyStates;
for (const auto& mapping : ModifierKeyStateTranslationTable)
{
if (RuntimeIsFlagSet(flags, mapping.second))
{
keyStates.insert(mapping.first);
}
}
return keyStates;
}
// Routine Description:
// - Converts ModifierKeyState back to the bizarre console bitflag associated with it.
// Arguments:
// - modifierKey - modifier to convert
// Return Value:
// - console bitflag associated with modifierKey
DWORD ToConsoleControlKeyFlag(const ModifierKeyState modifierKey) noexcept
{
for (const auto& mapping : ModifierKeyStateTranslationTable)
{
if (mapping.first == modifierKey)
{
return mapping.second;
}
}
return 0;
}

It turns a regular bitset into a std::unordered_set<> just to then test whether a given bit is set. It creates an entire hashmap instance just to do the equivalent of a & b != 0. It would be pretty terrific if this implied that binary operators are bad!

Passing primitive types by reference is a minuscule issue, so it's not really something I'm bothered by and I'll approve this PR regardless. But I believe if it came to a debate about this, that I could win it with fairly solid arguments.

Especially since statements like "it should not copy the value" are factually wrong. Passing parameters by value doesn't necessarily create a copy. Primitive types aren't classes after all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will not argue that you would have more solid arguments, but entering a code base, I found it better to do as already written, as to the arguments it is wrong or not, existing examples were already written with const&, so I found it better to copy the style.

My arguments are to the understanding of my knowledge and thank you for providing me with more information. My question would then be more along the lines of whether should we fix the existing code, or should we make it more clear that we do not need it, as now we are changing style from what is already written. I enjoy code that follows the same style.

void HideIcon(const bool hide);

void ShowBellIndicator(const bool show);
Expand Down
7 changes: 4 additions & 3 deletions src/cascadia/TerminalSettingsModel/MTSMSettings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,8 @@ Author(s):
X(winrt::Microsoft::Terminal::Settings::Model::ThemeColor, Background, "background", nullptr) \
X(winrt::Microsoft::Terminal::Settings::Model::ThemeColor, UnfocusedBackground, "unfocusedBackground", nullptr)

#define MTSM_THEME_TAB_SETTINGS(X) \
X(winrt::Microsoft::Terminal::Settings::Model::ThemeColor, Background, "background", nullptr) \
X(winrt::Microsoft::Terminal::Settings::Model::ThemeColor, UnfocusedBackground, "unfocusedBackground", nullptr) \
#define MTSM_THEME_TAB_SETTINGS(X) \
X(winrt::Microsoft::Terminal::Settings::Model::ThemeColor, Background, "background", nullptr) \
X(winrt::Microsoft::Terminal::Settings::Model::ThemeColor, UnfocusedBackground, "unfocusedBackground", nullptr) \
X(winrt::Microsoft::Terminal::Settings::Model::IconStyle, IconStyle, "iconStyle", winrt::Microsoft::Terminal::Settings::Model::IconStyle::Default) \
X(winrt::Microsoft::Terminal::Settings::Model::TabCloseButtonVisibility, ShowCloseButton, "showCloseButton", winrt::Microsoft::Terminal::Settings::Model::TabCloseButtonVisibility::Always)
8 changes: 8 additions & 0 deletions src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,14 @@ JSON_ENUM_MAPPER(::winrt::Microsoft::Terminal::Settings::Model::TabCloseButtonVi
};
};

JSON_ENUM_MAPPER(::winrt::Microsoft::Terminal::Settings::Model::IconStyle){
JSON_MAPPINGS(3) = {
pair_type{ "default", ValueType::Default },
pair_type{ "hidden", ValueType::Hidden },
pair_type{ "monochrome", ValueType::Monochrome },
};
};

// Possible ScrollToMarkDirection values
JSON_ENUM_MAPPER(::winrt::Microsoft::Terminal::Control::ScrollToMarkDirection)
{
Expand Down
8 changes: 8 additions & 0 deletions src/cascadia/TerminalSettingsModel/Theme.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,13 @@
namespace Microsoft.Terminal.Settings.Model
{

enum IconStyle
{
Default,
Hidden,
Monochrome
};

enum ThemeColorType
{
Accent,
Expand Down Expand Up @@ -63,6 +70,7 @@ namespace Microsoft.Terminal.Settings.Model
ThemeColor Background { get; };
ThemeColor UnfocusedBackground { get; };
TabCloseButtonVisibility ShowCloseButton { get; };
IconStyle IconStyle { get; };
}

[default_interface] runtimeclass Theme : Windows.Foundation.IStringable {
Expand Down
Loading