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

Warn when font isn't found and another is chosen #8207

Merged
merged 7 commits into from
Nov 11, 2020
Merged

Warn when font isn't found and another is chosen #8207

merged 7 commits into from
Nov 11, 2020

Conversation

miniksa
Copy link
Member

@miniksa miniksa commented Nov 9, 2020

Display a warning message when the DirectX renderer resolves a font that isn't the one you selected to warn that it couldn't be found.

PR Checklist

Detailed Description of the Pull Request / Additional comments

image

Also I wrote the dialog event chain out of TermControl to be reusable in the future for other messages the control might want to tell a host about and various levels.

Validation Steps Performed

  • Manual validation, setting bad font name, fixing font name with settings.json.

@miniksa
Add warning when the system chose a different font than then one you …
2f37fbe 
…picked.
@miniksa
we can't actually keep track of the previous font that was tried beca…
bee814f 
…use it's erased by settings updating and I don't want to poke that bear nor store another string copy.
@miniksa miniksa self-assigned this Nov 9, 2020
@ghost ghost added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Nov 9, 2020
@miniksa miniksa requested review from DHowett, carlos-zamora and zadjii-msft and removed request for DHowett November 9, 2020 22:26
carlos-zamora
carlos-zamora reviewed Nov 9, 2020
View reviewed changes
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.

Most of these things are pretty small.

src/cascadia/TerminalApp/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp
Comment on lines +1941 to +1952
case TerminalControl::NoticeLevel::Debug:
title = RS_(L"NoticeDebug"); //\xebe8
break;
case TerminalControl::NoticeLevel::Info:
title = RS_(L"NoticeInfo"); // \xe946
break;
case TerminalControl::NoticeLevel::Warning:
title = RS_(L"NoticeWarning"); //\xe7ba
break;
case TerminalControl::NoticeLevel::Error:
title = RS_(L"NoticeError"); //\xe783
break;
Copy link
Member

Choose a reason for hiding this comment

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

What are all of these //\xebe8 for?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had this bright idea that I was going to use Segoe icons for a bug shaped icon for debug and an I in a circle info icon for informational. But it didn't work right because the text in the dialog won't font fallback. And in lieu of adding another text field with the font set just right to take only the Segoe icon.... I just left the codepoints here for if I cared enough in the future because it was mildly painful to look them up in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

Yea les keep em

src/cascadia/TerminalApp/TerminalPage.cpp
Comment on lines +1962 to +1963
// FindName needs to be called first to actually load the xaml object
auto controlNoticeDialog = FindName(L"ControlNoticeDialog").try_as<WUX::Controls::ContentDialog>();
Copy link
Member

Choose a reason for hiding this comment

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

Wait...Really? So you never actually use controlNoticeDialog? I guess might as well not set it to the variable (or call Title off of controlNoticeDialog below).

Copy link
Member Author

Choose a reason for hiding this comment

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

Well.... the trickery is, as far as I know, that these things are lazy-instantiated. So I believe this is necessary to make XAML load the object before I can do the things below. So yeah, I don't use it. But I had to ask for it to make sure XAML did the needful first on the relevant other variables.

src/cascadia/TerminalApp/TerminalPage.xaml Outdated Show resolved Hide resolved
@mdtauk

This comment has been minimized.

Sign in to view
@DHowett

This comment has been minimized.

Sign in to view
@carlos-zamora

This comment has been minimized.

Sign in to view
@DHowett

This comment has been minimized.

Sign in to view
@mdtauk

This comment has been minimized.

Sign in to view
@zadjii-msft zadjii-msft self-assigned this Nov 10, 2020
zadjii-msft
zadjii-msft reviewed Nov 10, 2020
View reviewed changes
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.

I don't think I have much that people haven't already mentioned

src/cascadia/TerminalControl/TermControl.idl
Comment on lines +50 to +53
Debug = 10,
Info = 20,
Warning = 30,
Error = 40,
Copy link
Member

Choose a reason for hiding this comment

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

Do these values have a particular meaning?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I just pulled 4 levels out of my butt and left space for future ones to be put between without renumbering, like BASIC used to do...

If you have another idea... lemme know.

@zadjii-msft zadjii-msft removed their assignment Nov 10, 2020
DHowett
DHowett approved these changes Nov 10, 2020
View reviewed changes
src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.h Show resolved Hide resolved
carlos-zamora
carlos-zamora approved these changes Nov 10, 2020
View reviewed changes
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.

Idk what the deal with those two "changes, but not changes" are, but meh.

.github/actions/spell-check/expect/5757ec679b03a4240130c3c53766c91bbc5cd6a7.txt Outdated Show resolved Hide resolved
.github/actions/spell-check/expect/expect.txt Outdated Show resolved Hide resolved
@DHowett DHowett merged commit 4998779 into main Nov 11, 2020
@DHowett DHowett deleted the dev/miniksa/warn_font branch November 11, 2020 02:24
@ghost
Copy link

ghost commented Jan 28, 2021

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

Handy links:

@ghost ghost mentioned this pull request Jan 28, 2021
Closed
@zadjii-msft zadjii-msft mentioned this pull request Mar 4, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zadjii-msft zadjii-msft zadjii-msft left review comments

@carlos-zamora carlos-zamora carlos-zamora approved these changes

@DHowett DHowett DHowett approved these changes

Assignees

@miniksa miniksa

Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Display a dialog when the user selects an invalid font
5 participants
@miniksa @mdtauk @DHowett @carlos-zamora @zadjii-msft