Hi
On Sun, Dec 6, 2020 at 12:02 AM Allan McRae <al...@archlinux.org> wrote:
>
> On 6/12/20 4:39 am, Anatol Pomozov wrote:
> > With the recent 'multibar' interface changes TotalDownload has been
> > disabled.
> > Now we have a new UI and we need to find another way to display this
> > information.
> >
> > When 'TotalDownload' config option is enabled we are going to have an extra
> > progress bar at the bottom of the screen that shows how much of the entire
> > download has been completed.
> >
> > Closes FS#68202
>
> Thanks - that progress bar looks good.
>
> >
> > Signed-off-by: Anatol Pomozov <anatol.pomo...@gmail.com>
> > ---
> > doc/pacman.conf.5.asciidoc | 6 +--
> > lib/libalpm/sync.c | 5 --
> > src/pacman/callback.c | 108 ++++++++++++++++++++++++++++++++-----
> > 3 files changed, 96 insertions(+), 23 deletions(-)
> >
> > diff --git a/doc/pacman.conf.5.asciidoc b/doc/pacman.conf.5.asciidoc
> > index dd1d3d5e..32612eb9 100644
> > --- a/doc/pacman.conf.5.asciidoc
> > +++ b/doc/pacman.conf.5.asciidoc
> > @@ -191,10 +191,8 @@ Options
> > not support escape characters.
> >
> > *TotalDownload*::
> > - When downloading, display the amount downloaded, download rate, ETA,
> > - and completed percentage of the entire download list rather
> > - than the percent of each individual download target. The progress
> > - bar is still based solely on the current file download.
> > + When downloading, display an extra progress bar with the amount
> > downloaded,
> > + download rate, ETA, and completed percentage of the entire download
> > list.
> > This option won't work if XferCommand is used.
>
> OK.
>
> >
> > *CheckSpace*::
> > diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
> > index 5d8652a5..e25e56d4 100644
> > --- a/lib/libalpm/sync.c
> > +++ b/lib/libalpm/sync.c
> > @@ -846,11 +846,6 @@ finish:
> > pkg->download_size = 0;
> > }
> >
> > - /* clear out value to let callback know we are done */
> > - if(handle->totaldlcb) {
> > - handle->totaldlcb(0);
> > - }
> > -
>
> OK.
>
> > return ret;
> > }
> >
> > diff --git a/src/pacman/callback.c b/src/pacman/callback.c
> > index 12ab952f..49bc3df4 100644
> > --- a/src/pacman/callback.c
> > +++ b/src/pacman/callback.c
> > @@ -40,8 +40,9 @@
> > #include "conf.h"
> >
> > /* download progress bar */
> > -static off_t list_xfered = 0.0;
>
> Appears that should have been cleaned up a while ago?
>
> > +static int total_enabled = 0;
> > static off_t list_total = 0.0;
> > +static struct pacman_progress_bar *totalbar;
> >
>
> OK
>
> > /* delayed output during progress bar */
> > static int on_progress = 0;
> > @@ -92,6 +93,10 @@ struct pacman_multibar_ui {
> >
> > struct pacman_multibar_ui multibar_ui = {0};
> >
> > +static int dload_progressbar_enabled(void);
> > +static void init_total_progressbar(void);
> > +static void update_total_finalstats(void);
> > +
>
> Why do these need forward declared? Just to avoid moving code around in
> the file?
>
> > void multibar_move_completed_up(bool value) {
> > multibar_ui.move_completed_up = value;
> > }
> > @@ -327,6 +332,10 @@ void cb_event(alpm_event_t *event)
> > case ALPM_EVENT_PKG_RETRIEVE_START:
> > colon_printf(_("Retrieving packages...\n"));
> > on_progress = 1;
> > + total_enabled = config->totaldownload && list_total;
> > + if(total_enabled) {
> > + init_total_progressbar();
> > + }
>
> OK
>
> > break;
> > case ALPM_EVENT_DISKSPACE_START:
> > if(config->noprogressbar) {
> > @@ -388,6 +397,11 @@ void cb_event(alpm_event_t *event)
> > case ALPM_EVENT_PKG_RETRIEVE_DONE:
> > case ALPM_EVENT_PKG_RETRIEVE_FAILED:
> > console_cursor_move_end();
> > + if(total_enabled && dload_progressbar_enabled()) {
> > + update_total_finalstats();
> > + printf("\n");
> > + }
> > + total_enabled = 0;
>
> OK.
>
> > flush_output_list();
> > on_progress = 0;
> > break;
> > @@ -679,11 +693,6 @@ void cb_progress(alpm_progress_t event, const char
> > *pkgname, int percent,
> > void cb_dl_total(off_t total)
> > {
> > list_total = total;
> > - /* if we get a 0 value, it means this list has finished downloading,
> > - * so clear out our list_xfered as well */
> > - if(total == 0) {
> > - list_xfered = 0;
> > - }
> > }
>
> OK
>
> >
> > static int dload_progressbar_enabled(void)
> > @@ -726,6 +735,16 @@ static bool find_bar_for_filename(const char
> > *filename, int *index, struct pacma
> > return false;
> > }
> >
> > +static void init_total_progressbar(void)
> > +{
> > + totalbar = calloc(1, sizeof(struct pacman_progress_bar));
> > + assert(totalbar);
> > + totalbar->filename = "Total progress:";
I need to localize this string. Will resend an updated patch for it.
Is "Total progress:" a good prefix for the total bar?
> > + totalbar->init_time = get_time_ms();
> > + totalbar->total_size = list_total;
> > + totalbar->rate = 0.0;
> > +}
> > +
>
> OK.
>
> > static void draw_pacman_progress_bar(struct pacman_progress_bar *bar)
> > {
> > int infolen;
> > @@ -851,10 +870,17 @@ static void dload_init_event(const char *filename,
> > alpm_download_event_init_t *d
> > printf(_(" %s downloading...\n"), filename);
> > multibar_ui.cursor_lineno++;
> > multibar_ui.active_downloads_num++;
> > +
> > + if(total_enabled) {
> > + /* redraw the total download progress bar */
> > + draw_pacman_progress_bar(totalbar);
> > + printf("\n");
> > + multibar_ui.cursor_lineno++;
> > + }
> > }
>
> OK.
>
> >
> > -/* Draws download progress */
> > -static void dload_progress_event(const char *filename,
> > alpm_download_event_progress_t *data)
> > +
> > +static off_t update_file_dload_bar(const char *filename,
> > alpm_download_event_progress_t *data)
> > {
> > int index;
> > struct pacman_progress_bar *bar;
> > @@ -862,10 +888,7 @@ static void dload_progress_event(const char *filename,
> > alpm_download_event_progr
> > double last_chunk_rate;
> > int64_t timediff;
> > bool ok;
> > -
> > - if(!dload_progressbar_enabled()) {
> > - return;
> > - }
> > + off_t last_chunk_amount;
> >
>
> OK.
>
> > ok = find_bar_for_filename(filename, &index, &bar);
> > assert(ok);
> > @@ -875,11 +898,12 @@ static void dload_progress_event(const char
> > *filename, alpm_download_event_progr
> >
> > if(timediff < UPDATE_SPEED_MS) {
> > /* return if the calling interval was too short */
> > - return;
> > + return 0;
> > }
> > bar->sync_time = curr_time;
> >
> > - last_chunk_rate = (double)(data->downloaded - bar->xfered) /
> > (timediff / 1000.0);
> > + last_chunk_amount = data->downloaded - bar->xfered;
> > + last_chunk_rate = (double)last_chunk_amount * 1000 / timediff;
>
> OK.
>
> > /* average rate to reduce jumpiness */
> > bar->rate = (last_chunk_rate + 2 * bar->rate) / 3;
> > if(bar->rate > 0.0) {
> > @@ -894,6 +918,62 @@ static void dload_progress_event(const char *filename,
> > alpm_download_event_progr
> >
> > console_cursor_goto_bar(index);
> > draw_pacman_progress_bar(bar);
> > +
> > + return last_chunk_amount;
> > +}
> > +
> > +static void update_total_dload_bar(off_t last_chunk_amount)
> > +{
> > + int64_t curr_time = get_time_ms();
> > + int64_t timediff;
> > + double last_chunk_rate;
> > +
> > + timediff = curr_time - totalbar->init_time;
> > + totalbar->xfered += last_chunk_amount;
> > + last_chunk_rate = (double)totalbar->xfered * 1000 / timediff;
> > + totalbar->rate = (last_chunk_rate + 2 * totalbar->rate) / 3;
> > + totalbar->eta = (totalbar->total_size - totalbar->xfered) /
> > totalbar->rate;
> > +
> > + console_cursor_move_end();
> > + draw_pacman_progress_bar(totalbar);
> > +}
>
> OK.
>
> > +
> > +static void update_total_finalstats(void)
> > +{
> > + int64_t timediff;
> > +
> > + /* compute final values */
> > + totalbar->xfered = totalbar->total_size;
> > + timediff = get_time_ms() - totalbar->init_time;
> > +
> > + /* if transfer was too fast, treat it as a 1ms transfer, for the sake
> > + * of the rate calculation */
> > + if(timediff < 1)
> > + timediff = 1;
> > +
> > + totalbar->rate = (double)totalbar->xfered * 1000 / timediff;
> > + /* round elapsed time (in ms) to the nearest second */
> > + totalbar->eta = (unsigned int)(timediff + 500) / 1000;
> > +
> > + console_cursor_move_end();
> > + draw_pacman_progress_bar(totalbar);
> > +}
>
> OK. I get the feeling that this and the above function are duplicating
> a bit of code from normal progress bars. But a quick look leads me to
> think it is not worth refactoring.
These 2 functions (update and finalupdate for total bar) are slightly different.
But there is some duplication in "update file" and "update total" bar
logic. I can try to refactor and see if it makes things cleaner.
> > +
> > +/* Handles download progress event */
> > +static void dload_progress_event(const char *filename,
> > alpm_download_event_progress_t *data)
> > +{
> > + off_t last_chunk_amount;
> > +
> > + if(!dload_progressbar_enabled()) {
> > + return;
> > + }
> > +
> > + last_chunk_amount = update_file_dload_bar(filename, data);
> > +
> > + if(total_enabled && last_chunk_amount) {
> > + update_total_dload_bar(last_chunk_amount);
> > + }
> > +
> > fflush(stdout);
> > }
> >
>
>
> OK.
