[pacman-dev] Cleaning up pretty printing in pacman.

[Dan McGee]

On Wed, Mar 6, 2013 at 12:44 PM, Simon Gomizelj <simongmzlj at gmail.com> wrote:
> While diving into pacman's output code to try and add colours I've
> noticed that pacman's pretty printing code is somewhat awkward. Its very
> clear its grow organically without ever really being cleaned up.
>
> - Translation messages shouldn't have formatting information embedded
>   into them:
>
>     _("Name"),    not _("Name           :")
>     _("Version"), not _("Version        :")
>     etc
>
> - Pacman puts a lot of effort into making some parts of its output is
>   nicely aligned, while other parts don't and expects padding to be
>   hardcoded. Pacman should do _all_ the aligning.
Agreed.
>
> An example of where pacman's pretty printing code is specifically
> inefficient... dump_pkg_full:
>
>     cols = getcols(fileno(stdout));
>
>     string_display(_("Name           :"), alpm_pkg_get_name(pkg), cols);
>     string_display(_("Version        :"), alpm_pkg_get_version(pkg), cols);
>     string_display(_("Description    :"), alpm_pkg_get_desc(pkg), cols);
>     string_display(_("Architecture   :"), alpm_pkg_get_arch(pkg), cols);
>     string_display(_("URL            :"), alpm_pkg_get_url(pkg), cols);
>     list_display(_("Licenses       :"), alpm_pkg_get_licenses(pkg), cols);
>     list_display(_("Groups         :"), alpm_pkg_get_groups(pkg), cols);
>     deplist_display(_("Provides       :"), alpm_pkg_get_provides(pkg), cols);
>     deplist_display(_("Depends On     :"), alpm_pkg_get_depends(pkg), cols);
>
> - cols is recalculated for each package.
> - string_display,list_display,etc call string_length on $1 each time.
>
> These two numbers could basically be considered constants though through
> the outputting.
string_length on the gettext string == useful optimization.

Please don't change the getcols logic though. This has been made cheap
enough and we do this quite on purpose. See something like the
download and install progress code- we call getcols() every 0.1
seconds at a minimum and this is to ensure resizing a console window
resizes the output you get on screen. While odd that someone would
resize during a full package listing, I don't think it is unusual for
us to accomidate it. If you profile and actually find this is a
significant slowness, we might consider it, but I would guess the
string_length changes are MUCH more effective.

> So this means when I do `pacman -Si` (output for ~6080
> packages here):
>
> - cols is recalculated ~6080 times too much.
> - string_display about (6080 * 20) or 121600 times.
>
> dump_pkg_full is basically outputting a table. There is no reason pacman
> couldn't pre-calculate the dimensions of the table to significantly
> reduce the amount of work it does when printing each row.
>
> Furthermore, there seems to be, with my colour patches, two more
> independent table implementations: table_display and
> add_table_row/display_transaction_sizes. They all build the same kind of
> table and format them in similar ways.
>
> I think the right way forward is to add a nice table formatter library
> to pacman that can be reused for everything. I whipped up a very quick
> prototype as a proof of concept: http://ix.io/4CP/
>
> I wanted to bring this up so that a) its on the record and b) we can
> start talking about if/how to implement this. I'm willing to put in the
> effort to get this through.
Haven't looked at the code yet but I like where this is going. Will
try to do that sometime and offer more concrete feedback.

>From a history perspective, the table stuff is super new while the
Qi/Si is nearly original code, with translation and whatnot added much
later.

-Dan