[5/5] progress: break too long progress bar lines
diff mbox series

Message ID 20190325103844.26749-6-szeder.dev@gmail.com
State New
Headers show
Series
  • Progress display fixes
Related show

Commit Message

SZEDER Gábor March 25, 2019, 10:38 a.m. UTC
Some of the recently added progress indicators have quite long titles,
which might be even longer when translated to some languages, and when
they are shown while operating on bigger repositories, then the
progress bar grows longer than the default 80 column terminal width.

When the progress bar exceeds the width of the terminal it gets
line-wrapped, and after that the CR at the end doesn't return to the
beginning of the progress bar, but to the first column of its last
line.  Consequently, the first line of the previously shown progress
bar is not overwritten by the next, and we end up with a bunch of
truncated progress bar lines scrolling past:

  $ LANG=es_ES.UTF-8 git commit-graph write
  Encontrando commits para commit graph entre los objetos empaquetados:   2% (1599
  Encontrando commits para commit graph entre los objetos empaquetados:   3% (1975
  Encontrando commits para commit graph entre los objetos empaquetados:   4% (2633
  Encontrando commits para commit graph entre los objetos empaquetados:   5% (3292
  [...]

Prevent this by breaking progress bars after the title once they
exceed the width of the terminal, so the counter and optional
percentage and throughput, i.e. all changing parts, are on the last
line.  Subsequent updates will from then on only refresh the changing
parts, but not the title, and it will look like this:

  $ LANG=es_ES.UTF-8 ~/src/git/git commit-graph write
  Encontrando commits para commit graph entre los objetos empaquetados:
    100% (6584502/6584502), listo.
  Calculando números de generación de commit graph: 100% (824705/824705), listo.
  Escribiendo commit graph en 4 pasos: 100% (3298820/3298820), listo.

Note that the number of columns in the terminal is cached by
term_columns(), so this might not kick in when it should when a
terminal window is resized while the operation is running.
Furthermore, this change won't help if the terminal is so narrow that
the counters don't fit on one line, but I would put this in the "If it
hurts, don't do it" box.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

Comments

Duy Nguyen March 25, 2019, 11:02 a.m. UTC | #1
On Mon, Mar 25, 2019 at 5:41 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> Some of the recently added progress indicators have quite long titles,
> which might be even longer when translated to some languages, and when
> they are shown while operating on bigger repositories, then the
> progress bar grows longer than the default 80 column terminal width.
>
> When the progress bar exceeds the width of the terminal it gets
> line-wrapped, and after that the CR at the end doesn't return to the
> beginning of the progress bar, but to the first column of its last
> line.  Consequently, the first line of the previously shown progress
> bar is not overwritten by the next, and we end up with a bunch of
> truncated progress bar lines scrolling past:
>
>   $ LANG=es_ES.UTF-8 git commit-graph write
>   Encontrando commits para commit graph entre los objetos empaquetados:   2% (1599
>   Encontrando commits para commit graph entre los objetos empaquetados:   3% (1975
>   Encontrando commits para commit graph entre los objetos empaquetados:   4% (2633
>   Encontrando commits para commit graph entre los objetos empaquetados:   5% (3292
>   [...]
>
> Prevent this by breaking progress bars after the title once they
> exceed the width of the terminal, so the counter and optional
> percentage and throughput, i.e. all changing parts, are on the last
> line.  Subsequent updates will from then on only refresh the changing
> parts, but not the title, and it will look like this:
>
>   $ LANG=es_ES.UTF-8 ~/src/git/git commit-graph write
>   Encontrando commits para commit graph entre los objetos empaquetados:
>     100% (6584502/6584502), listo.
>   Calculando números de generación de commit graph: 100% (824705/824705), listo.
>   Escribiendo commit graph en 4 pasos: 100% (3298820/3298820), listo.
>
> Note that the number of columns in the terminal is cached by
> term_columns(), so this might not kick in when it should when a
> terminal window is resized while the operation is running.
> Furthermore, this change won't help if the terminal is so narrow that
> the counters don't fit on one line, but I would put this in the "If it
> hurts, don't do it" box.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  progress.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/progress.c b/progress.c
> index b37a5398c5..36aaeea340 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -8,7 +8,7 @@
>   * published by the Free Software Foundation.
>   */
>
> -#include "git-compat-util.h"
> +#include "cache.h"
>  #include "gettext.h"
>  #include "progress.h"
>  #include "strbuf.h"
> @@ -37,6 +37,8 @@ struct progress {
>         struct throughput *throughput;
>         uint64_t start_ns;
>         struct strbuf counters_sb;
> +       int title_len;
> +       int split;
>  };
>
>  static volatile sig_atomic_t progress_update;
> @@ -119,8 +121,22 @@ static void display(struct progress *progress, uint64_t n, const char *done)
>                 const char *eol = done ? done : "\r";
>                 int clear_len = counters_sb->len < last_count_len ?
>                                 last_count_len - counters_sb->len : 0;
> -               fprintf(stderr, "%s: %s%-*s", progress->title,
> -                       counters_sb->buf, clear_len, eol);
> +               int cols = term_columns();
> +
> +               if (progress->split) {
> +                       fprintf(stderr, "  %s%-*s", counters_sb->buf, clear_len,
> +                               eol);
> +               } else if (!done &&
> +                          cols < progress->title_len + counters_sb->len + 2) {
> +                       clear_len = progress->title_len + 1 < cols ?
> +                                   cols - progress->title_len - 1 : 0;
> +                       fprintf(stderr, "%s:%*s\n  %s%s", progress->title,
> +                                       clear_len, "", counters_sb->buf, eol);
> +                       progress->split = 1;
> +               } else {
> +                       fprintf(stderr, "%s: %s%-*s", progress->title,
> +                               counters_sb->buf, clear_len, eol);
> +               }
>                 fflush(stderr);
>                 progress_update = 0;
>         }
> @@ -225,6 +241,8 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,
>         progress->throughput = NULL;
>         progress->start_ns = getnanotime();
>         strbuf_init(&progress->counters_sb, 0);
> +       progress->title_len = strlen(title);

I think you need utf8_strwidth() so that the "cols < title_len"
comparison above works for multibyte encoding too.

> +       progress->split = 0;
>         set_progress_signal();
>         return progress;
>  }
> --
> 2.21.0.539.g07239c3a71.dirty
>
SZEDER Gábor March 25, 2019, 11:12 a.m. UTC | #2
On Mon, Mar 25, 2019 at 06:02:13PM +0700, Duy Nguyen wrote:
> > @@ -225,6 +241,8 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,
> >         progress->throughput = NULL;
> >         progress->start_ns = getnanotime();
> >         strbuf_init(&progress->counters_sb, 0);
> > +       progress->title_len = strlen(title);
> 
> I think you need utf8_strwidth() so that the "cols < title_len"
> comparison above works for multibyte encoding too.

Oh, indeed.  I remember seeing the Chinese translation and wondering
about it, but then completely forgot by the time I got around to it.

> > +       progress->split = 0;
> >         set_progress_signal();
> >         return progress;
> >  }
> > --
> > 2.21.0.539.g07239c3a71.dirty
> >
> 
> 
> -- 
> Duy

Patch
diff mbox series

diff --git a/progress.c b/progress.c
index b37a5398c5..36aaeea340 100644
--- a/progress.c
+++ b/progress.c
@@ -8,7 +8,7 @@ 
  * published by the Free Software Foundation.
  */
 
-#include "git-compat-util.h"
+#include "cache.h"
 #include "gettext.h"
 #include "progress.h"
 #include "strbuf.h"
@@ -37,6 +37,8 @@  struct progress {
 	struct throughput *throughput;
 	uint64_t start_ns;
 	struct strbuf counters_sb;
+	int title_len;
+	int split;
 };
 
 static volatile sig_atomic_t progress_update;
@@ -119,8 +121,22 @@  static void display(struct progress *progress, uint64_t n, const char *done)
 		const char *eol = done ? done : "\r";
 		int clear_len = counters_sb->len < last_count_len ?
 				last_count_len - counters_sb->len : 0;
-		fprintf(stderr, "%s: %s%-*s", progress->title,
-			counters_sb->buf, clear_len, eol);
+		int cols = term_columns();
+
+		if (progress->split) {
+			fprintf(stderr, "  %s%-*s", counters_sb->buf, clear_len,
+				eol);
+		} else if (!done &&
+			   cols < progress->title_len + counters_sb->len + 2) {
+			clear_len = progress->title_len + 1 < cols ?
+				    cols - progress->title_len - 1 : 0;
+			fprintf(stderr, "%s:%*s\n  %s%s", progress->title,
+					clear_len, "", counters_sb->buf, eol);
+			progress->split = 1;
+		} else {
+			fprintf(stderr, "%s: %s%-*s", progress->title,
+				counters_sb->buf, clear_len, eol);
+		}
 		fflush(stderr);
 		progress_update = 0;
 	}
@@ -225,6 +241,8 @@  static struct progress *start_progress_delay(const char *title, uint64_t total,
 	progress->throughput = NULL;
 	progress->start_ns = getnanotime();
 	strbuf_init(&progress->counters_sb, 0);
+	progress->title_len = strlen(title);
+	progress->split = 0;
 	set_progress_signal();
 	return progress;
 }