FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > ArchLinux > ArchLinux Pacman Development

 
 
LinkBack Thread Tools
 
Old 08-05-2012, 12:13 PM
Allan McRae
 
Default Add color commands for stuff in util

On 05/08/12 19:46, Daniel Wallace wrote:
> Signed-off-by: Daniel Wallace <daniel.wallace@gatech.edu>
> ---
> src/pacman/color.c | 728 ++++++++++++++++++++++++++++++++++++++++++++++++++ +++
> src/pacman/color.h | 70 ++++++
> src/pacman/util.c | 35 ++-
> src/pacman/util.h | 6 +
> 4 files changed, 830 insertions(+), 9 deletions(-)
> create mode 100644 src/pacman/color.c
> create mode 100644 src/pacman/color.h


BIG patch... so I am going to provide general comments.

Firstly, header for color.c is wrong, color.h is missing.


And here is the big one. There is a LOT of code "duplication". I say
"duplication" because some of the functions have diverged in the time
this patch has been around so they are no longer duplicates. That
should be an obvious maintenance burden we want to avoid.

An example:

color_yesno() vs. yesno():
- the only difference is the call to color_question() rather question()

then

color_question() vs. question():
- calls color_vfprintf() rather than vfprintf()


So, lets reduce this a lot.

I see that color_vfprintf falls back to just a standard vfprintf when
the "colors" variable is NULL. So we can just call this whenever this
is needed and pass NULL... in fact... see comment on the test below.



> +int color_vfprintf(FILE* stream, const colordata_t* colors, const
char* format, va_list args)

I'd call this vfprintf_color(). Well... I'd call it vfprintf_colour!
Anyway, I think teh vfprintf is the more important part so should be
read first.

> +{
> + int ret = 0;
> +
> + if(isatty(fileno(stream)) && colors) {

We could do:
if(config->color && colors && isatty(fileno(stream)))
then this could be a drop in replacement for vfprintf.

> + char* msg = NULL;
> + ret = vasprintf(&msg, format, args);
> + if(msg == NULL) {
> + return(ret);
> + }
> +
> + const colordata_t* colorpos = colors;

Huh?

> + color_t colorlast = COLOR_NONE;
> + int len = strlen(msg) + 1;
> + wchar_t* wcstr = calloc(len, sizeof(wchar_t));
> + len = mbstowcs(wcstr, msg, len);
> + free(msg);
> + const wchar_t *strpos = wcstr;
> +
> + while(*strpos) {
> + if(colorpos->color != COLOR_END &&
> + ((colorpos->separator == SEP_ANY) ||
> + (colorpos->separator == SEP_LINE && *strpos == L'
') ||
> + (colorpos->separator == SEP_COLON && (*strpos == L':' || *strpos
== L':')))) {

What is that second character being tested?

> + _insert_color(stream, colorpos->color);
> + colorlast = colorpos->color;
> + colorpos++;
> + }
> + fprintf(stream, "%lc", (wint_t)*strpos);
> + strpos++;
> + }
> + free(wcstr);
> +
> + if(colorlast != COLOR_NONE) {
> + _insert_color(stream, COLOR_NONE);
> + }
> + } else {
> + ret = vfprintf(stream, format, args);
> + }
> + return(ret);
> +}


If similar things were done so that printf_color() etc all just fell
through to the non-colour version, the whole patchset could just be
printf -> printf_color changes.

Allan
 

Thread Tools




All times are GMT. The time now is 01:25 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org