[pacman-dev] [PATCH v2 3/3] Align titles automatically in information display

[Pierre Neidhardt]

On 15-10-19 17:22:55, Dave Reisner wrote:
> On Sat, Oct 17, 2015 at 07:02:57PM +0200, Pierre Neidhardt wrote:
> > +/* we truncate titles longer than this arbitrary value */
> 
> Well no, it isn't arbitrary, you said so yourself. Why not state this?
> Something like:
> 
> "As of such and such a date, the longest relevant string from the PO
> files was measured to be X characters long. We set this to 50 to allow
> for some potential growth."
> 
> > +#define TITLE_MAXLEN 50
> > +
> > +/* A title is referenced by titles[T_XXX]. The T_ prefix avoids shadowing. */
> 
> Not a useful comment. Nowhere in this file do you explain what a "title"
> is.

The term "title" is used (not only by me) here and there throughout the code.
(See 'util.c'.) I thought this was a given, but it does not cost much to explain
here what it is. What is more useful though is to explain what an "aligned
title" is.

> The fact that you use a prefix is quite obviously done to make for
> unique enum identifiers.
> 
> > +enum {
> > +	T_ARCHITECTURE = 0,
> > +	T_BACKUP_FILES,
> > +	T_BUILD_DATE,
> > +	T_COMPRESSED_SIZE,
> > +	T_CONFLICTS_WITH,
> > +	T_DEPENDS_ON,
> > +	T_DESCRIPTION,
> > +	T_DOWNLOAD_SIZE,
> > +	T_GROUPS,
> > +	T_INSTALL_DATE,
> > +	T_INSTALL_REASON,
> > +	T_INSTALL_SCRIPT,
> > +	T_INSTALLED_SIZE,
> > +	T_LICENSES,
> > +	T_MD5_SUM,
> > +	T_NAME,
> > +	T_OPTIONAL_DEPS,
> > +	T_OPTIONAL_FOR,
> > +	T_PACKAGER,
> > +	T_PROVIDES,
> > +	T_REPLACES,
> > +	T_REPOSITORY,
> > +	T_REQUIRED_BY,
> > +	T_SHA_256_SUM,
> > +	T_SIGNATURES,
> > +	T_URL,
> > +	T_VALIDATED_BY,
> > +	T_VERSION,
> > +	TITLE_COUNT
> 
> Please comment that this is a sentinel value and needs to be last...

OK.

> I'd also love to claim that this needs to share the same prefix as the enums
> it belongs to (and have an underscore prefix), but there's not really any
> prevailing coding style in ALPM that I can lean on to "enforce" that.

Agreed, but I prefer keeping underscore prefixes for library code. This is not
ALPM but the pacman frontend if this is what you meant.

> > +};
> > +
> > +static char titles[TITLE_COUNT][TITLE_MAXLEN * sizeof(wchar_t)];
> > +
> > +/** Truncate all titles longer than TITLE_MAX and pad the rest with spaces so
> > + * that they align with the longest title. " :" is appended to all titles.
> > + *
> > + * Truncation allows us to avoid allocations and clean up code.
>
> This isn't a useful comment. It's stack allocation which avoids the
> cleanup and allocation, not truncation. Being forced to truncate is a
> side effect of using stack allocating.

Agreed.

> Perhaps something like:
>
> "Build an array of localized strings used to create aligned titles in
> verbose package information. Storage for strings is stack allocated and
> naively truncated to TITLE_MAXLEN characters."

Your version is a bit misleading: we could think that the alignment happens
outside this function. It misses an explanation for what "aligning" titles
precisely means. I'd say:

> Build the `titles` array of localized titles and pad them with spaces so
> that they align with the longest title. Storage for strings is stack
> allocated and naively truncated to TITLE_MAXLEN characters."

-- 
Pierre Neidhardt

It is better to have loved and lost than just to have lost.