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

Add wide character support to wincolor_sink #3013

Open
wants to merge 2 commits into
base: v1.x
Choose a base branch
from

Conversation

DominikGrabiec
Copy link

Add wide character output support to wincolor_sink that's used as the default sink on Windows.

This uses the SPDLOG_WCHAR_TO_UTF8_SUPPORT define to enable this functionality much like in the msvc_sink.

The problem it solves is incorrect output in the Windows console when UTF8 characters are encountered in the string.
For example when outputting std::chrono::microsecond values like: log("Thing took {}", std::chrono::microsecond(1)). It ends up printing Thing took 1µs rather than the expected Thing took 1µs.

The issue is because fmt and hence spdlog create strings of UTF8 characters, whereas the Windows console expects the output to be ASCII with a specified codepage. Therefore converting the text to wide characters and outputting with WriteConsoleW fixes the issue.

However an alternative solution is to call SetConsoleOutputCP(CP_UTF8) at the start of your program to set the codepage of the console to UTF8. This does set the codepage for the console that's running, so the change will persist after the program exits, therefore more code needs to be added to retrieve the existing codepage and restore it at the end of the program. As such I don't think that code is appropriate to have in a library. Other developers can still implement codepage switching in their applications if they so choose, and in that case they do not need to enable SPDLOG_WCHAR_TO_UTF8_SUPPORT.

I also have to note that this doesn't change the WriteFile based output for when the program is not actually writing to the console but to a file or memory instead. The main place that affects me is in the test explorer window in Visual Studio where the log output is not treated as UTF8. The previous pull request had this erroneous change.

You can squash the two commits into one.

Fixes printing of unicode characters to the windows console such as microsecond suffix for std::chrono types.
@gabime
Copy link
Owner

gabime commented Mar 10, 2024

closed. i dont want to add new features to this stable branch

@gabime gabime closed this Mar 10, 2024
@DominikGrabiec
Copy link
Author

I could argue that this is fixing a bug with UTF8 character output in the Windows console.

Where should this work be done then if not in this branch?
What is the estimated release of that branch to something like vcpkg?
Do you need help with anything else on that branch to get it done faster?

@petersj-ess
Copy link

How come this was closed?

I've also just come accross this bug when printing to windows consoles

Is there another branch with this fix in?

@gabime
Copy link
Owner

gabime commented Mar 26, 2024

Problem is that it converts to wstr (utf8_to_wstrbuf) and would affect existing code and performance which doesn't need wstring output to console.

Please provide a way to use it only if really needed.

@DominikGrabiec
Copy link
Author

DominikGrabiec commented Mar 27, 2024

The fix is behind #ifdef SPDLOG_WCHAR_TO_UTF8_SUPPORT in the same way that the code in msvc_sink is structured, so that's why I thought it was safe to implement this way.

If you want, I could create another different #define SPDLOG_UTF8_TO_WCHAR_SUPPORT and put both the msvc_sink and the wincolor_sink 'wchar to 'utf8' code behind it and both implementations are consistent.

@gabime
Copy link
Owner

gabime commented Mar 29, 2024

Yes, please surround with different macro, something like SPDLOG_UTF8_TO_WCHAR_CONSOLE

That being said, if this is a common issue, we should consider adding a console->enable_utf_to_wstr(bool) to the api instead.

@gabime gabime reopened this Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants