[pacman-dev] [PATCH] Added better colourisation support for the "-{Q, S}i" operation.

[Xavion]

Hi Allan,


> diff --git a/src/pacman/callback.c b/src/pacman/callback.c
>> index 1e1a4cd..3e4d7fa 100644
>> --- a/src/pacman/callback.c
>> +++ b/src/pacman/callback.c
>> @@ -427,6 +427,7 @@ void cb_question(alpm_question_t *question)
>>                               alpm_question_remove_pkgs_t *q =
>> &question->remove_pkgs;
>>                               alpm_list_t *namelist = NULL, *i;
>>                               size_t count = 0;
>> +                             const colstr_t *colstr = &config->colstr;
>>                               for(i = q->packages; i; i = i->next) {
>>                                       namelist = alpm_list_add(namelist,
>>                                                       (char
>> *)alpm_pkg_get_name(i->data));
>> @@ -436,7 +437,7 @@ void cb_question(alpm_question_t *question)
>>                                                       "The following
>> package cannot be upgraded due to unresolvable dependencies:\n",
>>                                                       "The following
>> packages cannot be upgraded due to unresolvable dependencies:\n",
>>                                                       count));
>> -                             list_display("     ", namelist, getcols());
>> +                             list_display("     ", namelist,
>> colstr->nocolor, getcols());
>
>
> Unrelated...
>

I increased the "list_display()" function's parameter count from three to
four.  This means all calls to that function - even from unrelated areas of
the code - needed to be updated as well.

diff --git a/src/pacman/package.c b/src/pacman/package.c
>> index 3ab9abc..92f6b37 100644
>> --- a/src/pacman/package.c
>> +++ b/src/pacman/package.c
>> @@ -148,14 +148,14 @@ static void make_aligned_titles(void)
>>   * @param deps a list with items of type alpm_depend_t
>>   */
>>  static void deplist_display(const char *title,
>> -             alpm_list_t *deps, unsigned short cols)
>> +             alpm_list_t *deps, const char *colour, unsigned short cols)
>
>
> Why is the parameter added?   It is only ever called with colstr->nocolor
>

Doing it this way allows future expansion, and it looks more consistent (in
the "dump_pkg_full()" function) as well.  If I were to hard-code
"colstr->nocolor" inside "deplist_display()", it would be stuck without
colour forever.

Plus, we wouldn't economise on lines of code by doing it that way either,
as "const colstr_t *colstr = &config->colstr;" would have to be added
inside "deplist_display()" as well.

I stopped reviewing here.
>
> Also, this patch did not apply on top of the group selection dialog
> colouring patch despite being later in the series.
>

There didn't seem to be a way around this.  The "display_repo_list()"
function in "util.c" has overlapping changes in both patches.  It wouldn't
have mattered which order they were listed in: they still would have
conflicted with each other.

To follow up this patch.   I have pulled the changes for the group
> selection dialog and the -Qo/-Fo output.


If your only beef is with this patch, why pull the other ones as well?  I
split them away - like you asked me to - so that they're completely
unrelated to this one.  In fact, that's precisely what has caused the
abovementioned conflict.

It is unlikely that I will ever accept the -S/Qi colour patches as it is
> too intrusive - at least in its current format.


I don't think there is a (practical) way of implementing this in a less
intrusive manner.  I originally had the new code in separate functions and
it looked too cumbersome.  Having said that, you (or anyone else) are
welcome to suggest an alternative method if desired.

Unless there is something simple that has not been submitted, this is as
> far as I am willing to go on adding colour for now.  Any other changes
> should first be discussed in a separate thread.


After reading what I've written above, please reconsider your stance on
this.  I've gone to a lot of trouble with these patches and I think many
'pacman' users are (secretly) hoping that they'll be accepted/included.


-- 
Regards, Xavion.