Re: [pacman-dev] [PATCH] Implement TotalDownload functionality

Hi On Sun, Dec 6, 2020 at 12:02 AM Allan McRae 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 --- 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.