-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
Upgrade fmt to 10.1.1 #16007
base: main
Are you sure you want to change the base?
Upgrade fmt to 10.1.1 #16007
Conversation
L"") | ||
}; | ||
const auto newTabCommandline = newTerminalArgs ? newTerminalArgs.ToCommandline() : winrt::hstring{}; | ||
const auto cmdline = fmt::format(FMT_COMPILE(L"-w -1 new-tab {}"), newTabCommandline); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed all fmt::format
calls to use FMT_COMPILE
.
I'm not entirely sure why, but by using FMT_COMPILE
a bunch of compile errors went away.
@@ -1078,14 +1075,14 @@ namespace winrt::TerminalApp::implementation | |||
{ | |||
if (const auto& realArgs = args.ActionArgs().try_as<SearchForTextArgs>()) | |||
{ | |||
queryUrl = realArgs.QueryUrl().c_str(); | |||
queryUrl = std::wstring_view{ realArgs.QueryUrl() }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR only includes a few (~10?) of these changes. I did them as a drive-by fix because this PR already addresses formatting issues anyways.
@@ -28,7 +28,7 @@ namespace winrt::Microsoft::Terminal::Settings | |||
hstring LocalizedNameForEnumName(const std::wstring_view sectionAndEnumType, const std::wstring_view enumValue, const std::wstring_view propertyType) | |||
{ | |||
// Uppercase the first letter to conform to our current Resource keys | |||
auto fmtKey = fmt::format(L"{}{}{}/{}", sectionAndEnumType, char(std::towupper(enumValue[0])), enumValue.substr(1), propertyType); | |||
auto fmtKey = fmt::format(FMT_COMPILE(L"{}{}/{}"), sectionAndEnumType, enumValue, propertyType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you got here, you're 3/4ths done with everything but ActionArgs.cpp
. I'd suggest skipping ActionArgs.cpp
first, because it's a bit different from the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems dangerous; how did you settle on making it?
"Chesterton's fence"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW: resource keys seem to be case sensitive. That's why we put the enum in title case with the weird "toupper" thing.
I don't know why somebody used the dispreferred form of casting, char(x)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
char(std::towupper(enumValue[0]))
doesn't work anymore, because it now prints an integer (presumably because the builtin char -> wchar support was removed).
I've considered just removing the char
cast, but I was curious whether it would work without this cast. I went through every view in the settings UI and none of them threw an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth trying on Windows 10 as well 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the enum names come from our IDL (through jsoncpp), so why would anything change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wut
This relies on specific behavior in the Windows resource manager / MRT core. That has presumably changed a lot between Windows builds 19041 and 22621. 😄
Should we hold off on this PR until after the 1.19 release? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we hold off on this PR until after the 1.19 release?
Yup! I'll block it.
I would also prefer to (as mentioned before) switch to vcpkg before introducing another copy/paste of some OSS code 😄 |
I'm moving this to draft for now to get it out of the queue.
We'd all love for vcpkg to like, be a good package manager, but that's probably not a viable reason to block this for now. |
Between fmt 7.1.3 and 10.1.1 a lot has happened apart from improvements.
wchar_t
support is now more limited and implicit conversions fromchar*
towchar_t*
don't work anymore.Furthermore, even the non-
FMT_COMPILE
API is now compile-time checkedand so it fails to work in our UI code which passes
hstring
formatstrings which aren't implicitly convertible to the expected type.
fmt::runtime
was introduced for this but it also fails to work forhstring
parameters. To solve this, a newRS_fmt
macro was addedto abstract the added
std::wstring_view
casting away.Finally, some additional changes to reduce
stringstream
usage havebeen made, whenever
format_to
, etc., is available.This mostly affects
ActionArgs.cpp
.Closes #16000
Validation Steps Performed