James Coglan <jcoglan@xxxxxxxxx> writes:
> - We don't want a general solution to this problem for everything
> `strbuf` could be used for; it only needs to address the graph
> padding problem.
Of course. Somebody may use strbuf to hold rows of a table and you
do not want to contaminate strbuf with fields like width of each
column etc, that are very specific to the application. IOW, strbuf
is merely _one_ component of a larger solution to each specific
problem, and the latter may be things like "struct graphbuf" like
Dscho suggested, which might use strbuf as an underlying
<byte-string, length> tuple mechanism, but that is an implementation
detail that should not be exposed to the users of the struct (and
that is why he did not call, and you should not call, the data
structure "graph-strbuf" or anything with "strbuf").
> - We only want to count printing characters, not color formatting sequences.
OK. But I'd phrase "count printing characters" as "measure display
width" for at least two reasons. Whitespaces are typically counted
as non-printing, but you do want to take them into account for this
application. Also the graph may not be limited to ASCII graphics
forever, and byte- or character-count may not match display width on
a fixed-width display.
> - We only need to consider the width of a small set of characters:
> { | / \ _ - . * }. We don't need to worry about Unicode, and the
> simple character counting in graph.c was working fine.
I have to warn you that we saw attempts to step outside these ASCII
graphics and use Unicode characters for prettier output in the past.
If you can do so without too much complexity, I'd suggest you try
not to close the door to those people who follow your footsteps to
further improve the system by pursuing the avenue further.
> To this end I've prepared a different implementation that
> introduces the indirection described above, and does not modify
> `strbuf`:
>
> +struct graph_strbuf {
> + struct strbuf *buf;
> + size_t width;
> +};
Is there a reason why you need a pointer to a strbuf that is
allocated separately? E.g. would it make it harder to manage
if the above were
struct graphbuf {
struct strbuf buf;
int width; /* display width in columns */
};
which is essentially what Dscho suggested?
> +static inline void graph_strbuf_addch(struct graph_strbuf *sb, int c)
> +{
> + strbuf_addch(sb->buf, c);
> + sb->width++;
> +}
> +
> +void graph_strbuf_addchars(struct graph_strbuf *sb, int c, size_t n)
> +{
> + strbuf_addchars(sb->buf, c, n);
> + sb->width += n;
> +}
> +
> +static inline void graph_strbuf_addstr(struct graph_strbuf *sb, const char *s)
> +{
> + strbuf_addstr(sb->buf, s);
> + sb->width += strlen(s);
> +}
I'd probably introduce another helper that takes color code and
graphbuf (also notice how I name the variables and types; calling
something sb that is not a strbuf is misleading and inviting
unnecessary bugs):
static inline void graphbuf_addcolor(struct graphbuf *gb, unsigned short color)
{
strbuf_addstr(gb->buf, column_get_color_code(color));
}
if I were writing this code. The goal is to make any strbuf_add*()
that is done directly on gb->buf outside these helpers a bug--that
way, grepping for strbuf_add* in this file would become a very easy
way to catch such a bug that bypasses the graphbuf_add*() layer and
potentially screw up the column counting.
Other than these three points (i.e. naming without "strbuf" that is
an implementation detail, embedded vs pointer of strbuf in the
graphbuf, and making sure everything can be done via graphbuf_add*()
wrappers to make it easier to spot bugs), it seems you are going in
the right direction. Thanks for working on this.
