[PATCH] gdb/debuginfod: Improve progress updates

[PATCH] gdb/debuginfod: Improve progress updates

[Aaron Merey]

Only print the size of the download if it is known.

If the size is less than 0.01 MB then print the size in KB instead.

If the size is unknown then also print a throbber to indicate that the
download is progressing.
---
 gdb/debuginfod-support.c | 64 ++++++++++++++++++++++++++++++----------
 1 file changed, 49 insertions(+), 15 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 56d8e7781c5..3094269f446 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -83,6 +83,7 @@ struct user_data
   const char * const desc;
   const char * const fname;
   gdb::optional<ui_out::progress_meter> meter;
+  bool printed_spin = false;
 };
 
 /* Deleter for a debuginfod_client.  */
@@ -112,24 +113,51 @@ progressfn (debuginfod_client *c, long cur, long total)
       return 1;
     }
 
-  if (total == 0)
-    return 0;
+  string_file styled_filename (current_uiout->can_emit_style_escape ());
+  fprintf_styled (&styled_filename,
+		  file_name_style.style (),
+		  "%s",
+		  data->fname);
 
-  if (!data->meter.has_value ())
+  if (total > 0)
     {
-      float size_in_mb = 1.0f * total / (1024 * 1024);
-      string_file styled_filename (current_uiout->can_emit_style_escape ());
-      fprintf_styled (&styled_filename,
-		      file_name_style.style (),
-		      "%s",
-		      data->fname);
-      std::string message
-	= string_printf ("Downloading %.2f MB %s %s", size_in_mb, data->desc,
-			 styled_filename.c_str());
-      data->meter.emplace (current_uiout, message, 1);
-    }
+      if (!data->meter.has_value ())
+	{
+	  double size = 1.0f * total / 1024;
+	  std::string unit = "KB";
+
+	  /* Switch to MB if size greater than 0.01 MB.  */
+	  if (size > 10.24)
+	    {
+	      size /= 1024;
+	      unit = "MB";
+	    }
+
+	  /* Overwrite an existing progress update line with no download
+	     size information.  */
+	  if (data->printed_spin)
+	    {
+	      current_uiout->message ("\r");
+	      data->printed_spin = false;
+	    }
+
+	  std::string message = string_printf ("Downloading %.2f %s %s %s",
+					       size, unit.c_str (),
+					       data->desc,
+					       styled_filename.c_str ());
+	  data->meter.emplace (current_uiout, message, 1);
+	}
 
-  current_uiout->progress ((double)cur / (double)total);
+      current_uiout->progress ((double)cur / (double)total);
+    }
+  else
+    {
+      static int spin = 0;
+      current_uiout->message (_("\rDownloading %s %s %c"), data->desc,
+			      styled_filename.c_str (), "-/|\\"[spin++ % 4]);
+      current_uiout->flush ();
+      data->printed_spin = true;
+    }
 
   return 0;
 }
@@ -217,6 +245,9 @@ debuginfod_source_query (const unsigned char *build_id,
 					build_id_len,
 					srcpath,
 					nullptr));
+  if (data.printed_spin)
+    current_uiout->message ("\b \n");
+
   debuginfod_set_user_data (c, nullptr);
 
   if (debuginfod_verbose > 0 && fd.get () < 0 && fd.get () != -ENOENT)
@@ -259,6 +290,9 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
 
   scoped_fd fd (debuginfod_find_debuginfo (c, build_id, build_id_len,
 					   &dname));
+  if (data.printed_spin)
+    current_uiout->message ("\b \n");
+
   debuginfod_set_user_data (c, nullptr);
 
   if (debuginfod_verbose > 0 && fd.get () < 0 && fd.get () != -ENOENT)
-- 
2.33.1

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

[Patrick Monnerat]

