Re: [PATCH v3] gdb: Improve debuginfod progress updates

[Tom Tromey <tom@tromey.com>]

>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

Aaron> The new function build_message will try to fit as much of the
Aaron> untruncated message as possible.  For example if the original
Aaron> message is:
Aaron>   Downloading XX MB separate debug info for /aa/bb/cc/dd/ee

Instead of this, which seems like it can hide useful info from the user,
what do you think about a "cylon"-style display:

Downloading mumble.so:
[      #             ]

... where the '#' moves back and forth inside the [].

See the "indeterminate" example here:

    https://github.com/p-ranav/indicators

(That page made me LOL with the emoji rocket, maybe gdb should be more
emojified.)

Anyway it seems like this sort of change would avoid some hair in the
implementation.

Aaron>    if (!stream->isatty ())
Aaron>      {
Aaron> -      fprintf_unfiltered (stream, "%s...", meter.name.c_str ());
Aaron> +      fprintf_unfiltered (stream, "%s\n", info.name.c_str ());

I suspect the flush can be removed here, it's normally only needed when
the previous print doesn't end in a newline.

Aaron> +      info.state = should_print ? progress_update::START
Aaron> +				: progress_update::NO_PRINT;

gdb style is to parenthesize the RHS of the assignment in a case like
this; something like:

      info.state = (should_print
                    ? progress_update::START
		    : progress_update::NO_PRINT);

Aaron> +/* Get the current state of the most recent progress update.  */
Aaron> +
Aaron> +cli_ui_out::progress_update::state
Aaron> +cli_ui_out::get_progress_state ()
Aaron> +{

I didn't really get this part of the design.

It seems to me that the debuginfod support code just wants to notify the
user of some progress.  It might know how much data to expect, or it
might not -- so when first starting the notification, it seems like it
can just explain this.  After that, it could just say "got xyz", and the
progress meter itself can be responsible for all state changes, UI
display, etc.  That is, I don't understand why this needs to be exported
from the cli-out.

Aaron> +      if (data->progress.has_value ())
Aaron> +	data->progress.reset ();

In retrospect I also don't understand why the progress meter is even an
optional.  It seems like one could be made unconditionally and then the
display (or not) left to the ui-out implementation.

Aaron> +  else if (debuginfod_verbose > 0 && fd < 0 && fd != -ENOENT)
Aaron> +    printf_filtered (_("Download failed: %s. " \
Aaron> +		       "Continuing without %s %s.\n"),
Aaron> +		     safe_strerror (-fd), data.desc,
Aaron> +		     styled_fname.c_str ());

It seems like a download failure should be reported even in non-verbose
mode.

Aaron> +void
Aaron> +mi_ui_out::do_progress_start (const std::string &name, bool should_print)

I tend to doubt that MI should be changed.

Technically MI does have a progress notification approach, see
mi_load_progress.  I don't know if anything MI consumer actually uses
this, though, and so I'm not sure if it makes sense to try to wire this
up to debuginfo downloads.

Tom