On Wed, 08 Apr 2009, Junio C Hamano wrote:
> Allan Caffee <allan.caffee@xxxxxxxxx> writes:
>
> > @@ -72,11 +69,21 @@ struct column {
> > */
> > struct commit *commit;
> > /*
> > - * XXX: Once we add support for colors, struct column could also
> > - * contain the color of its branch line.
> > + * The color to (optionally) print this column in.
> > */
> > + const char *color;
>
> You already use short for git_graph.default_column_color and I suspect in
> the longer term you want to make this one an index into column_colors[]
> array the same way. We may someday decide to support non-ANSI color
> scheme using ncurses or something, and at that point the "hardware color"
> constants like GIT_COLOR_RED and friends may change from strings to small
> integers we use to call our (yet to be written) curses interface layer
> with.
The problem with making it an index into the column_colors array is that
we don't have a convenient place to test whether the user actually wants
color. We can't do it in strbuf_write_column because AFAIK there's no
way to get the rev-info to test the options. I suppose we could define
GIT_NOT_A_COLOR to -1 and just set the color to that when we don't
intend to use color. (Either way I should probably change that to an
unsigned short.) What do you think?
> > @@ -312,6 +343,33 @@ static struct commit_list *first_interesting_parent(struct git_graph *graph)
> > return next_interesting_parent(graph, parents);
> > }
> >
> > +static const char* graph_get_current_column_color(const struct git_graph* graph)
>
> Style. Asterisk comes next to identifiers, not types (the parameter to
> graph_increment_column_color has the same issue).
Okay.
> static const char *graph_...color(const struct git_graph *graph)
>
> > @@ -596,7 +661,7 @@ static void graph_output_padding_line(struct git_graph *graph,
> > * Output a padding row, that leaves all branch lines unchanged
> > */
> > for (i = 0; i < graph->num_new_columns; i++) {
> > - strbuf_addstr(sb, "| ");
> > + strbuf_write_column(sb, &graph->new_columns[i], "| ");
>
> Hmmm, this forbids us to use reverse color in the color palette because
> that would highlight the trailing whitespace. Is that something we care
> about, or a reversed "|", "/" and "\" are already too ugly that we won't
> want to support them?
Personally I don't think it's a good idea to limit the colors just
because they would "look funny". It seems to me that if the user wanted
the background colored they would expect to see just the line segment
colored and not the whitespace.
The simplest way to fix this AFAIKS is to change strbuf_write_column to
take a single character and change the existing code to add spaces in
seperately.
/* For example */
strbuf_write_column(sb, &graph->new_columns[i], "| ");
/* Becomes */
strbuf_write_column(sb, &graph->new_columns[i], '|');
strbuf_addch(sb, ' ');
This would fix the problem at the minor expense of adding ~15 lines of
code.
> [...]
>
> > @@ -744,14 +813,25 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
> > if (graph->num_parents < 3)
> > strbuf_addch(sb, ' ');
> > else {
> > + /*
> > + * Here dashless_commits represents the
> > + * number of parents which don't need
> > + * to have dashes (because their edges
> > + * fit neatly under the commit).
> > + */
> > + const int dashless_commits = 2;
> > int num_dashes =
> > - ((graph->num_parents - 2) * 2) - 1;
> > + ((graph->num_parents - dashless_commits) * 2) - 1;
> > for (j = 0; j < num_dashes; j++)
> > - strbuf_addch(sb, '-');
> > - strbuf_addstr(sb, ". ");
> > + strbuf_write_column(sb,
> > + &graph->new_columns[(j / 2) + dashless_commits],
> > + "-");
> > + strbuf_write_column(sb,
> > + &graph->new_columns[(j / 2) + dashless_commits],
> > + ". ");
>
> The nesting seems to be becoming too deep and the body of the for loop is
> getting too long. Time to make it a helper function that handles only one
> column, perhaps?
If I understand this correctly in order to write a per column function
would be like:
/*
* Draw one peice of the commit line and return the new value of
* seen_this.
*/
static int graph_commit_line_draw_column(struct git_graph *graph,
struct strbuf *sb,
int i, int seen_this)
Which moves the entire body of the for loop into a function and only
reduces the indentation by one level. Am I missing something?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html