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 <hiltjo_AT_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
>
>
Received on Wed Mar 23 2022 - 11:13:00 CET