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(renderer): Deal with invalid utf8 #2958

Merged
merged 12 commits into from May 10, 2023

Conversation

patrick96
Copy link
Member

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • Other: Replace this with a description of the type of this PR

Description

Refactors a lot of the utf8 handling in the renderer + tests.
In addition utf8_to_ucs4 now drops any invalid part of a utf8 string.

The renderer (in context.hpp) also produces a warning with some diagnostic information if invalid utf8 is encountered.

Related Issues & Documents

Fixes #2091

Documentation (check all applicable)

  • This PR requires changes to the Wiki documentation (describe the changes)
  • This PR requires changes to the documentation inside the git repo (please add them to the PR).
  • Does not require documentation changes

@patrick96 patrick96 added this to the 3.7.0 milestone May 10, 2023
@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

Merging #2958 (55a73fd) into master (d74a4fa) will increase coverage by 0.28%.
The diff coverage is 41.70%.

❗ Current head 55a73fd differs from pull request most recent head 15c9965. Consider uploading reports for the commit 15c9965 to get more accurate results

@@            Coverage Diff             @@
##           master    #2958      +/-   ##
==========================================
+ Coverage   12.60%   12.88%   +0.28%     
==========================================
  Files         162      162              
  Lines       12050    12049       -1     
==========================================
+ Hits         1519     1553      +34     
+ Misses      10531    10496      -35     
Flag Coverage Δ
unittests 12.88% <41.70%> (+0.28%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
include/cairo/context.hpp 0.00% <0.00%> (ø)
include/cairo/font.hpp 0.00% <0.00%> (ø)
include/utils/string.hpp 80.00% <0.00%> (-20.00%) ⬇️
src/cairo/utils.cpp 0.00% <0.00%> (ø)
src/utils/string.cpp 86.45% <86.45%> (-8.47%) ⬇️

@patrick96 patrick96 merged commit f78ec80 into polybar:master May 10, 2023
7 checks passed
@patrick96 patrick96 deleted the issue/2091-invalid-utf8 branch May 10, 2023 15:06
@yancelawang
Copy link

yancelawang commented Aug 19, 2023

hello, i still have this crash if i show browser window title to polybar using custom script. does it related if i open new bug report or it is not related to polybar? for now, i solved it by removing all non utf-8 characters

@patrick96
Copy link
Member Author

This fix is not part of a release yet. So unless you built from the latest sources, you are probably still experiencing the same bug.

@yancelawang
Copy link

Oh I see. i thought this fix was included in the last version. Thanks for your fast response.

@yancelawang
Copy link

This fix is not part of a release yet. So unless you built from the latest sources, you are probably still experiencing the same bug.

Hell again. I also face issue when try to show korean text to polybar, like [엘리멘탈] (다 같이) 사랑해, 러츠! 영상

@jasperla
Copy link

Commit 32c78aa seems to break with clang 13.0.0 on OpenBSD:

/build/polybar-3.7.0/src/utils/string.cpp:325:9: error: type 'std::array<char, 5>' does not provide a subscript operator
    utf8[0] = ucs;
    ~~~~^~

@patrick96
Copy link
Member Author

Not at my computer, so can't investigate myself, but that error seems strange because std::array clearly declares the [] operator: https://en.cppreference.com/w/cpp/container/array/operator_at

Maybe were missing an #include <array> somewhere

@patrick96
Copy link
Member Author

Yes we're indeed missing an include. This wasn't caught because it only seems to happen with the libc++ standard library implementation.

Should be easy to fix

@patrick96
Copy link
Member Author

@jasperla Please try out #3038 and see if you run into any other issues. I couldn't reproduce the issue locally (only on compiler explorer), so can't really know if everything else works properly on clang 13 with libc++.

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.

Font issue crashes polybar when string displayed is not valid UTF-8
3 participants