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

Display useful pixel shader compilation errors #17436

Merged
merged 5 commits into from
Jun 21, 2024
Merged

Display useful pixel shader compilation errors #17436

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a75edea
Descriptive custom pixel shader compilation errors
blitzRahul Jun 15, 2024
542467e
proper formatting for TermControl.cpp
blitzRahul Jun 15, 2024
a76e370
ensure unicode text compatibility
blitzRahul Jun 15, 2024
3f4d582
add conditional warning message construction
blitzRahul Jun 21, 2024
7dbaa3b
improve efficiency of message construction
blitzRahul Jun 21, 2024
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
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/Resources/en-US/Resources.resw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ Please either install the missing font or choose another one.</value>
<comment>{Locked="2023.5.31","MacType"}</comment>
</data>
<data name="RendererErrorOther" xml:space="preserve">
<value>Renderer encountered an unexpected error: {0:#010x} {1} "{2}"</value>
<value>Renderer encountered an unexpected error: {0:#010x} {1}</value>
<comment>{Locked="{0:#010x}","{1}"} {0:#010x} is a placeholder for a Windows error code (e.g. 0x88985002). {1} is the corresponding message. {2} is the filename.</comment>
</data>
<data name="TermControlReadOnly" xml:space="preserve">
Expand Down
12 changes: 11 additions & 1 deletion src/cascadia/TerminalControl/TermControl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1175,7 +1175,17 @@ namespace winrt::Microsoft::Terminal::Control::implementation
wchar_t buf[512];
const auto len = FormatMessageW(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, nullptr, hr, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), &buf[0], ARRAYSIZE(buf), nullptr);
const std::wstring_view msg{ &buf[0], len };
message = winrt::hstring{ fmt::format(std::wstring_view{ RS_(L"RendererErrorOther") }, hr, msg, parameter) };
std::wstring resourceString = RS_(L"RendererErrorOther").c_str();
//conditional message construction
if (!parameter.empty())
{
resourceString += L" \"{2}\"";
message = winrt::hstring{ fmt::format(std::wstring_view{ resourceString }, hr, msg, parameter) };
}
else
{
message = winrt::hstring{ fmt::format(std::wstring_view{ resourceString }, hr, msg) };
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!parameter.empty())
{
resourceString += L" \"{2}\"";
message = winrt::hstring{ fmt::format(std::wstring_view{ resourceString }, hr, msg, parameter) };
}
else
{
message = winrt::hstring{ fmt::format(std::wstring_view{ resourceString }, hr, msg) };
}
std::wstring partialMessage = fmt::format(std::wstring_view{ resourceString }, hr, msg);
if (!parameter.empty())
{
fmt::format_to(std::back_inserter(partialMessage), FMT_COMPILE(LR"( "{2}")"), parameter);
}
message = winrt::hstring{ partialMessage };

How does this sound? It simplifies the logic (there is only one format call that uses the resource string) and reduces the dependency on the number of format specifiers in the resource (e.g. we don't need to be as careful when we change the resource string)

Copy link
Contributor Author

@blitzRahul blitzRahul Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It made sense; learnt something new today! Thank you for the review!
Edit: also next time I'll stick to referencing code via text and not screenshots, I imagine it makes a little bit of a mess.

break;
}
}
Expand Down
Loading