Hi All,
It's great that "pacman" finally has native colour support in v4.1.0. It's just a shame that it's not as good as that of "pacman-color". Without meaning any offence, it falls way short of the mark IMHO.
I'm hoping you can just replicate the colourisation of "pacman-color". For your convenience, I've attached the relevant files you'd need. I'm only requesting this because "pacman-color" no longer exists.
On 07/04/13 20:10, Xavion wrote:
Hi All,
It's great that "pacman" finally has native colour support in v4.1.0. It's just a shame that it's not as good as that of "pacman-color". Without meaning any offence, it falls way short of the mark IMHO.
I'm hoping you can just replicate the colourisation of "pacman-color". For your convenience, I've attached the relevant files you'd need. I'm only requesting this because "pacman-color" no longer exists.
That patch is a steaming pile of hackery. So the color output needed to be rewritten to become included in pacman.
Rather than just attaching a ~1300 line file that no longer applies in any way and stating that it is better, you might like to actually state what is currently missing?
Allan
Hi Allan,
Tell me, did you ever actually use "pacman-color" before it was killed? If so, it should be pretty obvious what's currently missing from "pacman". I attached said "steaming pile of hackery" to give you the complete picture.
For starters, here are four screenshots http://imgur.com/a/DL4kycomparing "pacman" to "pacman-color". As you can see, the colourisation of the latter is far superior to that of the former. Accordingly, human brains can extract information faster from the latter's output.
On Tue, Apr 09, 2013 at 07:57:45AM +1000, Xavion wrote:
Hi Allan,
Tell me, did you ever actually use "pacman-color" before it was killed? If so, it should be pretty obvious what's currently missing from "pacman". I attached said "steaming pile of hackery" to give you the complete picture.
For starters, here are four screenshots http://imgur.com/a/DL4kycomparing "pacman" to "pacman-color". As you can see, the colourisation of the latter is far superior to that of the former. Accordingly, human brains can extract information faster from the latter's output.
This all depends on what you think is superior. You could probably get scientific studies that say that one is better than the other, but it's a matter of opinion. If you want, you can colorize those other strings, if I'm not mistaken, it wouldn't take too much to do that even. I'd suggest submitting some patches that improve the color output rather than just complaining.
Thanks,
Are you seriously trying to convince me that anyone in their right mind would choose the colourisation of "pacman" over that of "pacman-color"?
I wouldn't be complaining if you guys hadn't killed off "pacman-color". Similarly, the responsibility shouldn't be on me to reinvent the wheel for you.
On 09/04/13 17:07, Xavion wrote:
I wouldn't be complaining if you guys hadn't killed off "pacman-color". Similarly, the responsibility shouldn't be on me to reinvent the wheel for you.
We had nothing to do with pacman-color, so you can go find that developer and convince them not to stop it.
Otherwise, you can be constructive and either 1) submit patches for improvements you would like, or 2) provide details of areas you think need improved (and I mean in written format, not with screenshots).
Allan
Hi All,
I finally garnered the willpower to get off my arse and do something about this. I've attached a patch that adds colourised output to the information commands. I've tried hard to ensure that the style and location of my code is suitable.
As you can tell from the attached screenshot, these colours surpass those of "pacman-color". Once you've accepted my changes, I'll start adding to the colourisation of the upgrade commands. In the meantime, let me know if you want me to be doing something differently.
On 28/05/13 13:24, Xavion wrote:
Hi All,
I finally garnered the willpower to get off my arse and do something about this. I've attached a patch that adds colourised output to the information commands. I've tried hard to ensure that the style and location of my code is suitable.
As you can tell from the attached screenshot, these colours surpass those of "pacman-color". Once you've accepted my changes, I'll start adding to the colourisation of the upgrade commands. In the meantime, let me know if you want me to be doing something differently.
Thanks for the patch. Two quick comments:
1) please send patches inline (via "git send-email" is best) as it allows up to comment on the more easily.
2) There is a bunch of patches waiting for me to look at in more detail (see [1-3]) which alter this area of the code to keep the output nicely aligned, even with translations. This patch will need rebased on top of those at some stage. So I suggest waiting on those to be merged (hopefully soon...)
[1] https://patchwork.archlinux.org/patch/1131/ [2] https://patchwork.archlinux.org/patch/1132/ [3] https://patchwork.archlinux.org/patch/1133/
Thanks for the prompt and detailed reply. I will happily wait for those patches to be merged, and then modify mine if necessary. I will also look into resubmitting it, and any future ones, via Git. As I haven't done any collaborative coding in several years, there's been no need to advance beyond Subversion :-).
Also, I've thought of two other things since sending my last email. Would you prefer that I merge my new code into the "string_display()" and "list_display()" functions. Doing so would make my patch less of a band-aid solution. I didn't do this at first in case you didn't want me messing with those functions.
Secondly, you'll notice that the "Description" text was yellow in the screenshot attached to my last email. Similarly, I'd like to make the "Conflicts With" text red if you're okay with it. These colours are normally used for warnings and errors respectively. Are you happy for me to reuse them for different reasons here?
I've discovered that the patch I submitted earlier today fails if the string or list is empty. I've modified the code to prevent this, but will wait until those other patches you mentioned are merged before resubmitting mine.
The other thing I've been thinking is that perhaps it's better to use the yellow text for the "Replaces" array. This is kind of like a warning, while having the "Conflicts With" array in red symbolises something stronger (i.e. like an error).
If the "Description" is coloured yellow in the information output, it probably should look the same in the search output as well. I'm thinking you guys might not want to go down that path, which strengthens the case for using yellow in "Replaces" instead.
On 28/05/13 17:43, Xavion wrote:
I've discovered that the patch I submitted earlier today fails if the string or list is empty. I've modified the code to prevent this, but will wait until those other patches you mentioned are merged before resubmitting mine.
The other thing I've been thinking is that perhaps it's better to use the yellow text for the "Replaces" array. This is kind of like a warning, while having the "Conflicts With" array in red symbolises something stronger (i.e. like an error).
If the "Description" is coloured yellow in the information output, it probably should look the same in the search output as well. I'm thinking you guys might not want to go down that path, which strengthens the case for using yellow in "Replaces" instead.
The search output is already too much of a rainbow. So I agree with using yellow for replaces and red for conflicts.
I'd suggest splitting your patch into two parts. The first being adding the set-up for adding colours and the second changing the output colours.
Allan
On Tue, May 28, 2013 at 04:09:49PM +1000, Xavion wrote:
Thanks for the prompt and detailed reply. I will happily wait for those patches to be merged, and then modify mine if necessary. I will also look into resubmitting it, and any future ones, via Git. As I haven't done any collaborative coding in several years, there's been no need to advance beyond Subversion :-).
Also, I've thought of two other things since sending my last email. Would you prefer that I merge my new code into the "string_display()" and "list_display()" functions. Doing so would make my patch less of a band-aid solution. I didn't do this at first in case you didn't want me messing with those functions.
Secondly, you'll notice that the "Description" text was yellow in the screenshot attached to my last email. Similarly, I'd like to make the "Conflicts With" text red if you're okay with it. These colours are normally used for warnings and errors respectively. Are you happy for me to reuse them for different reasons here?
Hi, I did most of the work on pacman's colour support and the author of the patches Allan pointed out to you.
I'm actually looking to completely remove the *_display family of functions. They're old and inefficient and need to replaced with something far less clunky to represent tables (pacman already has three unique table representations in the code base. I'm currently working to merge too of them).
Hi Simon,
Thanks for the heads up about those forthcoming changes. I'll be happy to modify my code as necessary to fit the new style.
I realise that the finesse elements, like colourisation, take a back seat to structural solidification. Plus, I'll have more spare time on my hands once the French Open and NBA Finals are done.
Dear All,
I'm very sorry that it has taken me so long to come back to this thread. I got bogged down with other things, and there was also the procrastination factor at play.
I have remerged my colourisation patch into the latest (Git) 'pacman' codebase. I will attempt to post it to this thread via "git format-patch" soon. I'm new to that command, so let's hope nothing goes wrong.
Refer to these screenshots http://imgur.com/a/DL4ky for a visual on how certain pacman operations will look if my patch is accepted. If so, and when time permits, I can look into the prospect of colourising further pacman functionality.
From: Xavion <Xavion (dot) 0 (at) Gmail (dot) com>
Signed-off-by: Xavion <Xavion (dot) 0 (at) Gmail (dot) com> --- src/pacman/callback.c | 11 +++++++---- src/pacman/package.c | 55 ++++++++++++++++++++++++++------------------------- src/pacman/pacman.c | 3 ++- src/pacman/sync.c | 7 ++++--- src/pacman/util.c | 29 +++++++++++++++++---------- src/pacman/util.h | 4 ++-- 6 files changed, 62 insertions(+), 47 deletions(-)
diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 1e1a4cd..5ca1166 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -149,7 +149,8 @@ static void fill_progress(const int bar_percent, const int disp_percent, /* print display percent after progress bar */ /* 5 = 1 space + 3 digits + 1 % */ if(proglen >= 5) { - printf(" %3d%%", disp_percent); + const colstr_t *colstr = &config->colstr; + printf(" %s%3d%%%s", colstr->meta, disp_percent, colstr->nocolor); }
if(bar_percent == 100) { @@ -436,7 +437,8 @@ 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()); + const colstr_t *colstr = &config->colstr; + list_display(" ", namelist, colstr->nocolor, getcols()); printf("\n"); q->skip = noyes(_n( "Do you want to skip the above package for this upgrade?", @@ -618,8 +620,9 @@ void cb_progress(alpm_progress_t event, const char *pkgname, int percent,
}
- printf("(%*zu/%*zu) %ls%-*s", digits, current, - digits, howmany, wcstr, padwid, ""); + const colstr_t *colstr = &config->colstr; + printf("%s(%*zu/%*zu)%s %ls%-*s", colstr->groups, digits, current, + digits, howmany, colstr->nocolor, wcstr, padwid, "");
free(wcstr);
diff --git a/src/pacman/package.c b/src/pacman/package.c index 3ab9abc..de6ebb0 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) { alpm_list_t *i, *text = NULL; for(i = deps; i; i = alpm_list_next(i)) { alpm_depend_t *dep = i->data; text = alpm_list_add(text, alpm_dep_compute_string(dep)); } - list_display(title, text, cols); + list_display(title, text, colour, cols); FREELIST(text); }
@@ -256,29 +256,30 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) }
cols = getcols(); + const colstr_t *colstr = &config->colstr;
/* actual output */ if(from == ALPM_PKG_FROM_SYNCDB) { string_display(titles[T_REPOSITORY], - alpm_db_get_name(alpm_pkg_get_db(pkg)), cols); + alpm_db_get_name(alpm_pkg_get_db(pkg)), colstr->repo, cols); } - string_display(titles[T_NAME], alpm_pkg_get_name(pkg), cols); - string_display(titles[T_VERSION], alpm_pkg_get_version(pkg), cols); - string_display(titles[T_DESCRIPTION], alpm_pkg_get_desc(pkg), cols); - string_display(titles[T_ARCHITECTURE], alpm_pkg_get_arch(pkg), cols); - string_display(titles[T_URL], alpm_pkg_get_url(pkg), cols); - list_display(titles[T_LICENSES], alpm_pkg_get_licenses(pkg), cols); - list_display(titles[T_GROUPS], alpm_pkg_get_groups(pkg), cols); - deplist_display(titles[T_PROVIDES], alpm_pkg_get_provides(pkg), cols); - deplist_display(titles[T_DEPENDS_ON], alpm_pkg_get_depends(pkg), cols); + string_display(titles[T_NAME], alpm_pkg_get_name(pkg), colstr->title, cols); + string_display(titles[T_VERSION], alpm_pkg_get_version(pkg), colstr->version, cols); + string_display(titles[T_DESCRIPTION], alpm_pkg_get_desc(pkg), colstr->nocolor, cols); + string_display(titles[T_ARCHITECTURE], alpm_pkg_get_arch(pkg), colstr->nocolor, cols); + string_display(titles[T_URL], alpm_pkg_get_url(pkg), colstr->meta, cols); + list_display(titles[T_LICENSES], alpm_pkg_get_licenses(pkg), colstr->nocolor, cols); + list_display(titles[T_GROUPS], alpm_pkg_get_groups(pkg), colstr->groups, cols); + deplist_display(titles[T_PROVIDES], alpm_pkg_get_provides(pkg), colstr->nocolor, cols); + deplist_display(titles[T_DEPENDS_ON], alpm_pkg_get_depends(pkg), colstr->nocolor, cols); optdeplist_display(pkg, cols);
if(extra || from == ALPM_PKG_FROM_LOCALDB) { - list_display(titles[T_REQUIRED_BY], requiredby, cols); - list_display(titles[T_OPTIONAL_FOR], optionalfor, cols); + list_display(titles[T_REQUIRED_BY], requiredby, colstr->nocolor, cols); + list_display(titles[T_OPTIONAL_FOR], optionalfor, colstr->nocolor, cols); } - deplist_display(titles[T_CONFLICTS_WITH], alpm_pkg_get_conflicts(pkg), cols); - deplist_display(titles[T_REPLACES], alpm_pkg_get_replaces(pkg), cols); + deplist_display(titles[T_CONFLICTS_WITH], alpm_pkg_get_conflicts(pkg), colstr->err, cols); + deplist_display(titles[T_REPLACES], alpm_pkg_get_replaces(pkg), colstr->warn, cols);
size = humanize_size(alpm_pkg_get_size(pkg), '\0', 2, &label); if(from == ALPM_PKG_FROM_SYNCDB) { @@ -296,15 +297,15 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) printf("%s%s%s %.2f %s\n", config->colstr.title, titles[T_INSTALLED_SIZE], config->colstr.nocolor, size, label);
- string_display(titles[T_PACKAGER], alpm_pkg_get_packager(pkg), cols); - string_display(titles[T_BUILD_DATE], bdatestr, cols); + string_display(titles[T_PACKAGER], alpm_pkg_get_packager(pkg), colstr->nocolor, cols); + string_display(titles[T_BUILD_DATE], bdatestr, colstr->nocolor, cols); if(from == ALPM_PKG_FROM_LOCALDB) { - string_display(titles[T_INSTALL_DATE], idatestr, cols); - string_display(titles[T_INSTALL_REASON], reason, cols); + string_display(titles[T_INSTALL_DATE], idatestr, colstr->nocolor, cols); + string_display(titles[T_INSTALL_REASON], reason, colstr->nocolor, cols); } if(from == ALPM_PKG_FROM_FILE || from == ALPM_PKG_FROM_LOCALDB) { string_display(titles[T_INSTALL_SCRIPT], - alpm_pkg_has_scriptlet(pkg) ? _("Yes") : _("No"), cols); + alpm_pkg_has_scriptlet(pkg) ? _("Yes") : _("No"), colstr->nocolor, cols); }
if(from == ALPM_PKG_FROM_SYNCDB && extra) { @@ -321,25 +322,25 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) keys = alpm_list_add(keys, _("None")); }
- string_display(titles[T_MD5_SUM], alpm_pkg_get_md5sum(pkg), cols); - string_display(titles[T_SHA_256_SUM], alpm_pkg_get_sha256sum(pkg), cols); - list_display(titles[T_SIGNATURES], keys, cols); + string_display(titles[T_MD5_SUM], alpm_pkg_get_md5sum(pkg), colstr->nocolor, cols); + string_display(titles[T_SHA_256_SUM], alpm_pkg_get_sha256sum(pkg), colstr->nocolor, cols); + list_display(titles[T_SIGNATURES], keys, colstr->nocolor, cols);
if(base64_sig) { FREELIST(keys); } } else { - list_display(titles[T_VALIDATED_BY], validation, cols); + list_display(titles[T_VALIDATED_BY], validation, colstr->nocolor, cols); }
if(from == ALPM_PKG_FROM_FILE) { alpm_siglist_t siglist; int err = alpm_pkg_check_pgp_signature(pkg, &siglist); if(err && alpm_errno(config->handle) == ALPM_ERR_SIG_MISSING) { - string_display(titles[T_SIGNATURES], _("None"), cols); + string_display(titles[T_SIGNATURES], _("None"), colstr->nocolor, cols); } else if(err) { string_display(titles[T_SIGNATURES], - alpm_strerror(alpm_errno(config->handle)), cols); + alpm_strerror(alpm_errno(config->handle)), colstr->err, cols); } else { signature_display(titles[T_SIGNATURES], &siglist, cols); } diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index be52d1b..e42e9fa 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -1237,7 +1237,8 @@ int main(int argc, char *argv[]) printf("Lock File : %s\n", alpm_option_get_lockfile(config->handle)); printf("Log File : %s\n", alpm_option_get_logfile(config->handle)); printf("GPG Dir : %s\n", alpm_option_get_gpgdir(config->handle)); - list_display("Targets :", pm_targets, 0); + const colstr_t *colstr = &config->colstr; + list_display("Targets :", pm_targets, colstr->nocolor, 0); }
/* Log command line */ diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 041b6b2..f2ca125 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -557,9 +557,10 @@ static int process_group(alpm_list_t *dbs, const char *group, int error) if(config->print == 0) { char *array = malloc(count); int n = 0; - colon_printf(_n("There is %d member in group %s:\n", - "There are %d members in group %s:\n", count), - count, group); + const colstr_t *colstr = &config->colstr; + colon_printf(_n("There is %d member in group %s%s%s:\n", + "There are %d members in group %s%s%s:\n", count), + count, colstr->groups, group, colstr->title); select_display(pkgs); if(!array) { ret = 1; diff --git a/src/pacman/util.c b/src/pacman/util.c index 0862de0..08526d8 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -473,7 +473,7 @@ static void add_transaction_sizes_row(alpm_list_t **rows, char *label, off_t siz *rows = alpm_list_add(*rows, row); }
-void string_display(const char *title, const char *string, unsigned short cols) +void string_display(const char *title, const char *string, const char *colour, unsigned short cols) { if(title) { printf("%s%s%s ", config->colstr.title, title, config->colstr.nocolor); @@ -483,7 +483,10 @@ void string_display(const char *title, const char *string, unsigned short cols) } else { /* compute the length of title + a space */ size_t len = string_length(title) + 1; - indentprint(string, (unsigned short)len, cols); + char *clrd_str = NULL; + pm_asprintf(&clrd_str, "%s%s", colour, string); + indentprint(clrd_str, (unsigned short)len, cols); + free(clrd_str); } printf("\n"); } @@ -654,7 +657,7 @@ cleanup: }
void list_display(const char *title, const alpm_list_t *list, - unsigned short maxcols) + const char *colour, unsigned short maxcols) { const alpm_list_t *i; size_t len = 0; @@ -669,7 +672,9 @@ void list_display(const char *title, const alpm_list_t *list, } else { size_t cols = len; const char *str = list->data; - fputs(str, stdout); + char *clrd_str = NULL; + pm_asprintf(&clrd_str, "%s%s", colour, str); + fputs(clrd_str, stdout); cols += string_length(str); for(i = alpm_list_next(list); i; i = alpm_list_next(i)) { str = i->data; @@ -687,10 +692,12 @@ void list_display(const char *title, const alpm_list_t *list, printf(" "); cols += 2; } - fputs(str, stdout); + pm_asprintf(&clrd_str, "%s%s", colour, str); + fputs(clrd_str, stdout); cols += s; } putchar('\n'); + free(clrd_str); } }
@@ -914,7 +921,8 @@ static void _display_targets(alpm_list_t *targets, int verbose) }
/* print to screen */ - pm_asprintf(&str, "%s (%zu)", _("Packages"), alpm_list_count(targets)); + const colstr_t *colstr = &config->colstr; + pm_asprintf(&str, "%s%s (%zu):", colstr->warn, _("Packages"), alpm_list_count(targets)); printf("\n");
cols = getcols(); @@ -922,10 +930,10 @@ static void _display_targets(alpm_list_t *targets, int verbose) header = create_verbose_header(alpm_list_count(targets)); if(table_display(header, rows, cols) != 0) { /* fallback to list display if table wouldn't fit */ - list_display(str, names, cols); + list_display(str, names, colstr->nocolor, cols); } } else { - list_display(str, names, cols); + list_display(str, names, colstr->nocolor, cols); } printf("\n");
@@ -1256,8 +1264,9 @@ static void display_repo_list(const char *dbname, alpm_list_t *list, { const char *prefix = " ";
- colon_printf(_("Repository %s\n"), dbname); - list_display(prefix, list, cols); + const colstr_t *colstr = &config->colstr; + colon_printf(_("Repository %s%s\n"), colstr->repo, dbname); + list_display(prefix, list, colstr->nocolor, cols); }
void select_display(const alpm_list_t *pkglist) diff --git a/src/pacman/util.h b/src/pacman/util.h index f5e37c8..86aa204 100644 --- a/src/pacman/util.h +++ b/src/pacman/util.h @@ -55,10 +55,10 @@ void columns_cache_reset(void); int rmrf(const char *path); void indentprint(const char *str, unsigned short indent, unsigned short cols); char *strreplace(const char *str, const char *needle, const char *replace); -void string_display(const char *title, const char *string, unsigned short cols); +void string_display(const char *title, const char *string, const char *colour, unsigned short cols); double humanize_size(off_t bytes, const char target_unit, int precision, const char **label); -void list_display(const char *title, const alpm_list_t *list, +void list_display(const char *title, const alpm_list_t *list, const char *colour, unsigned short maxcols); void list_display_linebreak(const char *title, const alpm_list_t *list, unsigned short maxcols);
On 24/03/16 09:31, Xavion wrote:
Dear All,
I'm very sorry that it has taken me so long to come back to this thread. I got bogged down with other things, and there was also the procrastination factor at play.
I have remerged my colourisation patch into the latest (Git) 'pacman' codebase. I will attempt to post it to this thread via "git format-patch" soon. I'm new to that command, so let's hope nothing goes wrong.
Refer to these screenshots http://imgur.com/a/DL4ky for a visual on how certain pacman operations will look if my patch is accepted. If so, and when time permits, I can look into the prospect of colourising further pacman functionality.
It is difficult to judge the screenshots without side-by-side current vs patched. But from what I can see, most of the colour used is for the sake of having colour.
Colour should be used to highlight the important information. How do you decide the URL is more important than the dependencies for -Si? Why is the number of packages being installed highlighted yellow when that is only used for warnings?
I'd like to see individual changes proposed, with justification of how they improve the readability of the pacman output.
A
Hi Allan,
Thanks for the same-day reply. It's been a few years, so I'm guessing everyone's memory is a bit patchy (excuse the pun). If you take a look earlier in this thread, you'll see that you've (basically) already approved these changes.
It is difficult to judge the screenshots without side-by-side current vs
patched. But from what I can see, most of the colour used is for the sake of having colour.
To make things easier for you, I've just added screenshots showing the current 'pacman' (colourised) output to the album http://imgur.com/a/DL4ky. The main purpose of having the extra colours is to make it easier for the brain to pick out the important bits (as noted earlier in this thread).
Colour should be used to highlight the important information. How do you decide the URL is more important than the dependencies for -Si? Why is the number of packages being installed highlighted yellow when that is only used for warnings?
As mentioned at/near the start of the thread, I'm mainly trying to reproduce the old "pacman-color" output. Having the URL in cyan and the dependencies uncoloured is the way that "pacman-color" used to do it. Similarly, "pacman-color" used to highlight the number of packages to be installed in yellow.
I'd like to see individual changes proposed, with justification of how they improve the readability of the pacman output.
Most of this has already been discussed earlier in the thread. Have a read of it when you get time and feel free to respond however you like. I realise that 'pacman' is your baby, and giving it tattoos is a big step :-).
On 24/03/16 12:53, Xavion wrote:
Hi Allan,
Thanks for the same-day reply. It's been a few years, so I'm guessing everyone's memory is a bit patchy (excuse the pun). If you take a look earlier in this thread, you'll see that you've (basically) already approved these changes.
I have become old and grumpy since then!
Lets start with what is already justified by what we already have in pacman with the aim of improving consistency of our current colour scheme. Further additions will need to be discussed separately.
package names - bold (in some places...) groups - blue repos - magenta versions - green
So, I'd like separate patches:
-Si/-Qi: just those changes -S group dialog: just those changes
-Qo/-Fo can have the same done and anywhere else that is currently not consistent.
Allan
Hi Allan,
I have become old and grumpy since then!
Okay, point taken: I did let it drag on a bit. It wasn't all my fault, though: I got sick of waiting for those other three patches to be merged in.
Lets start with what is already justified by what we already have in pacman with the aim of improving consistency of our current colour scheme. Further additions will need to be discussed separately.
package names - bold (in some places...) groups - blue repos - magenta versions - green
So, I'd like separate patches:
-Si/-Qi: just those changes -S group dialog: just those changes
Righto, you're the boss. I've just created a patch for those switches alone; I will post it to this thread soon. I'll get back to you about the other colours at a later date.
-Qo/-Fo can have the same done and anywhere else that is currently not consistent.
As far as I can tell, the following switches need better colourisation. Let me know if you disagree with any of this *before* I go ahead and create the patch :-).
- -Dk, -Dkk: the package name (but not the dependency) should be in bold, rather than quoted - -Qc: the package name should be in bold, rather than quoted - -Qg, -Sg: the group name should be in blue; the package name should be in bold - -Qk, -Qkk: the package name should be in bold - -Qo, -Fo: the package name should be in bold; the version number should be in green - -Qq, -Fq, -Sq: the package name should (possibly) be in bold - -T: the package name should (possibly) be in bold
From: Xavion <Xavion (dot) 0 (at) Gmail (dot) com>
Signed-off-by: Xavion <Xavion (dot) 0 (at) Gmail (dot) com> --- src/pacman/callback.c | 3 ++- src/pacman/package.c | 55 ++++++++++++++++++++++++++------------------------- src/pacman/pacman.c | 3 ++- src/pacman/sync.c | 7 ++++--- src/pacman/util.c | 29 +++++++++++++++++---------- src/pacman/util.h | 4 ++-- 6 files changed, 57 insertions(+), 44 deletions(-)
diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 1e1a4cd..f7c69e2 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -436,7 +436,8 @@ 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()); + const colstr_t *colstr = &config->colstr; + list_display(" ", namelist, colstr->nocolor, getcols()); printf("\n"); q->skip = noyes(_n( "Do you want to skip the above package for this upgrade?", diff --git a/src/pacman/package.c b/src/pacman/package.c index 3ab9abc..fab1a5c 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) { alpm_list_t *i, *text = NULL; for(i = deps; i; i = alpm_list_next(i)) { alpm_depend_t *dep = i->data; text = alpm_list_add(text, alpm_dep_compute_string(dep)); } - list_display(title, text, cols); + list_display(title, text, colour, cols); FREELIST(text); }
@@ -256,29 +256,30 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) }
cols = getcols(); + const colstr_t *colstr = &config->colstr;
/* actual output */ if(from == ALPM_PKG_FROM_SYNCDB) { string_display(titles[T_REPOSITORY], - alpm_db_get_name(alpm_pkg_get_db(pkg)), cols); + alpm_db_get_name(alpm_pkg_get_db(pkg)), colstr->repo, cols); } - string_display(titles[T_NAME], alpm_pkg_get_name(pkg), cols); - string_display(titles[T_VERSION], alpm_pkg_get_version(pkg), cols); - string_display(titles[T_DESCRIPTION], alpm_pkg_get_desc(pkg), cols); - string_display(titles[T_ARCHITECTURE], alpm_pkg_get_arch(pkg), cols); - string_display(titles[T_URL], alpm_pkg_get_url(pkg), cols); - list_display(titles[T_LICENSES], alpm_pkg_get_licenses(pkg), cols); - list_display(titles[T_GROUPS], alpm_pkg_get_groups(pkg), cols); - deplist_display(titles[T_PROVIDES], alpm_pkg_get_provides(pkg), cols); - deplist_display(titles[T_DEPENDS_ON], alpm_pkg_get_depends(pkg), cols); + string_display(titles[T_NAME], alpm_pkg_get_name(pkg), colstr->title, cols); + string_display(titles[T_VERSION], alpm_pkg_get_version(pkg), colstr->version, cols); + string_display(titles[T_DESCRIPTION], alpm_pkg_get_desc(pkg), colstr->nocolor, cols); + string_display(titles[T_ARCHITECTURE], alpm_pkg_get_arch(pkg), colstr->nocolor, cols); + string_display(titles[T_URL], alpm_pkg_get_url(pkg), colstr->nocolor, cols); + list_display(titles[T_LICENSES], alpm_pkg_get_licenses(pkg), colstr->nocolor, cols); + list_display(titles[T_GROUPS], alpm_pkg_get_groups(pkg), colstr->groups, cols); + deplist_display(titles[T_PROVIDES], alpm_pkg_get_provides(pkg), colstr->nocolor, cols); + deplist_display(titles[T_DEPENDS_ON], alpm_pkg_get_depends(pkg), colstr->nocolor, cols); optdeplist_display(pkg, cols);
if(extra || from == ALPM_PKG_FROM_LOCALDB) { - list_display(titles[T_REQUIRED_BY], requiredby, cols); - list_display(titles[T_OPTIONAL_FOR], optionalfor, cols); + list_display(titles[T_REQUIRED_BY], requiredby, colstr->nocolor, cols); + list_display(titles[T_OPTIONAL_FOR], optionalfor, colstr->nocolor, cols); } - deplist_display(titles[T_CONFLICTS_WITH], alpm_pkg_get_conflicts(pkg), cols); - deplist_display(titles[T_REPLACES], alpm_pkg_get_replaces(pkg), cols); + deplist_display(titles[T_CONFLICTS_WITH], alpm_pkg_get_conflicts(pkg), colstr->nocolor, cols); + deplist_display(titles[T_REPLACES], alpm_pkg_get_replaces(pkg), colstr->nocolor, cols);
size = humanize_size(alpm_pkg_get_size(pkg), '\0', 2, &label); if(from == ALPM_PKG_FROM_SYNCDB) { @@ -296,15 +297,15 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) printf("%s%s%s %.2f %s\n", config->colstr.title, titles[T_INSTALLED_SIZE], config->colstr.nocolor, size, label);
- string_display(titles[T_PACKAGER], alpm_pkg_get_packager(pkg), cols); - string_display(titles[T_BUILD_DATE], bdatestr, cols); + string_display(titles[T_PACKAGER], alpm_pkg_get_packager(pkg), colstr->nocolor, cols); + string_display(titles[T_BUILD_DATE], bdatestr, colstr->nocolor, cols); if(from == ALPM_PKG_FROM_LOCALDB) { - string_display(titles[T_INSTALL_DATE], idatestr, cols); - string_display(titles[T_INSTALL_REASON], reason, cols); + string_display(titles[T_INSTALL_DATE], idatestr, colstr->nocolor, cols); + string_display(titles[T_INSTALL_REASON], reason, colstr->nocolor, cols); } if(from == ALPM_PKG_FROM_FILE || from == ALPM_PKG_FROM_LOCALDB) { string_display(titles[T_INSTALL_SCRIPT], - alpm_pkg_has_scriptlet(pkg) ? _("Yes") : _("No"), cols); + alpm_pkg_has_scriptlet(pkg) ? _("Yes") : _("No"), colstr->nocolor, cols); }
if(from == ALPM_PKG_FROM_SYNCDB && extra) { @@ -321,25 +322,25 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) keys = alpm_list_add(keys, _("None")); }
- string_display(titles[T_MD5_SUM], alpm_pkg_get_md5sum(pkg), cols); - string_display(titles[T_SHA_256_SUM], alpm_pkg_get_sha256sum(pkg), cols); - list_display(titles[T_SIGNATURES], keys, cols); + string_display(titles[T_MD5_SUM], alpm_pkg_get_md5sum(pkg), colstr->nocolor, cols); + string_display(titles[T_SHA_256_SUM], alpm_pkg_get_sha256sum(pkg), colstr->nocolor, cols); + list_display(titles[T_SIGNATURES], keys, colstr->nocolor, cols);
if(base64_sig) { FREELIST(keys); } } else { - list_display(titles[T_VALIDATED_BY], validation, cols); + list_display(titles[T_VALIDATED_BY], validation, colstr->nocolor, cols); }
if(from == ALPM_PKG_FROM_FILE) { alpm_siglist_t siglist; int err = alpm_pkg_check_pgp_signature(pkg, &siglist); if(err && alpm_errno(config->handle) == ALPM_ERR_SIG_MISSING) { - string_display(titles[T_SIGNATURES], _("None"), cols); + string_display(titles[T_SIGNATURES], _("None"), colstr->nocolor, cols); } else if(err) { string_display(titles[T_SIGNATURES], - alpm_strerror(alpm_errno(config->handle)), cols); + alpm_strerror(alpm_errno(config->handle)), colstr->err, cols); } else { signature_display(titles[T_SIGNATURES], &siglist, cols); } diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index be52d1b..e42e9fa 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -1237,7 +1237,8 @@ int main(int argc, char *argv[]) printf("Lock File : %s\n", alpm_option_get_lockfile(config->handle)); printf("Log File : %s\n", alpm_option_get_logfile(config->handle)); printf("GPG Dir : %s\n", alpm_option_get_gpgdir(config->handle)); - list_display("Targets :", pm_targets, 0); + const colstr_t *colstr = &config->colstr; + list_display("Targets :", pm_targets, colstr->nocolor, 0); }
/* Log command line */ diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 041b6b2..f2ca125 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -557,9 +557,10 @@ static int process_group(alpm_list_t *dbs, const char *group, int error) if(config->print == 0) { char *array = malloc(count); int n = 0; - colon_printf(_n("There is %d member in group %s:\n", - "There are %d members in group %s:\n", count), - count, group); + const colstr_t *colstr = &config->colstr; + colon_printf(_n("There is %d member in group %s%s%s:\n", + "There are %d members in group %s%s%s:\n", count), + count, colstr->groups, group, colstr->title); select_display(pkgs); if(!array) { ret = 1; diff --git a/src/pacman/util.c b/src/pacman/util.c index 0862de0..0c0703f 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -473,7 +473,7 @@ static void add_transaction_sizes_row(alpm_list_t **rows, char *label, off_t siz *rows = alpm_list_add(*rows, row); }
-void string_display(const char *title, const char *string, unsigned short cols) +void string_display(const char *title, const char *string, const char *colour, unsigned short cols) { if(title) { printf("%s%s%s ", config->colstr.title, title, config->colstr.nocolor); @@ -483,7 +483,10 @@ void string_display(const char *title, const char *string, unsigned short cols) } else { /* compute the length of title + a space */ size_t len = string_length(title) + 1; - indentprint(string, (unsigned short)len, cols); + char *clrd_str = NULL; + pm_asprintf(&clrd_str, "%s%s", colour, string); + indentprint(clrd_str, (unsigned short)len, cols); + free(clrd_str); } printf("\n"); } @@ -654,7 +657,7 @@ cleanup: }
void list_display(const char *title, const alpm_list_t *list, - unsigned short maxcols) + const char *colour, unsigned short maxcols) { const alpm_list_t *i; size_t len = 0; @@ -669,7 +672,9 @@ void list_display(const char *title, const alpm_list_t *list, } else { size_t cols = len; const char *str = list->data; - fputs(str, stdout); + char *clrd_str = NULL; + pm_asprintf(&clrd_str, "%s%s", colour, str); + fputs(clrd_str, stdout); cols += string_length(str); for(i = alpm_list_next(list); i; i = alpm_list_next(i)) { str = i->data; @@ -687,10 +692,12 @@ void list_display(const char *title, const alpm_list_t *list, printf(" "); cols += 2; } - fputs(str, stdout); + pm_asprintf(&clrd_str, "%s%s", colour, str); + fputs(clrd_str, stdout); cols += s; } putchar('\n'); + free(clrd_str); } }
@@ -914,18 +921,19 @@ static void _display_targets(alpm_list_t *targets, int verbose) }
/* print to screen */ - pm_asprintf(&str, "%s (%zu)", _("Packages"), alpm_list_count(targets)); + pm_asprintf(&str, "%s (%zu):", _("Packages"), alpm_list_count(targets)); printf("\n");
cols = getcols(); + const colstr_t *colstr = &config->colstr; if(verbose) { header = create_verbose_header(alpm_list_count(targets)); if(table_display(header, rows, cols) != 0) { /* fallback to list display if table wouldn't fit */ - list_display(str, names, cols); + list_display(str, names, colstr->nocolor, cols); } } else { - list_display(str, names, cols); + list_display(str, names, colstr->nocolor, cols); } printf("\n");
@@ -1256,8 +1264,9 @@ static void display_repo_list(const char *dbname, alpm_list_t *list, { const char *prefix = " ";
- colon_printf(_("Repository %s\n"), dbname); - list_display(prefix, list, cols); + const colstr_t *colstr = &config->colstr; + colon_printf(_("Repository %s%s\n"), colstr->repo, dbname); + list_display(prefix, list, colstr->nocolor, cols); }
void select_display(const alpm_list_t *pkglist) diff --git a/src/pacman/util.h b/src/pacman/util.h index f5e37c8..86aa204 100644 --- a/src/pacman/util.h +++ b/src/pacman/util.h @@ -55,10 +55,10 @@ void columns_cache_reset(void); int rmrf(const char *path); void indentprint(const char *str, unsigned short indent, unsigned short cols); char *strreplace(const char *str, const char *needle, const char *replace); -void string_display(const char *title, const char *string, unsigned short cols); +void string_display(const char *title, const char *string, const char *colour, unsigned short cols); double humanize_size(off_t bytes, const char target_unit, int precision, const char **label); -void list_display(const char *title, const alpm_list_t *list, +void list_display(const char *title, const alpm_list_t *list, const char *colour, unsigned short maxcols); void list_display_linebreak(const char *title, const alpm_list_t *list, unsigned short maxcols);
On 25/03/16 11:16, xavion.0@gmail.com wrote:
- pm_asprintf(&str, "%s (%zu)", _("Packages"), alpm_list_count(targets));
- pm_asprintf(&str, "%s (%zu):", _("Packages"), alpm_list_count(targets));
The first thing I look at is an unrelated change...
I asked for small self contained patches that change only one area at a time. This makes things a lot easier to review and ensures unrelated changes such as the able do not appear. I will not look at this further until that is provided.
A
Hi Allan,
- pm_asprintf(&str, "%s (%zu)", _("Packages"),
alpm_list_count(targets));
pm_asprintf(&str, "%s (%zu):", _("Packages"),alpm_list_count(targets));
The first thing I look at is an unrelated change...
How was that the first thing you looked at? It's the seventh change in the fifth file of the patch. Do your eyes have inbuilt magnetisation or something?
It's not really unrelated either, as you had told me not to colour this heading yellow (like in "pacman-color"). When I was reverting the code, I noticed that the trailing colon was missing. Are you seriously saying that you want said colon in a patch all by itself?
I asked for small self contained patches that change only one area at a time. This makes things a lot easier to review and ensures unrelated changes such as the able do not appear. I will not look at this further until that is provided.
You're a hard taskmaster, Chief. I'm not really sure of what you're wanting. Here's what you requested from me:
So, I'd like separate patches:
-Si/-Qi: just those changes -S group dialog: just those changes
-Qo/-Fo can have the same done and anywhere else that is currently not consistent.
Due to the line breaks, I took this to mean that you wanted two patches in total. The first was to contain all of the changes to -Si, -Qi and -S (group), while the second should deal with -Qo/-Fo and the rest.
I'm now guessing that you want -Si and -Qi in a patch by themselves, and -S (group) in a separate one. Is this correct, or do you want -Si and -Qi to be in separate patches as well?
On 03/27/16 at 11:46am, Xavion wrote:
Hi Allan,
pm_asprintf(&str, "%s (%zu)", _("Packages"),alpm_list_count(targets));
pm_asprintf(&str, "%s (%zu):", _("Packages"),alpm_list_count(targets));
The first thing I look at is an unrelated change...
How was that the first thing you looked at? It's the seventh change in the fifth file of the patch. Do your eyes have inbuilt magnetisation or something?
It's not really unrelated either, as you had told me not to colour this heading yellow (like in "pacman-color"). When I was reverting the code, I noticed that the trailing colon was missing. Are you seriously saying that you want said colon in a patch all by itself?
Yes. Adding a colon to pacman's output is not part of "better colourisation support".
I asked for small self contained patches that change only one area at a time. This makes things a lot easier to review and ensures unrelated changes such as the able do not appear. I will not look at this further until that is provided.
You're a hard taskmaster, Chief. I'm not really sure of what you're wanting. Here's what you requested from me:
So, I'd like separate patches:
-Si/-Qi: just those changes -S group dialog: just those changes
-Qo/-Fo can have the same done and anywhere else that is currently not consistent.
Due to the line breaks, I took this to mean that you wanted two patches in total. The first was to contain all of the changes to -Si, -Qi and -S (group), while the second should deal with -Qo/-Fo and the rest.
I'm now guessing that you want -Si and -Qi in a patch by themselves, and -S (group) in a separate one. Is this correct, or do you want -Si and -Qi to be in separate patches as well?
Yes, -Si and -Qi should be together, the other two changes should be done individually.
apg
On 03/25/16 at 12:16pm, xavion.0@gmail.com wrote:
From: Xavion <Xavion (dot) 0 (at) Gmail (dot) com>
Signed-off-by: Xavion <Xavion (dot) 0 (at) Gmail (dot) com>
Please make the following changes before you resubmit: - begin your commit message(s) with a *brief* one-line summary; see http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html - move variable declarations to the top of their scope as described in HACKING - avoid allocating a string just to immediately print it with fputs, just use fprintf
apg
src/pacman/callback.c | 3 ++- src/pacman/package.c | 55 ++++++++++++++++++++++++++------------------------- src/pacman/pacman.c | 3 ++- src/pacman/sync.c | 7 ++++--- src/pacman/util.c | 29 +++++++++++++++++---------- src/pacman/util.h | 4 ++-- 6 files changed, 57 insertions(+), 44 deletions(-)
diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 1e1a4cd..f7c69e2 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -436,7 +436,8 @@ 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());
const colstr_t *colstr = &config->colstr;list_display(" ", namelist, colstr->nocolor, getcols()); printf("\n"); q->skip = noyes(_n( "Do you want to skip the above package for this upgrade?",diff --git a/src/pacman/package.c b/src/pacman/package.c index 3ab9abc..fab1a5c 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){ alpm_list_t *i, *text = NULL; for(i = deps; i; i = alpm_list_next(i)) { alpm_depend_t *dep = i->data; text = alpm_list_add(text, alpm_dep_compute_string(dep)); }
- list_display(title, text, cols);
- list_display(title, text, colour, cols); FREELIST(text);
}
@@ -256,29 +256,30 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) }
cols = getcols();
const colstr_t *colstr = &config->colstr;
/* actual output */ if(from == ALPM_PKG_FROM_SYNCDB) { string_display(titles[T_REPOSITORY],
alpm_db_get_name(alpm_pkg_get_db(pkg)), cols);
alpm_db_get_name(alpm_pkg_get_db(pkg)), colstr->repo, cols);
- string_display(titles[T_NAME], alpm_pkg_get_name(pkg), cols);
- string_display(titles[T_VERSION], alpm_pkg_get_version(pkg), cols);
- string_display(titles[T_DESCRIPTION], alpm_pkg_get_desc(pkg), cols);
- string_display(titles[T_ARCHITECTURE], alpm_pkg_get_arch(pkg), cols);
- string_display(titles[T_URL], alpm_pkg_get_url(pkg), cols);
- list_display(titles[T_LICENSES], alpm_pkg_get_licenses(pkg), cols);
- list_display(titles[T_GROUPS], alpm_pkg_get_groups(pkg), cols);
- deplist_display(titles[T_PROVIDES], alpm_pkg_get_provides(pkg), cols);
- deplist_display(titles[T_DEPENDS_ON], alpm_pkg_get_depends(pkg), cols);
string_display(titles[T_NAME], alpm_pkg_get_name(pkg), colstr->title, cols);
string_display(titles[T_VERSION], alpm_pkg_get_version(pkg), colstr->version, cols);
string_display(titles[T_DESCRIPTION], alpm_pkg_get_desc(pkg), colstr->nocolor, cols);
string_display(titles[T_ARCHITECTURE], alpm_pkg_get_arch(pkg), colstr->nocolor, cols);
string_display(titles[T_URL], alpm_pkg_get_url(pkg), colstr->nocolor, cols);
list_display(titles[T_LICENSES], alpm_pkg_get_licenses(pkg), colstr->nocolor, cols);
list_display(titles[T_GROUPS], alpm_pkg_get_groups(pkg), colstr->groups, cols);
deplist_display(titles[T_PROVIDES], alpm_pkg_get_provides(pkg), colstr->nocolor, cols);
deplist_display(titles[T_DEPENDS_ON], alpm_pkg_get_depends(pkg), colstr->nocolor, cols); optdeplist_display(pkg, cols);
if(extra || from == ALPM_PKG_FROM_LOCALDB) {
list_display(titles[T_REQUIRED_BY], requiredby, cols);list_display(titles[T_OPTIONAL_FOR], optionalfor, cols);
list_display(titles[T_REQUIRED_BY], requiredby, colstr->nocolor, cols);list_display(titles[T_OPTIONAL_FOR], optionalfor, colstr->nocolor, cols);
- deplist_display(titles[T_CONFLICTS_WITH], alpm_pkg_get_conflicts(pkg), cols);
- deplist_display(titles[T_REPLACES], alpm_pkg_get_replaces(pkg), cols);
deplist_display(titles[T_CONFLICTS_WITH], alpm_pkg_get_conflicts(pkg), colstr->nocolor, cols);
deplist_display(titles[T_REPLACES], alpm_pkg_get_replaces(pkg), colstr->nocolor, cols);
size = humanize_size(alpm_pkg_get_size(pkg), '\0', 2, &label); if(from == ALPM_PKG_FROM_SYNCDB) {
@@ -296,15 +297,15 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) printf("%s%s%s %.2f %s\n", config->colstr.title, titles[T_INSTALLED_SIZE], config->colstr.nocolor, size, label);
- string_display(titles[T_PACKAGER], alpm_pkg_get_packager(pkg), cols);
- string_display(titles[T_BUILD_DATE], bdatestr, cols);
- string_display(titles[T_PACKAGER], alpm_pkg_get_packager(pkg), colstr->nocolor, cols);
- string_display(titles[T_BUILD_DATE], bdatestr, colstr->nocolor, cols); if(from == ALPM_PKG_FROM_LOCALDB) {
string_display(titles[T_INSTALL_DATE], idatestr, cols);string_display(titles[T_INSTALL_REASON], reason, cols);
string_display(titles[T_INSTALL_DATE], idatestr, colstr->nocolor, cols);string_display(titles[T_INSTALL_REASON], reason, colstr->nocolor, cols);
alpm_pkg_has_scriptlet(pkg) ? _("Yes") : _("No"), cols);
alpm_pkg_has_scriptlet(pkg) ? _("Yes") : _("No"), colstr->nocolor, cols);}
if(from == ALPM_PKG_FROM_SYNCDB && extra) {
@@ -321,25 +322,25 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) keys = alpm_list_add(keys, _("None")); }
string_display(titles[T_MD5_SUM], alpm_pkg_get_md5sum(pkg), cols);string_display(titles[T_SHA_256_SUM], alpm_pkg_get_sha256sum(pkg), cols);list_display(titles[T_SIGNATURES], keys, cols);
string_display(titles[T_MD5_SUM], alpm_pkg_get_md5sum(pkg), colstr->nocolor, cols);string_display(titles[T_SHA_256_SUM], alpm_pkg_get_sha256sum(pkg), colstr->nocolor, cols);list_display(titles[T_SIGNATURES], keys, colstr->nocolor, cols);if(base64_sig) { FREELIST(keys); } } else {
list_display(titles[T_VALIDATED_BY], validation, cols);
list_display(titles[T_VALIDATED_BY], validation, colstr->nocolor, cols);}
if(from == ALPM_PKG_FROM_FILE) { alpm_siglist_t siglist; int err = alpm_pkg_check_pgp_signature(pkg, &siglist); if(err && alpm_errno(config->handle) == ALPM_ERR_SIG_MISSING) {
string_display(titles[T_SIGNATURES], _("None"), cols);
string_display(titles[T_SIGNATURES], _("None"), colstr->nocolor, cols);
alpm_strerror(alpm_errno(config->handle)), cols);
alpm_strerror(alpm_errno(config->handle)), colstr->err, cols);diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index be52d1b..e42e9fa 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -1237,7 +1237,8 @@ int main(int argc, char *argv[]) printf("Lock File : %s\n", alpm_option_get_lockfile(config->handle)); printf("Log File : %s\n", alpm_option_get_logfile(config->handle)); printf("GPG Dir : %s\n", alpm_option_get_gpgdir(config->handle));
list_display("Targets :", pm_targets, 0);
const colstr_t *colstr = &config->colstr;list_display("Targets :", pm_targets, colstr->nocolor, 0);}
/* Log command line */
diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 041b6b2..f2ca125 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -557,9 +557,10 @@ static int process_group(alpm_list_t *dbs, const char *group, int error) if(config->print == 0) { char *array = malloc(count); int n = 0;
colon_printf(_n("There is %d member in group %s:\n","There are %d members in group %s:\n", count),count, group);
const colstr_t *colstr = &config->colstr;colon_printf(_n("There is %d member in group %s%s%s:\n","There are %d members in group %s%s%s:\n", count),count, colstr->groups, group, colstr->title);diff --git a/src/pacman/util.c b/src/pacman/util.c index 0862de0..0c0703f 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -473,7 +473,7 @@ static void add_transaction_sizes_row(alpm_list_t **rows, char *label, off_t siz *rows = alpm_list_add(*rows, row); }
-void string_display(const char *title, const char *string, unsigned short cols) +void string_display(const char *title, const char *string, const char *colour, unsigned short cols) { if(title) { printf("%s%s%s ", config->colstr.title, title, config->colstr.nocolor); @@ -483,7 +483,10 @@ void string_display(const char *title, const char *string, unsigned short cols) } else { /* compute the length of title + a space */ size_t len = string_length(title) + 1;
indentprint(string, (unsigned short)len, cols);
char *clrd_str = NULL;pm_asprintf(&clrd_str, "%s%s", colour, string);indentprint(clrd_str, (unsigned short)len, cols);free(clrd_str);} @@ -654,7 +657,7 @@ cleanup: }
void list_display(const char *title, const alpm_list_t *list,
unsigned short maxcols)
const char *colour, unsigned short maxcols){ const alpm_list_t *i; size_t len = 0; @@ -669,7 +672,9 @@ void list_display(const char *title, const alpm_list_t *list, } else { size_t cols = len; const char *str = list->data;
fputs(str, stdout);
char *clrd_str = NULL;pm_asprintf(&clrd_str, "%s%s", colour, str);fputs(clrd_str, stdout);@@ -687,10 +692,12 @@ void list_display(const char *title, const alpm_list_t *list, printf(" "); cols += 2; }
fputs(str, stdout);
pm_asprintf(&clrd_str, "%s%s", colour, str);fputs(clrd_str, stdout); cols += s;free(clrd_str);}
@@ -914,18 +921,19 @@ static void _display_targets(alpm_list_t *targets, int verbose) }
/* print to screen */
- pm_asprintf(&str, "%s (%zu)", _("Packages"), alpm_list_count(targets));
pm_asprintf(&str, "%s (%zu):", _("Packages"), alpm_list_count(targets)); printf("\n");
cols = getcols();
const colstr_t *colstr = &config->colstr; if(verbose) { header = create_verbose_header(alpm_list_count(targets)); if(table_display(header, rows, cols) != 0) { /* fallback to list display if table wouldn't fit */
list_display(str, names, cols);
list_display(str, names, colstr->nocolor, cols);
list_display(str, names, cols);
list_display(str, names, colstr->nocolor, cols);@@ -1256,8 +1264,9 @@ static void display_repo_list(const char *dbname, alpm_list_t *list, { const char *prefix = " ";
- colon_printf(_("Repository %s\n"), dbname);
- list_display(prefix, list, cols);
- const colstr_t *colstr = &config->colstr;
- colon_printf(_("Repository %s%s\n"), colstr->repo, dbname);
- list_display(prefix, list, colstr->nocolor, cols);
}
void select_display(const alpm_list_t *pkglist) diff --git a/src/pacman/util.h b/src/pacman/util.h index f5e37c8..86aa204 100644 --- a/src/pacman/util.h +++ b/src/pacman/util.h @@ -55,10 +55,10 @@ void columns_cache_reset(void); int rmrf(const char *path); void indentprint(const char *str, unsigned short indent, unsigned short cols); char *strreplace(const char *str, const char *needle, const char *replace); -void string_display(const char *title, const char *string, unsigned short cols); +void string_display(const char *title, const char *string, const char *colour, unsigned short cols); double humanize_size(off_t bytes, const char target_unit, int precision, const char **label); -void list_display(const char *title, const alpm_list_t *list, +void list_display(const char *title, const alpm_list_t *list, const char *colour, unsigned short maxcols); void list_display_linebreak(const char *title, const alpm_list_t *list, unsigned short maxcols); -- 2.7.3
From: Xavion <Xavion (dot) 0 (at) Gmail (dot) com>
Signed-off-by: Xavion <Xavion (dot) 0 (at) Gmail (dot) com> --- src/pacman/util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/pacman/util.c b/src/pacman/util.c index 0862de0..07ae6ee 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -914,7 +914,7 @@ static void _display_targets(alpm_list_t *targets, int verbose) }
/* print to screen */ - pm_asprintf(&str, "%s (%zu)", _("Packages"), alpm_list_count(targets)); + pm_asprintf(&str, "%s (%zu):", _("Packages"), alpm_list_count(targets)); printf("\n");
cols = getcols();
From: Xavion <Xavion (dot) 0 (at) Gmail (dot) com>
Specifically, the group is in 'blue' and the repository is in 'magenta'.
Signed-off-by: Xavion <Xavion (dot) 0 (at) Gmail (dot) com> --- src/pacman/sync.c | 7 ++++--- src/pacman/util.c | 3 ++- 2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 041b6b2..f2ca125 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -557,9 +557,10 @@ static int process_group(alpm_list_t *dbs, const char *group, int error) if(config->print == 0) { char *array = malloc(count); int n = 0; - colon_printf(_n("There is %d member in group %s:\n", - "There are %d members in group %s:\n", count), - count, group); + const colstr_t *colstr = &config->colstr; + colon_printf(_n("There is %d member in group %s%s%s:\n", + "There are %d members in group %s%s%s:\n", count), + count, colstr->groups, group, colstr->title); select_display(pkgs); if(!array) { ret = 1; diff --git a/src/pacman/util.c b/src/pacman/util.c index 0862de0..cc63e2b 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -1255,8 +1255,9 @@ static void display_repo_list(const char *dbname, alpm_list_t *list, unsigned short cols) { const char *prefix = " "; + const colstr_t *colstr = &config->colstr;
- colon_printf(_("Repository %s\n"), dbname); + colon_printf(_("Repository %s%s\n"), colstr->repo, dbname); list_display(prefix, list, cols); }
On 28/03/16 08:35, xavion.0@gmail.com wrote:
From: Xavion <Xavion (dot) 0 (at) Gmail (dot) com>
Specifically, the group is in 'blue' and the repository is in 'magenta'.
Signed-off-by: Xavion <Xavion (dot) 0 (at) Gmail (dot) com>
OK.
I changed the commit message:
Add colour to group selection dialog
Colour the group name in 'blue' and the repository names in 'magenta'.
From: Xavion <Xavion (dot) 0 (at) Gmail (dot) com>
Specifically, the following changes have been made: * The repository is in 'magenta' * The package name is in 'bold' * The version is in 'green' * The group is in 'blue'
Signed-off-by: Xavion <Xavion (dot) 0 (at) Gmail (dot) com> --- src/pacman/callback.c | 3 ++- src/pacman/package.c | 55 ++++++++++++++++++++++++++------------------------- src/pacman/pacman.c | 3 ++- src/pacman/util.c | 21 ++++++++++++-------- src/pacman/util.h | 4 ++-- 5 files changed, 47 insertions(+), 39 deletions(-)
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()); printf("\n"); q->skip = noyes(_n( "Do you want to skip the above package for this upgrade?", 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) { alpm_list_t *i, *text = NULL; for(i = deps; i; i = alpm_list_next(i)) { alpm_depend_t *dep = i->data; text = alpm_list_add(text, alpm_dep_compute_string(dep)); } - list_display(title, text, cols); + list_display(title, text, colour, cols); FREELIST(text); }
@@ -198,6 +198,7 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) char bdatestr[50] = "", idatestr[50] = ""; const char *label, *reason; alpm_list_t *validation = NULL, *requiredby = NULL, *optionalfor = NULL; + const colstr_t *colstr = &config->colstr;
/* make aligned titles once only */ static int need_alignment = 1; @@ -260,25 +261,25 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) /* actual output */ if(from == ALPM_PKG_FROM_SYNCDB) { string_display(titles[T_REPOSITORY], - alpm_db_get_name(alpm_pkg_get_db(pkg)), cols); + alpm_db_get_name(alpm_pkg_get_db(pkg)), colstr->repo, cols); } - string_display(titles[T_NAME], alpm_pkg_get_name(pkg), cols); - string_display(titles[T_VERSION], alpm_pkg_get_version(pkg), cols); - string_display(titles[T_DESCRIPTION], alpm_pkg_get_desc(pkg), cols); - string_display(titles[T_ARCHITECTURE], alpm_pkg_get_arch(pkg), cols); - string_display(titles[T_URL], alpm_pkg_get_url(pkg), cols); - list_display(titles[T_LICENSES], alpm_pkg_get_licenses(pkg), cols); - list_display(titles[T_GROUPS], alpm_pkg_get_groups(pkg), cols); - deplist_display(titles[T_PROVIDES], alpm_pkg_get_provides(pkg), cols); - deplist_display(titles[T_DEPENDS_ON], alpm_pkg_get_depends(pkg), cols); + string_display(titles[T_NAME], alpm_pkg_get_name(pkg), colstr->title, cols); + string_display(titles[T_VERSION], alpm_pkg_get_version(pkg), colstr->version, cols); + string_display(titles[T_DESCRIPTION], alpm_pkg_get_desc(pkg), colstr->nocolor, cols); + string_display(titles[T_ARCHITECTURE], alpm_pkg_get_arch(pkg), colstr->nocolor, cols); + string_display(titles[T_URL], alpm_pkg_get_url(pkg), colstr->nocolor, cols); + list_display(titles[T_LICENSES], alpm_pkg_get_licenses(pkg), colstr->nocolor, cols); + list_display(titles[T_GROUPS], alpm_pkg_get_groups(pkg), colstr->groups, cols); + deplist_display(titles[T_PROVIDES], alpm_pkg_get_provides(pkg), colstr->nocolor, cols); + deplist_display(titles[T_DEPENDS_ON], alpm_pkg_get_depends(pkg), colstr->nocolor, cols); optdeplist_display(pkg, cols);
if(extra || from == ALPM_PKG_FROM_LOCALDB) { - list_display(titles[T_REQUIRED_BY], requiredby, cols); - list_display(titles[T_OPTIONAL_FOR], optionalfor, cols); + list_display(titles[T_REQUIRED_BY], requiredby, colstr->nocolor, cols); + list_display(titles[T_OPTIONAL_FOR], optionalfor, colstr->nocolor, cols); } - deplist_display(titles[T_CONFLICTS_WITH], alpm_pkg_get_conflicts(pkg), cols); - deplist_display(titles[T_REPLACES], alpm_pkg_get_replaces(pkg), cols); + deplist_display(titles[T_CONFLICTS_WITH], alpm_pkg_get_conflicts(pkg), colstr->nocolor, cols); + deplist_display(titles[T_REPLACES], alpm_pkg_get_replaces(pkg), colstr->nocolor, cols);
size = humanize_size(alpm_pkg_get_size(pkg), '\0', 2, &label); if(from == ALPM_PKG_FROM_SYNCDB) { @@ -296,15 +297,15 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) printf("%s%s%s %.2f %s\n", config->colstr.title, titles[T_INSTALLED_SIZE], config->colstr.nocolor, size, label);
- string_display(titles[T_PACKAGER], alpm_pkg_get_packager(pkg), cols); - string_display(titles[T_BUILD_DATE], bdatestr, cols); + string_display(titles[T_PACKAGER], alpm_pkg_get_packager(pkg), colstr->nocolor, cols); + string_display(titles[T_BUILD_DATE], bdatestr, colstr->nocolor, cols); if(from == ALPM_PKG_FROM_LOCALDB) { - string_display(titles[T_INSTALL_DATE], idatestr, cols); - string_display(titles[T_INSTALL_REASON], reason, cols); + string_display(titles[T_INSTALL_DATE], idatestr, colstr->nocolor, cols); + string_display(titles[T_INSTALL_REASON], reason, colstr->nocolor, cols); } if(from == ALPM_PKG_FROM_FILE || from == ALPM_PKG_FROM_LOCALDB) { string_display(titles[T_INSTALL_SCRIPT], - alpm_pkg_has_scriptlet(pkg) ? _("Yes") : _("No"), cols); + alpm_pkg_has_scriptlet(pkg) ? _("Yes") : _("No"), colstr->nocolor, cols); }
if(from == ALPM_PKG_FROM_SYNCDB && extra) { @@ -321,25 +322,25 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra) keys = alpm_list_add(keys, _("None")); }
- string_display(titles[T_MD5_SUM], alpm_pkg_get_md5sum(pkg), cols); - string_display(titles[T_SHA_256_SUM], alpm_pkg_get_sha256sum(pkg), cols); - list_display(titles[T_SIGNATURES], keys, cols); + string_display(titles[T_MD5_SUM], alpm_pkg_get_md5sum(pkg), colstr->nocolor, cols); + string_display(titles[T_SHA_256_SUM], alpm_pkg_get_sha256sum(pkg), colstr->nocolor, cols); + list_display(titles[T_SIGNATURES], keys, colstr->nocolor, cols);
if(base64_sig) { FREELIST(keys); } } else { - list_display(titles[T_VALIDATED_BY], validation, cols); + list_display(titles[T_VALIDATED_BY], validation, colstr->nocolor, cols); }
if(from == ALPM_PKG_FROM_FILE) { alpm_siglist_t siglist; int err = alpm_pkg_check_pgp_signature(pkg, &siglist); if(err && alpm_errno(config->handle) == ALPM_ERR_SIG_MISSING) { - string_display(titles[T_SIGNATURES], _("None"), cols); + string_display(titles[T_SIGNATURES], _("None"), colstr->nocolor, cols); } else if(err) { string_display(titles[T_SIGNATURES], - alpm_strerror(alpm_errno(config->handle)), cols); + alpm_strerror(alpm_errno(config->handle)), colstr->err, cols); } else { signature_display(titles[T_SIGNATURES], &siglist, cols); } diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index be52d1b..b2d2735 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -1221,6 +1221,7 @@ int main(int argc, char *argv[])
if(config->verbose > 0) { alpm_list_t *j; + const colstr_t *colstr = &config->colstr; printf("Root : %s\n", alpm_option_get_root(config->handle)); printf("Conf File : %s\n", config->configfile); printf("DB Path : %s\n", alpm_option_get_dbpath(config->handle)); @@ -1237,7 +1238,7 @@ int main(int argc, char *argv[]) printf("Lock File : %s\n", alpm_option_get_lockfile(config->handle)); printf("Log File : %s\n", alpm_option_get_logfile(config->handle)); printf("GPG Dir : %s\n", alpm_option_get_gpgdir(config->handle)); - list_display("Targets :", pm_targets, 0); + list_display("Targets :", pm_targets, colstr->nocolor, 0); }
/* Log command line */ diff --git a/src/pacman/util.c b/src/pacman/util.c index 0862de0..b4e8dff 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -473,7 +473,7 @@ static void add_transaction_sizes_row(alpm_list_t **rows, char *label, off_t siz *rows = alpm_list_add(*rows, row); }
-void string_display(const char *title, const char *string, unsigned short cols) +void string_display(const char *title, const char *string, const char *colour, unsigned short cols) { if(title) { printf("%s%s%s ", config->colstr.title, title, config->colstr.nocolor); @@ -483,7 +483,10 @@ void string_display(const char *title, const char *string, unsigned short cols) } else { /* compute the length of title + a space */ size_t len = string_length(title) + 1; - indentprint(string, (unsigned short)len, cols); + char *clrd_str = NULL; + pm_asprintf(&clrd_str, "%s%s", colour, string); + indentprint(clrd_str, (unsigned short)len, cols); + free(clrd_str); } printf("\n"); } @@ -654,7 +657,7 @@ cleanup: }
void list_display(const char *title, const alpm_list_t *list, - unsigned short maxcols) + const char *colour, unsigned short maxcols) { const alpm_list_t *i; size_t len = 0; @@ -669,7 +672,7 @@ void list_display(const char *title, const alpm_list_t *list, } else { size_t cols = len; const char *str = list->data; - fputs(str, stdout); + fprintf(stdout, "%s%s", colour, str); cols += string_length(str); for(i = alpm_list_next(list); i; i = alpm_list_next(i)) { str = i->data; @@ -687,7 +690,7 @@ void list_display(const char *title, const alpm_list_t *list, printf(" "); cols += 2; } - fputs(str, stdout); + fprintf(stdout, "%s%s", colour, str); cols += s; } putchar('\n'); @@ -873,6 +876,7 @@ static void _display_targets(alpm_list_t *targets, int verbose) off_t isize = 0, rsize = 0, dlsize = 0; unsigned short cols; alpm_list_t *i, *names = NULL, *header = NULL, *rows = NULL; + const colstr_t *colstr = &config->colstr;
if(!targets) { return; @@ -922,10 +926,10 @@ static void _display_targets(alpm_list_t *targets, int verbose) header = create_verbose_header(alpm_list_count(targets)); if(table_display(header, rows, cols) != 0) { /* fallback to list display if table wouldn't fit */ - list_display(str, names, cols); + list_display(str, names, colstr->nocolor, cols); } } else { - list_display(str, names, cols); + list_display(str, names, colstr->nocolor, cols); } printf("\n");
@@ -1255,9 +1259,10 @@ static void display_repo_list(const char *dbname, alpm_list_t *list, unsigned short cols) { const char *prefix = " "; + const colstr_t *colstr = &config->colstr;
colon_printf(_("Repository %s\n"), dbname); - list_display(prefix, list, cols); + list_display(prefix, list, colstr->nocolor, cols); }
void select_display(const alpm_list_t *pkglist) diff --git a/src/pacman/util.h b/src/pacman/util.h index f5e37c8..86aa204 100644 --- a/src/pacman/util.h +++ b/src/pacman/util.h @@ -55,10 +55,10 @@ void columns_cache_reset(void); int rmrf(const char *path); void indentprint(const char *str, unsigned short indent, unsigned short cols); char *strreplace(const char *str, const char *needle, const char *replace); -void string_display(const char *title, const char *string, unsigned short cols); +void string_display(const char *title, const char *string, const char *colour, unsigned short cols); double humanize_size(off_t bytes, const char target_unit, int precision, const char **label); -void list_display(const char *title, const alpm_list_t *list, +void list_display(const char *title, const alpm_list_t *list, const char *colour, unsigned short maxcols); void list_display_linebreak(const char *title, const alpm_list_t *list, unsigned short maxcols);
On 28/03/16 08:35, xavion.0@gmail.com wrote:
From: Xavion <Xavion (dot) 0 (at) Gmail (dot) com>
Specifically, the following changes have been made:
- The repository is in 'magenta'
- The package name is in 'bold'
- The version is in 'green'
- The group is in 'blue'
Signed-off-by: Xavion <Xavion (dot) 0 (at) Gmail (dot) com>
src/pacman/callback.c | 3 ++- src/pacman/package.c | 55 ++++++++++++++++++++++++++------------------------- src/pacman/pacman.c | 3 ++- src/pacman/util.c | 21 ++++++++++++-------- src/pacman/util.h | 4 ++-- 5 files changed, 47 insertions(+), 39 deletions(-)
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...
printf("\n"); q->skip = noyes(_n( "Do you want to skip the above package for this upgrade?",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
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.
On 18/04/16 16:05, Allan McRae wrote:
On 28/03/16 08:35, xavion.0@gmail.com wrote:
From: Xavion <Xavion (dot) 0 (at) Gmail (dot) com>
Specifically, the following changes have been made:
- The repository is in 'magenta'
- The package name is in 'bold'
- The version is in 'green'
- The group is in 'blue'
Signed-off-by: Xavion <Xavion (dot) 0 (at) Gmail (dot) com>
src/pacman/callback.c | 3 ++- src/pacman/package.c | 55 ++++++++++++++++++++++++++------------------------- src/pacman/pacman.c | 3 ++- src/pacman/util.c | 21 ++++++++++++-------- src/pacman/util.h | 4 ++-- 5 files changed, 47 insertions(+), 39 deletions(-)
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...
printf("\n"); q->skip = noyes(_n( "Do you want to skip the above package for this upgrade?",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
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.
To follow up this patch. I have pulled the changes for the group selection dialog and the -Qo/-Fo output.
It is unlikely that I will ever accept the -S/Qi colour patches as it is too intrusive - at least in its current format.
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.
Allan
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.
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.
I'm sitting here hoping for less colour abuse. It would be nice if all the colour pacman had was for warnings and errors to distinguish them more clearly from the rest of a text along with bright on prompts. The fact -Qi/-Si has colouring at all bothers me.
The colour in -Ss (bright) also bothers me but so does the layout (thankfully expac exists).
Point is, please don't include me in your secret users.
Hi Earnestly,
I'm sitting here hoping for less colour abuse. It would be nice if all
the colour pacman had was for warnings and errors to distinguish them more clearly from the rest of a text along with bright on prompts. The fact -Qi/-Si has colouring at all bothers me.
The colour in -Ss (bright) also bothers me but so does the layout (thankfully expac exists).
To keep people like you happy, maybe the 'Color' variable in "/etc/pacman.conf" should be ternary instead of binary (as follows):
- Color = 0 (for no colour at all) - Color = 1 (for coloured warnings and errors only) - Color = 2 (for full colourisation)
Point is, please don't include me in your secret users.
I wasn't planning to, Chief. That's why I wrote "many" (as opposed to "all") users. Nonetheless, thanks for your opinion; I can't fathom why anyone would want it like that, but thanks anyway :-).
On 23/04/16 10:07, Xavion wrote:
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.
I think there was a mix up there - those two patches have been pulled to my local git branch to be applied at some point.
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.
The -S/Qi patch is too intrusive for a feature that that I am not enthused about including (I believe Andrew has reservations about this patch too). It adds very little to the readability of the output.
I included the group dialog colour patch - I am not sold on it, but there is some gain in breaking the bold text by highlighting the group names, particularly if multiple groups are being installed. The repo names are also mildly helpful being highlighted.
The -F/Qo patch was accepted on the basis of using similar colours to that already in the -S/Q/Fs output.
I am still questioning the advantage of colour the repo/version/groups in all the -s and -o outputs though. I see the advantage in highlighting the package name and the installed state. The rest I need to decide on.
Allan
I think there was a mix up there - those two patches have been pulled to my local git branch to be applied at some point.
Yes, I keep forgetting to think of 'pull' in Git terms. When someone talks of pulling the changes to something, it makes me think they've decided to discard them.
The -S/Qi patch is too intrusive for a feature that that I am not
enthused about including (I believe Andrew has reservations about this patch too). It adds very little to the readability of the output.
I can't see how adding an extra parameter to a few functions is "too intrusive". Similarly, I think four extra colourings (repository, name, version, and groups) adds plenty to the readability of the output.
I am still questioning the advantage of colour the repo/version/groups
in all the -s and -o outputs though. I see the advantage in highlighting the package name and the installed state. The rest I need to decide on.
Please don't remove any of the colourisation that's already present. If there's one thing I hate in life, it's when people decide to devolve something back to the dark ages (excuse the pun).
On 23/04/16 15:39, Xavion wrote:
I am still questioning the advantage of colour the repo/version/groups in all the -s and -o outputs though. I see the advantage in highlighting the package name and the installed state. The rest I need to decide on.
Please don't remove any of the colourisation that's already present. If there's one thing I hate in life, it's when people decide to devolve something back to the dark ages (excuse the pun).
When running "pacman -Ss pacman", what is the advantage to every colour apart from the bold package name and the installed highlight?
Lets start with the version. The version information is not important and always directly beside the highlighted package name. Are you ever looking for the version when you run a search? What functionality is there to justify the version being highlighted?
If you can justify the need for the version to be green in the -s or -o output, I'd really like to hear it.
Allan
When running "pacman -Ss pacman", what is the advantage to every colour apart from the bold package name and the installed highlight?
As I've said from the start: "The main purpose of having the extra colours is to make it easier for the brain to pick out the important bits".
Lets start with the version. The version information is not important
and always directly beside the highlighted package name. Are you ever looking for the version when you run a search? What functionality is there to justify the version being highlighted?
If you can justify the need for the version to be green in the -s or -o output, I'd really like to hear it.
You might not be interested in the version when running a search, but I am (probably 20% of the time). Due to my above quotation, I want it to remain green-coloured.
For the same reason, I want proper colourisation in the -Si and -Qi operations as well. I don't think Andrew had issues with my patch(es) after I made the changes he suggested.
Similarly, I don't know why anyone wouldn't want the green to remain in -Xs and -Xo. It's not like the extra colours make extracting the relevant information more difficult for the brain.
On 24/04/16 14:01, Xavion wrote:
When running "pacman -Ss pacman", what is the advantage to every colour apart from the bold package name and the installed highlight?
As I've said from the start: "The main purpose of having the extra colours is to make it easier for the brain to pick out the important bits".
Lets start with the version. The version information is not important
and always directly beside the highlighted package name. Are you ever looking for the version when you run a search? What functionality is there to justify the version being highlighted?
If you can justify the need for the version to be green in the -s or -o output, I'd really like to hear it.
You might not be interested in the version when running a search, but I am (probably 20% of the time). Due to my above quotation, I want it to remain green-coloured.
Your justification is really no justification! "I am" is not a justification. Explain why you are interested in the version when you run a -s or -o. What question are you answering that you don't first locate by looking at the package name?
Allan
Your justification is really no justification! "I am" is not a justification. Explain why you are interested in the version when you run a -s or -o. What question are you answering that you don't first locate by looking at the package name?
It's mostly when I'm wondering if the currently installed package is the latest version, but it also happens when I'm looking for alternative offerings of an already installed package.
For example, I usually roll with the "dropbox" package from [archlinuxfr]; however, I sometimes like to check whether the AUR's offering is more up-to-date (which it currently is).
Hence, I run "yaourt -Ss dropbox", which uses the output from 'pacman' for the repository entries. Is that the justification you were looking for? If so, can we get my "-{Q,S}i" patch included without further ado?
On 24/04/16 15:57, Xavion wrote:
Your justification is really no justification! "I am" is not a justification. Explain why you are interested in the version when you run a -s or -o. What question are you answering that you don't first locate by looking at the package name?
It's mostly when I'm wondering if the currently installed package is the latest version, but it also happens when I'm looking for alternative offerings of an already installed package.
For example, I usually roll with the "dropbox" package from [archlinuxfr]; however, I sometimes like to check whether the AUR's offering is more up-to-date (which it currently is).
Hence, I run "yaourt -Ss dropbox", which uses the output from 'pacman' for the repository entries. Is that the justification you were looking for?
So you do a "-Ss dropbox" and among the half dozen packages, you look for the version number and not the pkgname to locate the entry?
If so, can we get my "-{Q,S}i" patch included without further ado?
That is not happening. At the moment you are justifying me not removing the colour of the version in -s and -o output.
Allan
So you do a "-Ss dropbox" and among the half dozen packages, you look for the version number and not the pkgname to locate the entry?
Not usually in that case, because I know the exact name of the package I want (dropbox). However, if I'm uncertain of the package name, I'll often scan the list by the version number. Hence, keep it green!
That is not happening. At the moment you are justifying me not removing
the colour of the version in -s and -o output.
"Yet" ... that is not happening *yet*. Whether you like it or not, my "-{Q,S}i" patch is one of the best things since sliced bread. I just have to find a way to add more colour to your life (so that you can appreciate it).
On 04/24/16 at 06:20pm, Xavion wrote:
So you do a "-Ss dropbox" and among the half dozen packages, you look for the version number and not the pkgname to locate the entry?
Not usually in that case, because I know the exact name of the package I want (dropbox). However, if I'm uncertain of the package name, I'll often scan the list by the version number. Hence, keep it green!
That is not happening. At the moment you are justifying me not removing
the colour of the version in -s and -o output.
"Yet" ... that is not happening *yet*. Whether you like it or not, my "-{Q,S}i" patch is one of the best things since sliced bread. I just have to find a way to add more colour to your life (so that you can appreciate it).
You seem to think that more color is so obviously better than less color that you don't need to justify its inclusion. It's not. If you really want all this color in pacman's output you need to start giving us reasons beyond just "I want it" or "I find it more readable". I believe Allan and I are in total agreement here: color is most effective when used sparingly to emphasize particular important information and we have no interest in tailoring pacman to a single person's subjective taste. We have already heard from one user who wants *less* color; you need to give us a reason why your desire for more color makes more sense than their desire for less.
I still don't understand why you would scan -[QS]s output for version numbers. You said that you do this when you want to know if your installed version is the latest, but surely you need to locate the correct package by name or description first. Or, in the case of -Ss output, you might scan for the "[installed]" tag.
apg
On Sun, Apr 24, 2016 at 2:25 PM, Andrew Gregory andrew.gregory.8@gmail.com wrote:
You seem to think that more color is so obviously better than less color that you don't need to justify its inclusion. It's not. If you really want all this color in pacman's output you need to start giving us reasons beyond just "I want it" or "I find it more readable". I believe Allan and I are in total agreement here: color is most
Urm, to just add my two cents here, but I'm assuming xavion has grown used to the former pacman-color. As someone who is regularly annoyed the fuck out of by things that change outside of my control and as I'm not really over the no longer so recent coreutils ls(1) change, seriously. We're geeks and we know where we want moving parts, and sometimes we just need something that stays the same even across project boundaries and/or merges, if I'm reading this correctly. If this justification doesn't count, take this as a call decision, because I don't think there's any party with any other more relevant argument.
cheers! mar77i
Urm, to just add my two cents here, but I'm assuming xavion has grown used to the former pacman-color. As someone who is regularly annoyed the fuck out of by things that change outside of my control and as I'm not really over the no longer so recent coreutils ls(1) change, seriously. We're geeks and we know where we want moving parts, and sometimes we just need something that stays the same even across project boundaries and/or merges, if I'm reading this correctly. If this justification doesn't count, take this as a call decision, because I don't think there's any party with any other more relevant argument.
Cool, I think someone has finally spoken up in my defence (assuming I read that correctly). Consistency is the name of the game here, and the current "-{Q,S}i" output falls well short of the mark in that regard.
You seem to think that more color is so obviously better than less color that you don't need to justify its inclusion. It's not. If you really want all this color in pacman's output you need to start giving us reasons beyond just "I want it" or "I find it more readable". I believe Allan and I are in total agreement here: color is most effective when used sparingly to emphasize particular important information and we have no interest in tailoring pacman to a single person's subjective taste. We have already heard from one user who wants *less* color; you need to give us a reason why your desire for more color makes more sense than their desire for less.
The only reason I should need to give is that I want to make the colouring of "-{Q,S}i" consistent with the rest of the 'pacman' output. This means magenta for repository, bold for name, green for version, and blue for groups.
Nonetheless, I have provided other justifications as well (even if you and Allan somehow missed them). Maybe you don't have time to go looking for them now, but that doesn't necessarily mean that they're nonexistent.
I still don't understand why you would scan -[QS]s output for version
numbers. You said that you do this when you want to know if your installed version is the latest, but surely you need to locate the correct package by name or description first. Or, in the case of -Ss output, you might scan for the "[installed]" tag.
Actually, I didn't say that; you should go back and read what I wrote again. You're getting the two examples I provided around the wrong way.
I really don't see what all the fuss is about here. It seems that some people are trying desperately to oppose my opinions for the sake of it. I'm just trying to make things consistent; it's as simple as that.
From: Xavion <Xavion (dot) 0 (at) Gmail (dot) com>
Specifically, the following changes have been made: * The repository is in 'magenta' * The package name is in 'bold' * The version is in 'green'
Signed-off-by: Xavion <Xavion (dot) 0 (at) Gmail (dot) com> --- src/pacman/files.c | 6 ++++-- src/pacman/query.c | 5 +++-- 2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/pacman/files.c b/src/pacman/files.c index 5240c07..692fcd5 100644 --- a/src/pacman/files.c +++ b/src/pacman/files.c @@ -77,8 +77,10 @@ static int files_fileowner(alpm_list_t *syncs, alpm_list_t *targets) { if(config->op_f_machinereadable) { print_line_machinereadable(repo, pkg, filename); } else if(!config->quiet) { - printf(_("%s is owned by %s/%s %s\n"), filename, - alpm_db_get_name(repo), alpm_pkg_get_name(pkg), + const colstr_t *colstr = &config->colstr; + printf(_("%s is owned by %s%s/%s%s %s%s\n"), filename, + colstr->repo, alpm_db_get_name(repo), colstr->title, + alpm_pkg_get_name(pkg), colstr->version, alpm_pkg_get_version(pkg)); } else { printf("%s/%s\n", alpm_db_get_name(repo), alpm_pkg_get_name(pkg)); diff --git a/src/pacman/query.c b/src/pacman/query.c index 0cc12e6..496eefe 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -83,8 +83,9 @@ static int search_path(char **filename, struct stat *bufptr) static void print_query_fileowner(const char *filename, alpm_pkg_t *info) { if(!config->quiet) { - printf(_("%s is owned by %s %s\n"), filename, - alpm_pkg_get_name(info), alpm_pkg_get_version(info)); + const colstr_t *colstr = &config->colstr; + printf(_("%s is owned by %s%s %s%s\n"), filename, colstr->title, + alpm_pkg_get_name(info), colstr->version, alpm_pkg_get_version(info)); } else { printf("%s\n", alpm_pkg_get_name(info)); }
On 28/03/16 10:54, xavion.0@gmail.com wrote:
From: Xavion <Xavion (dot) 0 (at) Gmail (dot) com>
Specifically, the following changes have been made:
- The repository is in 'magenta'
- The package name is in 'bold'
- The version is in 'green'
Signed-off-by: Xavion <Xavion (dot) 0 (at) Gmail (dot) com>
src/pacman/files.c | 6 ++++-- src/pacman/query.c | 5 +++-- 2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/pacman/files.c b/src/pacman/files.c index 5240c07..692fcd5 100644 --- a/src/pacman/files.c +++ b/src/pacman/files.c @@ -77,8 +77,10 @@ static int files_fileowner(alpm_list_t *syncs, alpm_list_t *targets) { if(config->op_f_machinereadable) { print_line_machinereadable(repo, pkg, filename); } else if(!config->quiet) {
printf(_("%s is owned by %s/%s %s\n"), filename,alpm_db_get_name(repo), alpm_pkg_get_name(pkg),
const colstr_t *colstr = &config->colstr;printf(_("%s is owned by %s%s/%s%s %s%s\n"), filename,colstr->repo, alpm_db_get_name(repo), colstr->title,alpm_pkg_get_name(pkg), colstr->version, alpm_pkg_get_version(pkg));
Should the '/' between the repo and package names be magenta?
} else { printf("%s/%s\n", alpm_db_get_name(repo), alpm_pkg_get_name(pkg));diff --git a/src/pacman/query.c b/src/pacman/query.c index 0cc12e6..496eefe 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -83,8 +83,9 @@ static int search_path(char **filename, struct stat *bufptr) static void print_query_fileowner(const char *filename, alpm_pkg_t *info) { if(!config->quiet) {
printf(_("%s is owned by %s %s\n"), filename,alpm_pkg_get_name(info), alpm_pkg_get_version(info));
const colstr_t *colstr = &config->colstr;printf(_("%s is owned by %s%s %s%s\n"), filename, colstr->title,alpm_pkg_get_name(info), colstr->version, alpm_pkg_get_version(info));
On 18/04/16 16:08, Allan McRae wrote:
On 28/03/16 10:54, xavion.0@gmail.com wrote:
From: Xavion <Xavion (dot) 0 (at) Gmail (dot) com>
Specifically, the following changes have been made:
- The repository is in 'magenta'
- The package name is in 'bold'
- The version is in 'green'
Signed-off-by: Xavion <Xavion (dot) 0 (at) Gmail (dot) com>
src/pacman/files.c | 6 ++++-- src/pacman/query.c | 5 +++-- 2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/pacman/files.c b/src/pacman/files.c index 5240c07..692fcd5 100644 --- a/src/pacman/files.c +++ b/src/pacman/files.c @@ -77,8 +77,10 @@ static int files_fileowner(alpm_list_t *syncs, alpm_list_t *targets) { if(config->op_f_machinereadable) { print_line_machinereadable(repo, pkg, filename); } else if(!config->quiet) {
printf(_("%s is owned by %s/%s %s\n"), filename,alpm_db_get_name(repo), alpm_pkg_get_name(pkg),
const colstr_t *colstr = &config->colstr;printf(_("%s is owned by %s%s/%s%s %s%s\n"), filename,colstr->repo, alpm_db_get_name(repo), colstr->title,alpm_pkg_get_name(pkg), colstr->version, alpm_pkg_get_version(pkg));Should the '/' between the repo and package names be magenta?
Replying to myself... it is for -Ss and -Qs.
A
Should the '/' between the repo and package names be magenta?
Replying to myself... it is for -Ss and -Qs.
Yes, I deliberately made it like -Ss and -Qs in this regard. I don't particularly like it this way, but at least it's consistent.
Hi Allan,
Yes. Adding a colon to pacman's output is not part of "better
colourisation support".
Yes, -Si and -Qi should be together, the other two changes should be done individually.
Please make the following changes before you resubmit:
- begin your commit message(s) with a *brief* one-line summary; see http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
- move variable declarations to the top of their scope as described in HACKING
- avoid allocating a string just to immediately print it with fputs, just use fprintf
You might have noticed that I incorporated all of Andrew's change requests and released four separate patches on March 28. I haven't heard anything back from either of you since then.
Do I take that as an acceptance of my revised patches, or is it just a sign that no-one has looked at them yet? If the former, does this mean that they're in the merge queue for the trunk?
On 09/04/16 11:01, Xavion wrote:
Hi Allan,
Yes. Adding a colon to pacman's output is not part of "better
colourisation support".
Yes, -Si and -Qi should be together, the other two changes should be done individually.
Please make the following changes before you resubmit:
- begin your commit message(s) with a *brief* one-line summary; see http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
- move variable declarations to the top of their scope as described in HACKING
- avoid allocating a string just to immediately print it with fputs, just use fprintf
You might have noticed that I incorporated all of Andrew's change requests and released four separate patches on March 28. I haven't heard anything back from either of you since then.
Do I take that as an acceptance of my revised patches, or is it just a sign that no-one has looked at them yet? If the former, does this mean that they're in the merge queue for the trunk?
I have not had tome to sit down and do much patch review lately. They are in the queue.
A
- -Dk, -Dkk: the package name (but not the dependency) should be in
bold, rather than quoted
- -Qc: the package name should be in bold, rather than quoted
I realised after sending my last email that removing the quotes would harm
monochrome 'pacman' users. I find it hard to remember that we have to factor in the poor souls who can't (yet) afford a colour monitor.
On 25/03/16 11:06, Xavion wrote:
Hi Allan,
I have become old and grumpy since then!
Okay, point taken: I did let it drag on a bit. It wasn't all my fault, though: I got sick of waiting for those other three patches to be merged in.
Lets start with what is already justified by what we already have in pacman with the aim of improving consistency of our current colour scheme. Further additions will need to be discussed separately.
package names - bold (in some places...) groups - blue repos - magenta versions - green
So, I'd like separate patches:
-Si/-Qi: just those changes -S group dialog: just those changes
Righto, you're the boss. I've just created a patch for those switches alone; I will post it to this thread soon. I'll get back to you about the other colours at a later date.
-Qo/-Fo can have the same done and anywhere else that is currently not consistent.
As far as I can tell, the following switches need better colourisation. Let me know if you disagree with any of this *before* I go ahead and create the patch :-).
- -Dk, -Dkk: the package name (but not the dependency) should be in
bold, rather than quoted
Colour adds nothing here
- -Qc: the package name should be in bold, rather than quoted
or here...
- -Qg, -Sg: the group name should be in blue; the package name should be
in bold
or here... You don't need colour to distinguish between left and right columns.
- -Qk, -Qkk: the package name should be in bold
Only errors/warnings are highlighted. Adding more colour hides the important information
- -Qo, -Fo: the package name should be in bold; the version number
should be in green
I said this was fine above.
- -Qq, -Fq, -Sq: the package name should (possibly) be in bold
Not sure what package name you are referring to.
- -T: the package name should (possibly) be in bold
No colour here ever.
Hi Allan,
-Qg, -Sg: the group name should be in blue; the package name should be in
bold
or here... You don't need colour to distinguish between left and right columns.
It's more for the sake of consistency. The -Q{e,m,n} command highlights the left (of two) columns, as does -Ql (although the second column is much bigger in this case). Are you sure that you don't want -{Q,S}g to follow suit?
-Qo, -Fo: the package name should be in bold; the version number should be
in green
I said this was fine above.
Yep, I was just listing it for the sake of completeness. As you can probably tell by now, I like to be comprehensive about these things :-).
-Qq, -Fq, -Sq: the package name should (possibly) be in bold
Not sure what package name you are referring to.
If I run -Qq or -Slq for example, a single column of package names is output. Depending on your response to my first paragraph, you might want this to be colourised as well.
Xavion xavion.0@gmail.com writes:
To make things easier for you, I've just added screenshots showing the current 'pacman' (colourised) output to the album http://imgur.com/a/DL4ky. The main purpose of having the extra colours is to make it easier for the brain to pick out the important bits (as noted earlier in this thread).
Hey, late to the party, and I get the sense this patch set isn't going to fly, so I hope I'm not adding salt to the wound.
I was the guy who originally got the colour support pushed into pacman. The pacman-color patch was my starting point, so I thought I should explain why I deviated and brought in less colours.
A lot of discussion happened again back then too, trying to decide what should and shouldn't be coloured, if anyone wants to dive through the mailing list archive.
What bothered me about the pacman-color patch was how half-hazard the -Si/-Qi colouring is. The important bits (to me, of course) aren't colourized. The repository and URL are not very important. Dependency information, on the other hand, is. 90% of the time I search the pacman database, what I care about is the description and the dependencies - both demphasized with their implemented colouring scheme.
By attempting to colourizing individual fields IMHO we'll either end up with a soup of colours that ends up more distracting then informational, or we make the wrong choice, emphasizing the wrong stuff and making the output harder to read. The current way is at least a nice balance that doesn't fall into either pitfalls.
The pacman -Sl output, these days, is too colourful in my opinion and I kinda regret it. I do wish it less bright. Something closer to only the package name and maybe the "[installed]" bit should have colour.
Hey, late to the party, and I get the sense this patch set isn't going to fly, so I hope I'm not adding salt to the wound.
You're not really late to the party: you might recall contributing to this thread on June 6, 2013.
What bothered me about the pacman-color patch was how half-hazard the
-Si/-Qi colouring is. The important bits (to me, of course) aren't colourized. The repository and URL are not very important. Dependency information, on the other hand, is. 90% of the time I search the pacman database, what I care about is the description and the dependencies - both demphasized with their implemented colouring scheme.
Maybe the description and dependencies can be given other colours once my patch is included.
By attempting to colourizing individual fields IMHO we'll either end up with a soup of colours that ends up more distracting then informational, or we make the wrong choice, emphasizing the wrong stuff and making the output harder to read. The current way is at least a nice balance that doesn't fall into either pitfalls.
In order to be consistent, the repository, name, version, and groups should all be coloured.
The pacman -Sl output, these days, is too colourful in my opinion and I
kinda regret it. I do wish it less bright. Something closer to only the package name and maybe the "[installed]" bit should have colour.
I think it looks good; the colours are so different from each other that there's no confusion.
What bothered me about the pacman-color patch was how half-hazard the -Si/-Qi colouring is. The important bits (to me, of course) aren't colourized. The repository and URL are not very important. Dependency information, on the other hand, is. 90% of the time I search the pacman database, what I care about is the description and the dependencies - both demphasized with their implemented colouring scheme.
Maybe the description and dependencies can be given other colours once my patch is included.
You only have a pallet of 8 colours you can reasonably work with and they're almost all allocated. The full table has around 30 unique rows, which show depends on various conditions. Which are important enough to allocate a colour too?
By attempting to colourizing individual fields IMHO we'll either end up with a soup of colours that ends up more distracting then informational, or we make the wrong choice, emphasizing the wrong stuff and making the output harder to read. The current way is at least a nice balance that doesn't fall into either pitfalls.
In order to be consistent, the repository, name, version, and groups should all be coloured.
I don't think this is a particularly strong argument, output already isn't very consistent: sometimes we output tables (-i), other times information is much more condensed and inlined (-s). This would be a different story if the table showed "core/linux 4.5.1-1" instead of separate Repo, Name and Version rows.
As much as I had stated regret for putting as much colour in the condensed view as I did, it at least works because it makes the dense information more distinct. I don't think the argument is anywhere near as strong for the tabled output. Colourization here will have the effect of drawing the eye to specific fields.
Just consider the shear amount of disagreement in this thread over workflows and which fields are important to who. Maybe we shouldn't be trying to mark certain fields as important and let the information be on equal footing. I appreciate your efforts to try and make pacman more consistent, but this is a value call.
I vote for sticking to keeping things neutral.
You only have a pallet of 8 colours you can reasonably work with and they're almost all allocated. The full table has around 30 unique rows, which show depends on various conditions. Which are important enough to allocate a colour too?
I don't know, you're the one telling the story. I just want the "-{Q,S}i" colours to be consistent with the rest of 'pacman'. Including my patch is the best (known) way to achieve this.
I don't think this is a particularly strong argument, output already isn't very consistent: sometimes we output tables (-i), other times information is much more condensed and inlined (-s). This would be a different story if the table showed "core/linux 4.5.1-1" instead of separate Repo, Name and Version rows.
I don't agree with that. I think the core colours (repository, name, version, and groups) should be consistent across all commands/switches. To not have this makes 'pacman' look amateurish in my opinion.
As much as I had stated regret for putting as much colour in the condensed view as I did, it at least works because it makes the dense information more distinct. I don't think the argument is anywhere near as strong for the tabled output. Colourization here will have the effect of drawing the eye to specific fields.
That's what we want, as far as I'm concerned. Also, having the colours makes it quicker to find the uncoloured fields as well. For example, it won't take long for our brains to learn/remember that 'licenses' is right above (the blue-coloured) 'groups'.
Just consider the shear amount of disagreement in this thread over workflows and which fields are important to who. Maybe we shouldn't be trying to mark certain fields as important and let the information be on equal footing. I appreciate your efforts to try and make pacman more consistent, but this is a value call.
I vote for sticking to keeping things neutral.
The worst thing we can possibly do is leave "-{Q,S}i" in its current (uncoloured) state. People who don't want to see coloured output should disable the 'Color' flag in "/etc/pacman.conf".
On 27/04/16 15:45, Xavion wrote:
You only have a pallet of 8 colours you can reasonably work with and they're almost all allocated. The full table has around 30 unique rows, which show depends on various conditions. Which are important enough to allocate a colour too?
I don't know, you're the one telling the story. I just want the "-{Q,S}i" colours to be consistent with the rest of 'pacman'. Including my patch is the best (known) way to achieve this.
Removing lots of unnecessary colour in -s/-o operations is better.
And this conversation is done - the patch for -Si colouring has been rejected by the two lead developers. There is no-one left to convince to include it.
Allan
Removing lots of unnecessary colour in -s/-o operations is better.
Aren't we supposed to provide justifications for our arguments in here?
And this conversation is done - the patch for -Si colouring has been rejected by the two lead developers. There is no-one left to convince to include it.
I think one of them is agreeing with the other out of fear more than anything else :-).
Colour should be used to highlight the important information. How do you decide the URL is more important than the dependencies for -Si? Why is the number of packages being installed highlighted yellow when that is only used for warnings?
Agreed in general, but it makes sense to reuse the same colors for the same classes of information across different commands. The package names, repositories, groups and versions have color in -Ss/-Qs so it makes sense to reuse those same colors in the -Si output and perhaps elsewhere like the -Su output.
It makes it easier to visually jump to the right place in the output if there are some consistently colored lines. It doesn't all need color.
I don't use Pacman enough that I'd benefit from any of this though... only upgrades, occasionally package installation and rarely a search or query command.
pacman-dev@lists.archlinux.org
