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 <nrk_AT_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
bakkeby_AT_gmail.com
Received on Sat Mar 19 2022 - 11:06:25 CET