Hi NRK, I have gone through these patches again and they seem pretty solid.
Regarding the first patch I got the impression that the overflow should be triggered if (ew + tmpw > w) { The reasoning is that it appears to crop 4 characters instead of 3 when it adds the ellipsis. If we try a long echo on the form of $ echo "---------------------------------------------------------------------------------------------aaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbYcccX" | ./dmenu -l 10 and you add or remove characters such that it fits exactly without getting the ellipsis drawn. If you add one more b character here the X character will overflow and the ellipsis will replace the Yccc rather than just the ccc. If you replace the ccc with ... then it is clear that there is enough space to hold them. I first thought that this had to do with deducing the ellipsis length and x position, but those are perfectly fine. My conclusion was that the last character is allowed to exceed the given width as we add tmpw to ew after checking. Note that this is different from text being allowed to bleed into the (assumed) right hand padding. For the second patch I am not sure if this would be bad form or not, but the invert variable is only ever used when text is rendered so I was wondering if this might be re-used for the purposes of containing the minw value when it is calculating text width. w = invert ? invert : ~invert; This would avoid the wrapper function and change of signature and simplify the patch quite a bit. It would probably be best to rename the parameter in that case. For the third patch I didn't have anything to note, seemed pretty straightforward. Thanks, -Stein On Wed, Mar 23, 2022 at 9:14 AM Hiltjo Posthuma <hil...@codemadness.org> wrote: > On Wed, Mar 23, 2022 at 12:26:24AM +0600, NRK wrote: > > On Tue, Mar 22, 2022 at 06:06:30PM +0100, Stein Gunnar Bakkeby wrote: > > > With the first patch the text is still allowed to bleed into the right > hand > > > side padding as long as it fits. > > > > > > I worked around that by reducing w with 2 * lpad and adding lpad to x > > > before returning. > > > > Ahh, sorry my bad. I missed the "adding lpad to return" part. > > Yeah, that seems to stop the prompt from getting cut off, but I as I > > said given the variable name `lpad`, I guess that's probably > > intentional. > > > > Let's wait for someone to clarify what the intentional behavior is. > > > > > I was thinking that the function drw_font_getexts might get removed, > it was > > > probably used for more in previous iterations. > > > > I made the patches with the assumption that they could/would also be > > integrated into libsl[0]. So I didn't remove anything or break the API. > > > > Please continue working on this patch, it is appreciated. > When it mostly works it can be put into libsl and dmenu and dwm. > > If possible please make the first iteration compatible with the current > API. > This would make reviewing easier. > > Afterwards it's no issue (although preferably not) if some API changes are > needed. > > Reducing the changes would make this easier to review, as this code and > also > Xft etc are finicky (as you probably can already tell :)). > > > [0]: https://git.suckless.org/libsl > > > > - NRK > > > > Thanks, > > -- > Kind regards, > Hiltjo > >