Add padding to ProgressBar messages to avoid overlapping #1936
Conversation
Add padding to fully fill the terminal_width, this is because MultiProgressBar overrides its own messages, it doesn't clear the lines. If the message is short some leftover characters could be still present after it.
| // Add padding to fully fill the terminal_width, this is because MultiProgressBar | ||
| // overrides its own messages, it doesn't clear the lines. | ||
| // If the message is short some leftover characters could be still present after it. | ||
| if (message.length() < terminal_width - 4) { |
There was a problem hiding this comment.
message.length() does not always equal the displayed width of the message, e.g. message=もで 諤奯ゞ. I tried with 92ab80c to add support for wide characters using our existing libdnf5::cli::utils::utf8::width helper but it's not quite correct:
- I introduced a
double free or corruptionfault in the libdnf_cli tests, which seems to be triggered by callingsetlocale(LC_ALL, "C.UTF-8")inProgressbarInteractiveTest::setUp. Adding that call on the current main branch (without this PR's changes) also triggers the double free. I'm not sure what the issue is. - See 92ab80c#r150174705.
Thought I would share my progress and see if you could make anything of this.
There was a problem hiding this comment.
Also loosely connected to #1865 - trimming the message to the terminal width in the middle of a UTF multibyte character.
There was a problem hiding this comment.
That is a good point, I like your improvements.
Interestingly I cannot reproduce the double free or corruption it is working for me fine.
I tried running the libdnf5-cli tests through valgrind and apart from an unrelated versionlock_config issue I didn't see any problems.
There was a problem hiding this comment.
Interesting, want to cherry-pick 92ab80c here then? Then we can see whether the Packit CI tests will reproduce the double-free.
There was a problem hiding this comment.
It worked here even with you commit.
There was a problem hiding this comment.
Great, I think this is good to merge then.
f4b5ac7
Add padding to fully fill the terminal_width, this is because
MultiProgressBaroverrides its own messages, it doesn't clear the lines. If the message is short some leftover characters could be still present after it.The combination of adding/removing scriptlet messages and just overwriting
MultiProgressBarlines is making this quite complicated.Another approach to fix the issues could be reverting 8fbbeb2 it would make our code simpler but I think this should still be more efficient though I have not tested it.
Follow up for: #1925