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

zadjii-msft · 2023-08-17T15:32:19Z

Basically, title. If you null out the icon, we'll automatically try to use the commandline as an icon (because we can now). We'll even be smart about it - cmd.exe /k echo wassup will still just use the ico of cmd.exe.

This doesn't work for ubuntu.exe (et. al), because that commandline is technically a reparse point, that doesn't actually have an icon associated with it.
Closes #705

"none" becomes our sentinel value for "no icon".

This will also use the same NormalizeCommandLine we use for commandline matching for finding the full path to the exe.

lhecker · 2023-08-21T15:20:01Z

The only way to hide the icon becomes "icon": " ". That's weird. but passable?

Why can't we make it so that there's a differentiation between null and "empty string"? Empty string to hide the icon seems fine-ish to me...

src/cascadia/TerminalSettingsModel/Profile.cpp

zadjii-msft · 2023-08-21T21:10:58Z

Discussion notes:

Add a checkbox for "none" to the SUI for this setting.
This doesn't fix wt.exe from command line with named console binary picks the wrong tab icon and wrong tab title #7552 but it sure does make it easier in the future

…he icon of the commandline

…ult-to-commandline

zadjii-msft · 2023-09-07T16:51:32Z

lhecker · 2023-09-19T09:28:46Z

src/cascadia/TerminalSettingsModel/Profile.cpp

+    if (cmdline.empty())
+    {
+        return {};
+    }


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.

lhecker · 2023-09-19T09:29:51Z

src/cascadia/TerminalSettingsModel/Profile.cpp

+    const size_t beforeNull{ ::wcslen(cmdline.c_str()) };
+    const std::wstring_view exe{ cmdline.c_str(), beforeNull };
+    return winrt::hstring{ exe };


Just passing cmdline.c_str() to the hstring constructor will be fine. It has a hstring(wchar_t const* c) constructor, which calls wcslen for you. With the above comment this turns into:

const auto cmdline{ NormalizeCommandLine(Commandline().c_str()) }; // NormalizeCommandLine will return a string with embedded // nulls after each arg. We just want the first one. return winrt::hstring{ cmdline.c_str() };

I disagree generally that we should move from a smart string type to an hstring by using c_str. String types know their length; winrt::hstring(wchar_t*) must use wcslen to re-derive the length. Why do extra work?

We only want to return the first segment of the double-null terminated string. (See the comment 😅)

…ult-to-commandline

…hub.com/microsoft/terminal into dev/migrie/b/705-default-to-commandline

…he icon of the commandline

DHowett · 2023-09-25T19:36:12Z

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

+    <comment>A supplementary setting to the "icon" setting.</comment>
+  </data>
+  <data name="Profile_HideIconCheckbox.[using:Windows.UI.Xaml.Controls]ToolTipService.ToolTip" xml:space="preserve">
+    <value>If enabled, this will have no icon.</value>


Suggested change

<value>If enabled, this will have no icon.</value>

<value>If enabled, this profile will have no icon.</value>

this what?

DHowett · 2023-09-25T19:37:00Z

src/cascadia/TerminalSettingsModel/Profile.cpp

