Option to default to show icon in tab, hide tabicon or make icon in tab monochrome. by bundgaard · Pull Request #15948 · microsoft/terminal

bundgaard · 2023-09-10T15:38:52Z

Adding enum iconstyle for hiding the icon in the tab #8157

Summary of the Pull Request

Please confirm if I am on the right track.

References and Relevant Issues

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

PR Checklist

Closes Feature request: option to hide tab icons #8157
Tests added/passed
Documentation updated
- If checked, please file a pull request on our docs repo and link it here: #xxx
Schema updated (if necessary)

Adding enum iconstyle for hiding the icon in the tab

bundgaard · 2023-09-10T15:40:15Z

@microsoft-github-policy-service agree

bundgaard · 2023-09-10T15:56:30Z

I will be looking into making monochrome work, but I need to understand if I need to implement grayscale features or we have a way of getting monochrome icons from iconPath.

KalleOlaviNiemitalo · 2023-09-10T16:12:27Z

src/cascadia/TerminalApp/TerminalTab.h


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


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

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.

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.

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.)

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.

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.

zadjii-msft · 2023-09-11T10:42:55Z

Thanks for the contribution! We're all gonna be a little extra busy this week, so it might be a bit before we can give this a full review. Thanks for your patience!

I will be looking into making monochrome work, but I need to understand if I need to implement grayscale features or we have a way of getting monochrome icons from iconPath.

FYI: this is probably easily doable with iconSource.ShowAsMonochrome(true)

bundgaard · 2023-09-11T16:12:26Z

@zadjii-msft, I understand. I tried the iconSource.ShowAsMonochrome(true) but the result is not that beautiful. I can upload the code for you, and then we should have solved both.

Also added support for monochrome configuration.

zadjii-msft · 2023-09-19T12:07:22Z

I tried the iconSource.ShowAsMonochrome(true) but the result is not that beautiful

No, no it is not 😅 There's maybe a few icons where that's actually useful, but hey, I'm here to give users options. If they want the silly XAML-powered "convert all the pixels to the text foreground color" behavior, who am I to say no 😝?

lhecker · 2023-09-19T13:39:36Z

Personally speaking, I'm not sure whether we should add this if no user has asked for it yet though... Seems quite niche to me if I'm honest. 😶

lhecker

LGTM. But you need to format your PR. You can do this in PowerShell by navigating to the root of the repository and running:

Import-Module .\tools\OpenConsole.psm1
Invoke-CodeFormat

src/cascadia/TerminalApp/TerminalTab.cpp

zadjii-msft · 2023-09-19T16:55:21Z

Before I merge it, can you change the title to explain the change in brief?

Rather than "Fix bug 1234", it is easier to read if it is like (for example) "Evaluate the numbers before displaying them". It makes it so you can get a good idea of the code change without having to look at another bug ID

zadjii-msft

yep, this is exactly how I'd do it. Thanks!

zadjii-msft · 2023-09-19T16:57:39Z

src/cascadia/TerminalSettingsModel/IconPathConverter.cpp

+#include <d2d1.h>
+#include <d2d1effects_2.h>
+


nit: do we need these headers?

You are right, I missed removing them, it was from my idea to actually implement grayscale, but I will remove and sadly you guys have to re-approve.

One day into 1.19, and there's a LOT of hits here (**76.25%** of our ~300 crashes). A crash if the Theme doesn't have a `tab` member. Regressed in #15948 Closes MSFT:46714723

One day into 1.19, and there's a LOT of hits here (**76.25%** of our ~300 crashes). A crash if the Theme doesn't have a `tab` member. Regressed in #15948 Closes MSFT:46714723 (cherry picked from commit cf19385) Service-Card-Id: 90670731 Service-Version: 1.19

Enum IconStyle

0f3c340

Adding enum iconstyle for hiding the icon in the tab

bundgaard changed the title ~~Enum IconStyle~~ #8157 Enum IconStyle Sep 10, 2023

KalleOlaviNiemitalo reviewed Sep 10, 2023

View reviewed changes

Adding support for dynamic changing configuration

193fb60

Also added support for monochrome configuration.

lhecker approved these changes Sep 19, 2023

View reviewed changes

src/cascadia/TerminalApp/TerminalTab.cpp Outdated Show resolved Hide resolved

Code formatting according to PR feedback

83bafbd

lhecker approved these changes Sep 19, 2023

View reviewed changes

zadjii-msft approved these changes Sep 19, 2023

View reviewed changes

bundgaard changed the title ~~#8157 Enum IconStyle~~ Option to default to show icon in tab, hide tabicon or make icon in tab monochrome. Sep 19, 2023

removed leftover headers not wanted for this pr

03efa7a

zadjii-msft approved these changes Sep 19, 2023

View reviewed changes

zadjii-msft enabled auto-merge (squash) September 19, 2023 19:57

zadjii-msft merged commit 394b942 into microsoft:main Sep 19, 2023

zadjii-msft mentioned this pull request Sep 27, 2023

Fix a crash for users without a tab theme #16046

Merged

	// 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;
	}

		#include <d2d1.h>
		#include <d2d1effects_2.h>

Conversation

bundgaard commented Sep 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

References and Relevant Issues

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

PR Checklist

Uh oh!

bundgaard commented Sep 10, 2023

Uh oh!

bundgaard commented Sep 10, 2023

Uh oh!

KalleOlaviNiemitalo Sep 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bundgaard Sep 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KalleOlaviNiemitalo Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

lhecker Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

bundgaard Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

lhecker Sep 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bundgaard Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

zadjii-msft commented Sep 11, 2023

Uh oh!

bundgaard commented Sep 11, 2023

Uh oh!

zadjii-msft commented Sep 19, 2023

Uh oh!

lhecker commented Sep 19, 2023

Uh oh!

lhecker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zadjii-msft commented Sep 19, 2023

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

bundgaard Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bundgaard commented Sep 10, 2023 •

edited

Loading

KalleOlaviNiemitalo Sep 10, 2023 •

edited

Loading

bundgaard Sep 10, 2023 •

edited

Loading

lhecker Sep 19, 2023 •

edited

Loading