Re: [PATCH v10 04/13] utf8: add function to align a string into given strbuf

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int width,
>> +                    const char *s)
>> +{
>> +     int display_len = utf8_strnwidth(s, strlen(s), 0);
>> +     int utf8_compenstation = strlen(s) - display_len;
>
> compensation, perhaps?  But notice you are running two strlen and
> then also a call to utf8-strnwidth here already, and then
>

>> +     if (!strlen(s))
>> +             return;
>
> you return here without doing anything.
>
> Worse yet, this logic looks very strange.  If you give it a width of
> 8 because you want to align like-item on each record at that column,
> a record with 1-display-column item will be shown in 8-display-column
> with 7 SP padding (either at the beginning, or at the end, or on
> both ends to center).  If it happens to be an empty string, the entire
> 8-display-column disappears.
>
> Is that really what you meant to do?  The logic does not make much
> sense to me, even though the code may implement that logic correctly
> and clearly.
>

>> +     if (display_len >= width) {
>> +             strbuf_addstr(buf, s);
>> +             return;
>> +     }
>
> Mental note: this function values the information more than
> presentation; it refuses to truncate (to lose information) and
> instead accepts jaggy output.  OK.
>

>> +     if (position == ALIGN_LEFT)
>> +             strbuf_addf(buf, "%-*s", width + utf8_compenstation, s);
>> +     else if (position == ALIGN_MIDDLE) {
>> +             int left = (width - display_len)/2;
>> +             strbuf_addf(buf, "%*s%-*s", left, "", width - left + utf8_compenstation, s);
>> +     } else if (position == ALIGN_RIGHT)
>> +             strbuf_addf(buf, "%*s", width + utf8_compenstation, s);
>> +}