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

Allow for exe/dll paths for the Icon setting #14107

Merged
13 commits merged into from
Oct 26, 2022
Merged

Conversation

PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Sep 30, 2022

Summary of the Pull Request

Allow exe/dll paths for the Icon setting

The exe/dll icon needs to work in all the following areas:

  • The tab
  • The navigation view item in the SUI
  • The new tab flyout
  • The command palette

PR Checklist

  • Closes Support exe/dll paths in the 'icon' setting #1504
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
  • Schema updated.
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

For the command palette, we had to switch to using ContentPresenter because IconSourceElement cannot take in every type of icon we need to provide

Validation Steps Performed

Setting "%SystemRoot%\System32\shell32.dll,214" as the icon for a profile works in all the cases listed above.

@ghost ghost added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Sep 30, 2022
@PankajBhojwani
Copy link
Contributor Author

PankajBhojwani commented Sep 30, 2022

Taking over this feature since @zadjii-msft is going to be OOF for a while. Currently have it working for everything except the command palette, which is proving to be quite annoying because we cannot just declare an IconElement in xaml, we have to specify a child class of IconElement such as IconSourceElement / FontIcon or the like, which each take in a specific source type (whereas we need to be able to use both IconSource and SoftwareBitmapSource)

@PankajBhojwani PankajBhojwani marked this pull request as ready for review October 10, 2022 23:47
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

I think basically all of the comments are nits. I definitely want somebody to take a closer look at the async stuff, but other than that, nothing concerns me here :)

src/cascadia/TerminalSettingsModel/IconPathConverter.h Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/IconPathConverter.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/IconPathConverter.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/IconPathConverter.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/IconPathConverter.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/IconPathConverter.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/IconPathConverter.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/PaletteItem.idl Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/PaletteItem.cpp Show resolved Hide resolved
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

from bug bash:

image

font icons are now tiny, here and in the SUI.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 25, 2022
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 26, 2022
@PankajBhojwani
Copy link
Contributor Author

font icons are now tiny, here and in the SUI.

Thanks for the catch, fixed now!

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

god bless you for figuring out how to align the different icon types.

@carlos-zamora

This comment was marked as spam.

@carlos-zamora
Copy link
Member

@msftbot merge this in 10 minutes

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 26, 2022
@ghost
Copy link

ghost commented Oct 26, 2022

Hello @carlos-zamora!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Wed, 26 Oct 2022 18:11:08 GMT, which is in 10 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@ghost ghost merged commit aa62509 into main Oct 26, 2022
@ghost ghost deleted the dev/migrie/1504-prototype-2 branch October 26, 2022 18:12
@silkfire
Copy link

Which version will this be released in?

@zadjii-msft
Copy link
Member

The next release will be 1.17 Preview. No specific ETA on that yet - it'll be done when it's done 😉

@ghost
Copy link

ghost commented Jan 24, 2023

🎉Windows Terminal Preview v1.17.1023 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings Issues related to settings and customizability, for console or terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support exe/dll paths in the 'icon' setting
5 participants