On 1/12/22 03:54, Aaron Merey via Gdb-patches wrote:
> Only print the size of the download if it is known.
>
>   
> -  current_uiout->progress ((double)cur / (double)total);
> +      current_uiout->progress ((double)cur / (double)total);
> +    }
> +  else
> +    {
> +      static int spin = 0;
> +      current_uiout->message (_("\rDownloading %s %s %c"), data->desc,
> +			      styled_filename.c_str (), "-/|\\"[spin++ % 4]);
> +      current_uiout->flush ();
> +      data->printed_spin = true;
> +    }

Plase can you still call current_uiout->progress() with the negative cur 
value and move your spin process in it ?. I use ->progress() in insight 
for a graphical progress/spinning bar and this negative value is a good 
indicator that the total size is unknown.

https://sourceware.org/git/?p=insight.git;a=blob;f=gdbtk/generic/gdbtk-interp.c;hb=HEAD#l71

Or maybe you have another solution for the GUI?

Thanks in advance.

Patrick

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

[Aaron Merey]

Hi Patrick,

On Tue, Jan 11, 2022 at 10:27 PM Patrick Monnerat via Gdb-patches
<gdb-patches@sourceware.org> wrote:
> On 1/12/22 03:54, Aaron Merey via Gdb-patches wrote:
> > Only print the size of the download if it is known.
> >
> >
> > -  current_uiout->progress ((double)cur / (double)total);
> > +      current_uiout->progress ((double)cur / (double)total);
> > +    }
> > +  else
> > +    {
> > +      static int spin = 0;
> > +      current_uiout->message (_("\rDownloading %s %s %c"), data->desc,
> > +                           styled_filename.c_str (), "-/|\\"[spin++ % 4]);
> > +      current_uiout->flush ();
> > +      data->printed_spin = true;
> > +    }
>
> Plase can you still call current_uiout->progress() with the negative cur
> value and move your spin process in it ?. I use ->progress() in insight
> for a graphical progress/spinning bar and this negative value is a good
> indicator that the total size is unknown.
>
> https://sourceware.org/git/?p=insight.git;a=blob;f=gdbtk/generic/gdbtk-interp.c;hb=HEAD#l71
>
> Or maybe you have another solution for the GUI?

I don't think I fully understand the purpose of calling progress() with a
negative value since the progress bar is useful only if we know the
percentage of the transfer that has completed.

When it isn't known then we don't bother printing the download size
or progress bar. Instead the throbber indicates that the transfer is ongoing.

Let me know if I have misunderstood.

Aaron

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

[Patrick Monnerat]

On 1/12/22 23:20, Aaron Merey wrote:
> Hi Patrick,
>
> On Tue, Jan 11, 2022 at 10:27 PM Patrick Monnerat via Gdb-patches
> <gdb-patches@sourceware.org> wrote:
>> On 1/12/22 03:54, Aaron Merey via Gdb-patches wrote:
>>> Only print the size of the download if it is known.
>>>
>>>
>>> -  current_uiout->progress ((double)cur / (double)total);
>>> +      current_uiout->progress ((double)cur / (double)total);
>>> +    }
>>> +  else
>>> +    {
>>> +      static int spin = 0;
>>> +      current_uiout->message (_("\rDownloading %s %s %c"), data->desc,
>>> +                           styled_filename.c_str (), "-/|\\"[spin++ % 4]);
>>> +      current_uiout->flush ();
>>> +      data->printed_spin = true;
>>> +    }
>> Plase can you still call current_uiout->progress() with the negative cur
>> value and move your spin process in it ?. I use ->progress() in insight
>> for a graphical progress/spinning bar and this negative value is a good
>> indicator that the total size is unknown.
>>
>> https://sourceware.org/git/?p=insight.git;a=blob;f=gdbtk/generic/gdbtk-interp.c;hb=HEAD#l71
>>
>> Or maybe you have another solution for the GUI?
> I don't think I fully understand the purpose of calling progress() with a
> negative value since the progress bar is useful only if we know the
> percentage of the transfer that has completed.
>
> When it isn't known then we don't bother printing the download size
> or progress bar. Instead the throbber indicates that the transfer is ongoing._out