+winrt::hstring Profile::EvaluatedIcon()
+{
+    // We cache the result here, so we don't search the path for the exe every time.
+    if (!_evaluatedIcon.has_value())


hey, setting Icon doesn't invalidate this

we never really set directly on a Profile tho do we? We instantiate a new one when we build a new settings object, but that'd come with an empty cached _evaluatedIcon

Nah, the settings UI writes directly to Icon. It then fires local "Icon Changed" notifications and any listeners will probably re-call EvaluatedIcon (ugh)

which may mean that setting an icon in the SUI doesn't update IN THE UI until you mash Save and it re-loads the entire tree

but like, is that okay?

DHowett · 2023-09-25T19:37:17Z

src/cascadia/TerminalSettingsModel/Profile.cpp

+winrt::hstring Profile::_evaluateIcon() const
+{
+    // If the profile has an icon, return it.
+    if (!Icon().empty())


can we not use our own member here?

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

…ult-to-commandline

The settings UI and settings model allow you to set the icon to "none" to hide the icon (you can actually see this effect in the settings UI when changing the value of the profile icon). However, during settings validation, "none" is considered a file path, which is then failed to be parsed, resulting in the icon being marked as invalid and immediately clearing the value. This PR fixes this issue by considering "none" to be an accepted value during validation. Related to #15843 Closes #17943 ## Validation Steps Performed When an icon is set to "none", ... ✅ no more warning ✅ the icon is hidden

The settings UI and settings model allow you to set the icon to "none" to hide the icon (you can actually see this effect in the settings UI when changing the value of the profile icon). However, during settings validation, "none" is considered a file path, which is then failed to be parsed, resulting in the icon being marked as invalid and immediately clearing the value. This PR fixes this issue by considering "none" to be an accepted value during validation. Related to #15843 Closes #17943 ## Validation Steps Performed When an icon is set to "none", ... ✅ no more warning ✅ the icon is hidden (cherry picked from commit 36f064c) Service-Card-Id: PVTI_lADOAF3p4s4AmhmszgTyV8A Service-Version: 1.21

The settings UI and settings model allow you to set the icon to "none" to hide the icon (you can actually see this effect in the settings UI when changing the value of the profile icon). However, during settings validation, "none" is considered a file path, which is then failed to be parsed, resulting in the icon being marked as invalid and immediately clearing the value. This PR fixes this issue by considering "none" to be an accepted value during validation. Related to #15843 Closes #17943 ## Validation Steps Performed When an icon is set to "none", ... ✅ no more warning ✅ the icon is hidden (cherry picked from commit 36f064c) Service-Card-Id: PVTI_lADOAF3p4s4AmhmQzgTyV8E Service-Version: 1.22

Jihad85-D

Will be corrected

zadjii-msft added 2 commits August 17, 2023 10:04

This is easy enough

fb588b8

be smart, use the first word as the application name

c41895e

zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label Aug 17, 2023

microsoft-github-policy-service bot added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. labels Aug 17, 2023

lhecker reviewed Aug 21, 2023

View reviewed changes

src/cascadia/TerminalSettingsModel/Profile.cpp Outdated Show resolved Hide resolved

zadjii-msft removed the Needs-Discussion Something that requires a team discussion before we can proceed label Aug 21, 2023

zadjii-msft self-assigned this Aug 21, 2023

zadjii-msft mentioned this pull request Aug 22, 2023

wt.exe from command line with named console binary picks the wrong tab icon and wrong tab title #7552

Open

zadjii-msft added the zBugBash-Consider label Aug 29, 2023

zadjii-msft added a commit that referenced this pull request Aug 29, 2023

PRE-MERGE #15843 When the profile icon is set to null, fall back to t…

8859727

…he icon of the commandline

zadjii-msft added 3 commits September 7, 2023 10:29

Merge remote-tracking branch 'origin/main' into dev/migrie/b/705-defa…

c6c933a

…ult-to-commandline

add a sui checkbox for hiding the icon

cf7ebb8

better to use constants

9d9a8e3

zadjii-msft mentioned this pull request Sep 7, 2023

til::coalesce<T> should be able to take a function that returns T #15940

Open

This works for PowerShell in 'Program Files' now too

be28766

zadjii-msft removed their assignment Sep 7, 2023

lhecker approved these changes Sep 19, 2023

View reviewed changes

zadjii-msft added 3 commits September 21, 2023 16:11

Merge remote-tracking branch 'origin/main' into dev/migrie/b/705-defa…

180ac97

…ult-to-commandline

smort

a4de082

Merge branch 'dev/migrie/b/705-default-to-commandline' of https://git…

b804bc2

…hub.com/microsoft/terminal into dev/migrie/b/705-default-to-commandline

DHowett added a commit that referenced this pull request Sep 22, 2023

PRE-MERGE #15843 When the profile icon is set to null, fall back to t…

56ad204

…he icon of the commandline

DHowett reviewed Sep 25, 2023

View reviewed changes

DHowett requested changes Sep 25, 2023

View reviewed changes

microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 25, 2023

Merge remote-tracking branch 'origin/main' into dev/migrie/b/705-defa…

c31ea4d

…ult-to-commandline

microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 25, 2023

zadjii-msft added 2 commits September 26, 2023 09:17

phrasing

01b62c5

Merge remote-tracking branch 'origin/main' into dev/migrie/b/705-defa…

10d9157

…ult-to-commandline

zadjii-msft assigned DHowett Oct 12, 2023

zadjii-msft added 2 commits October 13, 2023 07:00

allow setting the icon to clear the cache

77fb50a

Merge remote-tracking branch 'origin/main' into dev/migrie/b/705-defa…

07fd586

…ult-to-commandline

zadjii-msft requested a review from DHowett February 8, 2024 15:38

Merge remote-tracking branch 'origin/main' into dev/migrie/b/705-defa…

6c20f9f

…ult-to-commandline

DHowett approved these changes Feb 26, 2024

View reviewed changes

zadjii-msft merged commit 4ff38c2 into main Feb 26, 2024
20 checks passed

zadjii-msft deleted the dev/migrie/b/705-default-to-commandline branch February 26, 2024 21:32

niclasmattsson mentioned this pull request Feb 27, 2024

Missing icon in Windows Terminal profile JuliaLang/juliaup#160

Open

DHowett mentioned this pull request Feb 27, 2024

Make the Settings Model tests into proper CI tests #16773

Merged

carlos-zamora mentioned this pull request Oct 10, 2024

Fix hiding the icon when it's set to "none" #18030

Merged

Jihad85-D reviewed Nov 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zadjii-msft commented Aug 17, 2023 •

edited

Loading

lhecker commented Aug 21, 2023

zadjii-msft commented Aug 21, 2023

zadjii-msft commented Sep 7, 2023

lhecker Sep 19, 2023

lhecker Sep 19, 2023 •

edited

Loading

DHowett Sep 21, 2023

lhecker Sep 22, 2023

DHowett Sep 25, 2023

DHowett Sep 25, 2023

zadjii-msft Sep 25, 2023

DHowett Sep 25, 2023

DHowett Sep 25, 2023

zadjii-msft Oct 12, 2023

DHowett Sep 25, 2023

zadjii-msft Oct 12, 2023

Jihad85-D left a comment

	<value>If enabled, this will have no icon.</value>
	<value>If enabled, this profile will have no icon.</value>

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

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

Conversation

zadjii-msft commented Aug 17, 2023 • edited Loading

lhecker commented Aug 21, 2023

zadjii-msft commented Aug 21, 2023

zadjii-msft commented Sep 7, 2023

Choose a reason for hiding this comment

lhecker Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jihad85-D left a comment

Choose a reason for hiding this comment

zadjii-msft commented Aug 17, 2023 •

edited

Loading

lhecker Sep 19, 2023 •

edited

Loading