Hi NRK, I was tinkering with this for a few hours earlier this week and there are several issues with this area of code and not specifically related to this modification of the patch.
The handling of adding the ellipsis (...) when the text is too long is rather hacky and can cause several issues. The general idea is that: - we work out how many consecutive characters we can print for the same font - then we check if the length of the string exceeds the remaining drawable space - if it is too long then we check the length of the string for every character until we find that it no longer fits - then we replace the last three bytes with periods (.) These are the problematic areas I have found: 1. In the original code the text is allowed to bleed into the right padding as long as it fits (I presume this is intentional). If you add one more character then the text is too long and it will crop of four characters to add the ellipsis. This has to do with that the for loop reduces len once more than necessary. 2. The adding of the ellipsis naively assumes that the last characters in the string are single-byte UTF-8 characters. As such when it replaces the last bytes of the string then you can end up with split multi-byte UTF-8 characters which can have various effects. 3. Even if the ellipsis has been drawn and it is not possible to draw anything else the code will continue as long as there is more text to process. 4. If there is a font change right before the ellipsis is drawn then the ellipsis will not be drawn (or not completely). More so if I am reading it right then if len is 1 it may be writing '.' to bad areas of memory. 5. When attempting to shorten the text it also assumes single-byte characters, which means that if you have a string of euro symbols (€) then it will call drw_font_getexts for each of the four bytes in that character. Now as for the patch. It is meant as a performance improvement when strings are too long to fit and I can see how it might be an improvement when the string is just a little bit too long for the drawable area, but what if it is twice as long? Given that it is asking XftTextExtentsUtf8 to calculate the width of every character every time this is checked I assume that it would be less performant (though possibly not so easy to measure). In an attempt to address some of these issues I played around with moving the XftTextExtentsUtf8 call to the section before that calculates utf8strlen and maintaining the ew variable manually. In practice means that the call to XftTextExtentsUtf8 is made for every single UTF-8 character (but only one character at a time). In addition to this I recorded the width of "..." to calculate and keep track of the last full UTF-8 character where the ellipsis would still fit so that I know where to go back to should the ellipsis need to be drawn. I can show the implementation of that should there be any interest. All in all I think the drawing of the ellipsis should either simply be dropped or a more simple solution should be found to indicate that the string (typically window title) is cut. Thanks, -Stein On Sat, Mar 19, 2022 at 9:09 AM NRK <n...@disroot.org> wrote: > Hi all, > > Adding to this: I've been using this patch, both in dmenu and dwm, for > the past week or so and haven't run into any issues. If someone finds > any, feel free to send some reproduction steps. > > Also noticed some small white-space issue in drw.h, don't think it's > worth sending another mail for, so I've attached the patch for it in > this mail. > > - NRK > -- Stein Gunnar Bakkeby OpenBet Developer bakk...@gmail.com