Currently, I have a graphical throbber triggered by calls to ui_out 
do_progress_notify() method. The message is displayed in the status bar, 
not in the throbber/progressbar widget.

The widget occupied by the progressbar/throbber is also used to display 
some other kind of information while not downloading.

I rely on calls to ui_out methods do_progress_start(), 
do_progress_notify() and do_progress_end() to multiplex this zone.

In do_progress_notify(), I check if the argument is negative to know if 
I must deal with a progress bar or a throbber.

Thanks for considering it,

Patrick

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

[Aaron Merey]

On Wed, Jan 12, 2022 at 6:35 PM Patrick Monnerat via Gdb-patches
<gdb-patches@sourceware.org> wrote:
> Currently, I have a graphical throbber triggered by calls to ui_out
> do_progress_notify() method. The message is displayed in the status bar,
> not in the throbber/progressbar widget.
>
> The widget occupied by the progressbar/throbber is also used to display
> some other kind of information while not downloading.
>
> I rely on calls to ui_out methods do_progress_start(),
> do_progress_notify() and do_progress_end() to multiplex this zone.

To keep the UI simple, for each download there is a line printed
indicating the name and type of file being downloaded as well as
the size, if available. If the size is unknown then the throbber is
displayed at the end of this line. If the size is known then the
progress bar is printed on the following line.

Do you think it would be better to print the throbber on its own line?

> In do_progress_notify(), I check if the argument is negative to know if
> I must deal with a progress bar or a throbber.

I do basically the same thing by checking total > 0. If <= 0 then the
transfer size is unknown, so we display the throbber in that case.

Thanks,
Aaron

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

[Patrick Monnerat]

Hi Aaron,

On 1/13/22 19:09, Aaron Merey wrote:
> On Wed, Jan 12, 2022 at 6:35 PM Patrick Monnerat via Gdb-patches
> <gdb-patches@sourceware.org> wrote:
>> Currently, I have a graphical throbber triggered by calls to ui_out
>> do_progress_notify() method. The message is displayed in the status bar,
>> not in the throbber/progressbar widget.
>>
>> The widget occupied by the progressbar/throbber is also used to display
>> some other kind of information while not downloading.
>>
>> I rely on calls to ui_out methods do_progress_start(),
>> do_progress_notify() and do_progress_end() to multiplex this zone.
> To keep the UI simple, for each download there is a line printed
> indicating the name and type of file being downloaded as well as
> the size, if available. If the size is unknown then the throbber is
> displayed at the end of this line. If the size is known then the
> progress bar is printed on the following line.
> Do you think it would be better to print the throbber on its own line?

Probably. At least it would separate the text (that goes in the status 
widget in my case), from the progress/throbber which are graphical concepts.

In insight's case, the throbber is a small token that runs back and 
forth in the progress widget.

In text mode, why not something like:

<      XXX                     >

occuping the whole line and with the XXX rolling or going back and 
forth? This could be triggered by do_progress_*() calls if progress() is 
called even when the size is unknown. Just a suggestion.

Thanks for paying attention to my situation,

Patrick

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

[Tom Tromey]

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

Aaron> I don't think I fully understand the purpose of calling progress() with a
Aaron> negative value since the progress bar is useful only if we know the
Aaron> percentage of the transfer that has completed.

Perhaps the throbber stuff could be moved to cli-out and progress_meter
could be changed to recognize the two different cases.

Tom

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

[Patrick Monnerat]

On 1/13/22 22:27, Tom Tromey wrote:
>>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
> Aaron> I don't think I fully understand the purpose of calling progress() with a
> Aaron> negative value since the progress bar is useful only if we know the
> Aaron> percentage of the transfer that has completed.
>
> Perhaps the throbber stuff could be moved to cli-out and progress_meter
> could be changed to recognize the two different cases.

This is exactly what Insight does! Thanks Tom for your clear and concise 
formulation.

Patrick