Re: [PATCH 3/2] pretty: support right alignment
On Sun, Sep 23, 2012 at 01:17:43AM -0700, Junio C Hamano wrote: > Nguyen Thai Ngoc Duy writes: > > > ... On the other hand, I don't > > really wish to turn pretty format machinery into a full feature text > > layout engine (by ripping of links/lynx?). > > That is very true. We should restrain ourselves and avoid going > overboard piling shiny new toys on a not-so-useful foundation that > is not expressive enough. One feature that is probably much more > needed on the foundation side would be some form of conditional > output, without which built-in output elements like --show-notes > cannot be emulated with --format option. The embedded lua patch series I just posted may interest (or horrify) the both of you: http://article.gmane.org/gmane.comp.version-control.git/206335 -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/2] pretty: support right alignment
Nguyen Thai Ngoc Duy writes: > ... On the other hand, I don't > really wish to turn pretty format machinery into a full feature text > layout engine (by ripping of links/lynx?). That is very true. We should restrain ourselves and avoid going overboard piling shiny new toys on a not-so-useful foundation that is not expressive enough. One feature that is probably much more needed on the foundation side would be some form of conditional output, without which built-in output elements like --show-notes cannot be emulated with --format option. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/2] pretty: support right alignment
Nguyen Thai Ngoc Duy writes: >> - How does this interact with the wrapped output? Should it? > > We have to deal with it anyway when the left aligned text takes all > the space. On one hand, I don't want to break the terminal width, > leading to ugly output, so it'll interact. On the other hand, I don't > really wish to turn pretty format machinery into a full feature text > layout engine (by ripping of links/lynx?). So we have a few options: > > 1. ellipses, line cutting means i18n issues ahead > 2. just put the right-aligned text on another line. We do something > similar in parse-options. When the option syntax is too long, we put > help description on the next line. > 3. bring in html/css box model for arranging text so that both > left/right aligned texts can share the same line. > 4. tell users upfront it's not supported. don't do that > > I'd vote 2, or 4. I am fine with 4 personally. >> - I am wondering if somebody ever want to do this with a follow-up >>patch: >> >> Left %h%|Center %cd%|Right %ad >> >>Is %| a sensible choice for "flush right"? I am wondering if it >>makes more sense to make %|, %< and %> as "multi-column >>introducer" (the example defines output with three columns) that >>also tells how text inside each column is flushed inside the >>column, e.g. >> >> %>col 1 right flushed%|col 2 centered%< col 3 left flushed >> >>or something like that (we may want explicit "column width" >>specifiers if we were to do this kind of thing). > > Yeah that crossed my mind. But I'll need to convince myself it's > actually useful. Once you're on that road, you may want >=4 column > tables. We can extend column.c to do that. That hard part is > converting pretty format to use column functions. Reading the above again, I realize that I may have sounded as if saying "With 'flush to right', we are inviting wishes for 'left' and 'center', and a patch that only does 'right' is unacceptable.", but that was not what I meant. I am perfectly fine with 'flush to right' without anything else as the first step. The only concern I had was that somebody who later tries to add 'left', 'center', etc. to accompany the 'right' that you are adding with your patch will find it unfortunate that the natural choice for 'center', i.e. %|, has been taken to mean a wrong thing and that mistake cannot be corrected without breaking backward compatibility. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/2] pretty: support right alignment
On Thu, Sep 20, 2012 at 09:40:49AM -0700, Junio C Hamano wrote: > - I am wondering if somebody ever want to do this with a follow-up >patch: > > Left %h%|Center %cd%|Right %ad > >Is %| a sensible choice for "flush right"? I am wondering if it >makes more sense to make %|, %< and %> as "multi-column >introducer" (the example defines output with three columns) that >also tells how text inside each column is flushed inside the >column, e.g. > > %>col 1 right flushed%|col 2 centered%< col 3 left flushed > >or something like that (we may want explicit "column width" >specifiers if we were to do this kind of thing). Instead of thinking of "columns", we could go back to "placeholders", or in printf terms, an "%s". In addition to plain %s, we need something similar to "%*s" and "%-*s" to pad right and left. Conceptually it's simpler. We don't have to deal with a bunch of problems in your quotes that I cut out. Still it allows users to do flush right, flush left and so on within limits. They just need to think in terms of fixed-size cells. So... %>(N)%? is transformed roughly to printf("%-*s", N, %?). Similarly %<(N)%? becomes printf("%*s", N, %?). We could have %|(N) to pad both %left and right (aka centered). Better? We might need a modifier or something to allow cutting (and maybe putting ellipsis in place) to keep oversized cells from breaking the layout. The demonstration patch follows. You can't build because I don't post the whole series. -- 8< -- diff --git a/pretty.c b/pretty.c index b1cec71..543c309 100644 --- a/pretty.c +++ b/pretty.c @@ -617,6 +617,12 @@ struct chunk { size_t len; }; +enum flush_type { + no_flush, + flush_right, + flush_left +}; + struct format_commit_context { const struct commit *commit; const struct pretty_print_context *pretty_ctx; @@ -624,13 +630,14 @@ struct format_commit_context { unsigned commit_message_parsed:1; unsigned commit_signature_parsed:1; unsigned use_color:1; + enum flush_type flush_type; struct { char *gpg_output; char good_bad; char *signer; } signature; char *message; - size_t width, indent1, indent2; + size_t width, indent1, indent2, padding; /* These offsets are relative to the start of the commit message. */ struct chunk author; @@ -944,6 +951,24 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder, return end - placeholder + 1; } else return 0; + + case '<': + case '>': + if (placeholder[1] == '(') { + const char *start = placeholder + 2; + const char *end = strchr(start, ')'); + char *next; + int width; + if (!end || end == start) + return 0; + width = strtoul(start, &next, 10); + if (next == start || width == 0) + return 0; + c->padding = width; + c->flush_type = *placeholder == '<' ? flush_right : flush_left; + return end - placeholder + 1; + } + return 0; } /* these depend on the commit */ @@ -1102,6 +1127,8 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder, static size_t format_commit_item(struct strbuf *sb, const char *placeholder, void *context) { + struct format_commit_context *c = context; + struct strbuf local_sb = STRBUF_INIT; int consumed; size_t orig_len; enum { @@ -1127,10 +1154,23 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder, if (magic != NO_MAGIC) placeholder++; - orig_len = sb->len; - consumed = format_commit_one(sb, placeholder, context); - if (magic == NO_MAGIC) - return consumed; + if (c->flush_type != no_flush) { + int len; + consumed = format_commit_one(&local_sb, placeholder, context); + /* the number of column, esc seq skipped */ + len = utf8_strnwidth(local_sb.buf, -1, 1); + strbuf_addf(sb, + c->flush_type == flush_right ? "%-*s" : "%*s", + (int)(c->padding + (local_sb.len - len)), + local_sb.buf); + strbuf_release(&local_sb); + c->flush_type = no_flush; + } else { + orig_len = sb->len; + consumed = format_commit_one(sb, placeholder, context); + if (magic == NO_MAGIC) + return consumed; + } if ((orig_len == sb->len) &&
Re: [PATCH 3/2] pretty: support right alignment
On Thu, Sep 20, 2012 at 11:40 PM, Junio C Hamano wrote: > I think this is a great feature at the conceptual level, and you > know "but" is coming ;-). I'm still not sure if it's useful beyond my simple example. For example, will it be useful in multiline log format, not just --oneline? > - Shouldn't it be "everything from there until the end of the >current line" than "everything after that"? The patch does that. I wasn't specific in my patch description. > - How is the display width determined and is it fixed once it gets >computed? term_columns(). But I'd rather have a (user-configurable) max limit. It's really hard to line up two distant text parts of a 200 char line without a physical ruler. In my patch I just hard code the max limit around 120 char or so. > - How does this interact with the wrapped output? Should it? We have to deal with it anyway when the left aligned text takes all the space. On one hand, I don't want to break the terminal width, leading to ugly output, so it'll interact. On the other hand, I don't really wish to turn pretty format machinery into a full feature text layout engine (by ripping of links/lynx?). So we have a few options: 1. ellipses, line cutting means i18n issues ahead 2. just put the right-aligned text on another line. We do something similar in parse-options. When the option syntax is too long, we put help description on the next line. 3. bring in html/css box model for arranging text so that both left/right aligned texts can share the same line. 4. tell users upfront it's not supported. don't do that I'd vote 2, or 4. > - I am wondering if somebody ever want to do this with a follow-up >patch: > > Left %h%|Center %cd%|Right %ad > >Is %| a sensible choice for "flush right"? I am wondering if it >makes more sense to make %|, %< and %> as "multi-column >introducer" (the example defines output with three columns) that >also tells how text inside each column is flushed inside the >column, e.g. > > %>col 1 right flushed%|col 2 centered%< col 3 left flushed > >or something like that (we may want explicit "column width" >specifiers if we were to do this kind of thing). Yeah that crossed my mind. But I'll need to convince myself it's actually useful. Once you're on that road, you may want >=4 column tables. We can extend column.c to do that. That hard part is converting pretty format to use column functions. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/2] pretty: support right alignment
Nguyen Thai Ngoc Duy writes: > And this is a for-fun patch that adds %| to right align everything > after that. I'm ignoring problems with line wrapping, i18n and so > on. "%C+%h %s%|%d" looks quite nice. I'm not sure how much useful it > is beyond --oneline though. It looks something like this > ... > [oneline output ellided] > ... I think this is a great feature at the conceptual level, and you know "but" is coming ;-). - Shouldn't it be "everything from there until the end of the current line" than "everything after that"? - How is the display width determined and is it fixed once it gets computed? - How does this interact with the wrapped output? Should it? - I am wondering if somebody ever want to do this with a follow-up patch: Left %h%|Center %cd%|Right %ad Is %| a sensible choice for "flush right"? I am wondering if it makes more sense to make %|, %< and %> as "multi-column introducer" (the example defines output with three columns) that also tells how text inside each column is flushed inside the column, e.g. %>col 1 right flushed%|col 2 centered%< col 3 left flushed or something like that (we may want explicit "column width" specifiers if we were to do this kind of thing). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/2] pretty: support right alignment
And this is a for-fun patch that adds %| to right align everything after that. I'm ignoring problems with line wrapping, i18n and so on. "%C+%h %s%|%d" looks quite nice. I'm not sure how much useful it is beyond --oneline though. It looks something like this cc543b2 pretty: support placeholders %C+ and %C- (HEAD, master) da6001e pretty: share code between format_decoration and show_decorations b0576a6 Update draft release notes to 1.8.0 (origin/master, origin/HEAD) 3d7535e Merge branch 'jc/maint-log-grep-all-match' 06e211a Merge branch 'jc/make-static' 8db3865 Merge branch 'pw/p4-submit-conflicts' 3387423 Merge branch 'mv/cherry-pick-s' d71abd9 Merge branch 'nd/fetch-status-alignment' 3c7d509 Sync with 1.7.12.1 304b7d9 Git 1.7.12.1 (tag: v1.7.12.1, origin/maint) 39e2e02 Merge branch 'er/doc-fast-import-done' into maint 8ffc331 Merge branch 'jk/config-warn-on-inaccessible-paths' into maint 01f7d7f Doc: Improve shallow depth wording 8093ae8 Documentation/git-filter-branch: Move note about effect of removing commits -- 8< -- diff --git a/pretty.c b/pretty.c index b1cec71..6e96f83 100644 --- a/pretty.c +++ b/pretty.c @@ -624,6 +624,7 @@ struct format_commit_context { unsigned commit_message_parsed:1; unsigned commit_signature_parsed:1; unsigned use_color:1; + unsigned right_alignment:1; struct { char *gpg_output; char good_bad; @@ -645,6 +646,8 @@ struct format_commit_context { struct chunk abbrev_tree_hash; struct chunk abbrev_parent_hashes; size_t wrap_start; + + struct strbuf *right_sb; }; static int add_again(struct strbuf *sb, struct chunk *chunk) @@ -944,6 +947,10 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder, return end - placeholder + 1; } else return 0; + + case '|': + c->right_alignment = 1; + return 1; } /* these depend on the commit */ @@ -1099,9 +1106,44 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder, return 0; /* unknown placeholder */ } +static void right_align(struct strbuf *sb, + struct format_commit_context *c, + int flush) +{ + const char *p; + int llen, rlen, len, total = term_columns() - 1; + if (!c->right_alignment) + return; + p = strchr(c->right_sb->buf, '\n'); + if (!p && flush) + p = c->right_sb->buf + c->right_sb->len; + if (!p) + return; + + c->right_alignment = 0; + len = p - c->right_sb->buf; + if (!len) + return; + if (total > 110) + total = 110; + rlen = utf8_strnwidth(c->right_sb->buf, len); + p = strrchr(sb->buf, '\n'); + if (!p) + p = sb->buf; + else + p++; + llen = utf8_strwidth(p); + strbuf_addf(sb, "%*s", + total - llen + (len - rlen), + c->right_sb->buf); + strbuf_reset(c->right_sb); +} + static size_t format_commit_item(struct strbuf *sb, const char *placeholder, void *context) { + struct format_commit_context *c = context; + struct strbuf *real_sb; int consumed; size_t orig_len; enum { @@ -1127,10 +1169,13 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder, if (magic != NO_MAGIC) placeholder++; + if (c->right_alignment && c->right_sb) { + real_sb = sb; + sb = c->right_sb; + } + orig_len = sb->len; consumed = format_commit_one(sb, placeholder, context); - if (magic == NO_MAGIC) - return consumed; if ((orig_len == sb->len) && magic == DEL_LF_BEFORE_EMPTY) { while (sb->len && sb->buf[sb->len - 1] == '\n') @@ -1141,7 +1186,13 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder, else if (magic == ADD_SP_BEFORE_NON_EMPTY) strbuf_insert(sb, orig_len, " ", 1); } - return consumed + 1; + + if (real_sb) + right_align(real_sb, c, 0); + + if (magic != NO_MAGIC) + consumed++; + return consumed; } static size_t userformat_want_item(struct strbuf *sb, const char *placeholder, @@ -1180,12 +1231,14 @@ void format_commit_message(const struct commit *commit, struct format_commit_context context; static const char utf8[] = "UTF-8"; const char *output_enc = pretty_ctx->output_encoding; + struct strbuf right_sb = STRBUF_INIT; memset(&context, 0, sizeof(context)); context.co