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

Fix encoding issue in qt_sinks #2862

Merged
merged 6 commits into from
Sep 9, 2023
Merged

Fix encoding issue in qt_sinks #2862

merged 6 commits into from
Sep 9, 2023

Conversation

neothenil
Copy link
Contributor

QString::fromLatin1 doesn't handle non-latin characters properly. When applied to Chinese characters, the message appeared in QTextEdit looks like corrupted.
image

This PR fixes this issue.
image

@gabime
Copy link
Owner

gabime commented Aug 22, 2023

Duplicate of the rejected #2836. Colors ranges must an addressed somehow

@neothenil
Copy link
Contributor Author

neothenil commented Aug 23, 2023

Oh, I see. The problem is color ranges are byte indices, but they are passed to QString::left and QString::mid where the expected parameter is character index. How about converting color ranges from byte index to character index before passing to invoke_method_ like this commit does?

@gabime
Copy link
Owner

gabime commented Aug 28, 2023

Not sure. This should be debugged. And anyway msg contents should not be changed directly since they may be used by other sinks

@neothenil
Copy link
Contributor Author

For my use case, the modification works fine. The characters in QTextEdit are normal. The color ranges which were incorrect before the last commit also appears correct. And as long as the underlying string is UTF8-encoded, this modification should work well. In case the changed msg contents affect other sinks, I can make the converted color ranges local variables without changing msg directly.

include/spdlog/sinks/qt_sinks.h Outdated Show resolved Hide resolved
Copy link
Contributor

@tt4g tt4g left a comment

Choose a reason for hiding this comment

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

Sorry, a link to the source code was missing. See next comment.

@gabime
Copy link
Owner

gabime commented Sep 8, 2023

This results in 3 QString::fromUtf8() per each log call. It is expensive and should be avoided if only ascii/latin1 is used.

Please add a bool flag is_utf8 to the constructor and perform the fromUtf8() only if set to true.

@neothenil
Copy link
Contributor Author

The bool flag is_utf8 is added on your demand.

@gabime gabime merged commit 8014d6c into gabime:v1.x Sep 9, 2023
8 checks passed
@gabime
Copy link
Owner

gabime commented Sep 9, 2023

Thanks @neothenil and @tt4g . Merged.

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