[pacman-dev] [PATCH] Implement multibar UI

Anatol Pomozov anatol.pomozov at gmail.com
Thu May 7 20:26:38 UTC 2020


Hi

Thank you for looking at this patch

On Thu, Apr 30, 2020 at 6:37 AM Allan McRae <allan at archlinux.org> wrote:
>
> On 22/4/20 1:46 pm, Anatol Pomozov wrote:
> > Multiplexed download requires ability to draw UI for multiple active progress bars.
> > To implement it we use ANSI codes to move cursor up/down.
> > `active_dls` variable represents the list of active progress bars.
> > `struct dl_progress_bar` is a data structure for a progress bar.
> >
> > In some cases we want to keep progress bars in order (e.g. db downloads).
> > In some other cases (package downloads) we want to move completed items to the
> > top of the screen. Global variable `multibar_move_complete_top` allows to
> > configure such behavior.
> >
> > Signature file downloads are not shown at the UI.
> >
> > Signed-off-by: Anatol Pomozov <anatol.pomozov at gmail.com>
>
>
> I have taken a first pass at this.  I can't guarentee this will be my
> last set of comments...
>
> > ---
> >  lib/libalpm/dload.c   |  31 +---
> >  lib/libalpm/dload.h   |   1 -
> >  src/pacman/callback.c | 393 +++++++++++++++++++++++++++++-------------
> >  src/pacman/callback.h |   3 +
> >  src/pacman/sync.c     |   2 +
> >  5 files changed, 285 insertions(+), 145 deletions(-)
> >
> > diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> > index c09abde4..cf467ff0 100644
> > --- a/lib/libalpm/dload.c
> > +++ b/lib/libalpm/dload.c
> > @@ -100,6 +100,11 @@ static int dload_progress_cb(void *file, curl_off_t dltotal, curl_off_t dlnow,
> >               return 1;
> >       }
> >
> > +     if(dlnow < 0 || dltotal <= 0 || dlnow > dltotal) {
> > +             /* bogus values : stop here */
> > +             return 0;
> > +     }
> > +
>
> OK.
>
> >       current_size = payload->initial_size + dlnow;
> >
> >       /* is our filesize still under any set limit? */
> > @@ -115,34 +120,15 @@ static int dload_progress_cb(void *file, curl_off_t dltotal, curl_off_t dlnow,
> >
> >       total_size = payload->initial_size + dltotal;
> >
> > -     if(dltotal == 0 || payload->prevprogress == total_size) {
> > +     if(payload->prevprogress == total_size) {
> >               return 0;
> >       }
>
> OK.
>
> >
> > -     /* initialize the progress bar here to avoid displaying it when
> > -      * a repo is up to date and nothing gets downloaded.
> > -      * payload->handle->dlcb will receive the remote_name
> > -      * and the following arguments:
> > -      * 0, -1: download initialized
> > -      * 0, 0: non-download event
> > -      * x {x>0}, x: download complete
> > -      * x {x>0, x<y}, y {y > 0}: download progress, expected total is known */
> > -     if(!payload->cb_initialized) {
> > -             cb_data.downloaded = 0;
> > -             cb_data.total = -1;
> > -             payload->cb_initialized = 1;
> > -     }
> > -     if(payload->prevprogress == current_size) {
> > -             cb_data.downloaded = 0;
> > -             cb_data.total = 0;
> > -     } else {
> >       /* do NOT include initial_size since it wasn't part of the package's
> >        * download_size (nor included in the total download size callback) */
> > -             cb_data.downloaded = dlnow;
> > -             cb_data.total = dltotal;
> > -     }
> > +     cb_data.total = dltotal;
> > +     cb_data.downloaded = dlnow;
> >       payload->handle->dlcb(payload->remote_name, ALPM_DOWNLOAD_PROGRESS, &cb_data);
> > -
> >       payload->prevprogress = current_size;
> >
>
> OK.
>
> >       return 0;
> > @@ -1160,5 +1146,4 @@ void _alpm_dload_payload_reset_for_retry(struct dload_payload *payload)
> >       payload->initial_size += payload->prevprogress;
> >       payload->prevprogress = 0;
> >       payload->unlink_on_fail = 0;
> > -     payload->cb_initialized = 0;
> >  }
>
> OK.
>
> > diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h
> > index a40b51b7..142568e8 100644
> > --- a/lib/libalpm/dload.h
> > +++ b/lib/libalpm/dload.h
> > @@ -42,7 +42,6 @@ struct dload_payload {
> >       int errors_ok;
> >       int unlink_on_fail;
> >       int trust_remote_name;
> > -     int cb_initialized;
> >  #ifdef HAVE_LIBCURL
> >       CURL *curl;
> >       char error_buffer[CURL_ERROR_SIZE];
>
> OK.
>
> > diff --git a/src/pacman/callback.c b/src/pacman/callback.c
> > index 387319b6..6133a036 100644
> > --- a/src/pacman/callback.c
> > +++ b/src/pacman/callback.c
> > @@ -18,6 +18,8 @@
> >   *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >   */
> >
> > +#include <assert.h>
> > +#include <stdbool.h>
> >  #include <stdio.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> > @@ -45,6 +47,8 @@ static off_t list_total = 0.0;
> >  static int on_progress = 0;
> >  static alpm_list_t *output = NULL;
> >
> > +static void cursor_goto_end(void);
> > +
>
> Does that need forward declared?  Should all the cursor functions go in
> a new file?

currently this function defined in the middle of this file, closer to
'draw UI' logic block. But this particular function is used in
cb_event() closer to beginning of the file. Hence this forward
declaration.

In case if we want to move cursor functions to a separate file then
this forward declaration will go away.

> >  /* update speed for the fill_progress based functions */
> >  #define UPDATE_SPEED_MS 200
> >
> > @@ -152,11 +156,7 @@ static void fill_progress(const int bar_percent, const int disp_percent,
> >               printf(" %3d%%", disp_percent);
> >       }
> >
> > -     if(bar_percent == 100) {
> > -             putchar('\n');
> > -     } else {
> > -             putchar('\r');
> > -     }
> > +     putchar('\r');
> >       fflush(stdout);
> >  }
> >
> > @@ -346,6 +346,7 @@ void cb_event(alpm_event_t *event)
> >               case ALPM_EVENT_DB_RETRIEVE_FAILED:
> >               case ALPM_EVENT_RETRIEVE_DONE:
> >               case ALPM_EVENT_RETRIEVE_FAILED:
> > +                     cursor_goto_end();
> >                       flush_output_list();
> >                       on_progress = 0;
> >                       break;
> > @@ -629,6 +630,7 @@ void cb_progress(alpm_progress_t event, const char *pkgname, int percent,
> >       fill_progress(percent, percent, cols - infolen);
> >
> >       if(percent == 100) {
> > +             putchar('\n');
> >               flush_output_list();
> >               on_progress = 0;
> >       } else {
> > @@ -647,124 +649,118 @@ void cb_dl_total(off_t total)
> >       }
> >  }
> >
> > -/* callback to handle display of download progress */
> > -static void dload_progress_event(const char *filename, off_t file_xfered, off_t file_total)
> > +static int dload_progressbar_enabled(void)
> >  {
> > -     static double rate_last;
> > -     static off_t xfered_last;
> > -     static int64_t initial_time = 0;
> > -     int infolen;
> > -     int filenamelen;
> > -     char *fname, *p;
> > -     /* used for wide character width determination and printing */
> > -     int len, wclen, wcwid, padwid;
> > -     wchar_t *wcfname;
> > +     return !config->noprogressbar && getcols() != 0;
>
> Add some parenthises around this, and use a == rather than !

noprogressbar is a boolean value essentially. Using "!" for booleans
is a normal practice, IMHO using "== 0" for it is a bit less readable.

And negation operator "!" used in pacman sources actively.

> > +}
> >
> > -     int totaldownload = 0;
> > -     off_t xfered, total;
> > -     double rate = 0.0;
> > -     unsigned int eta_h = 0, eta_m = 0, eta_s = 0;
> > -     double rate_human, xfered_human;
> > -     const char *rate_label, *xfered_label;
> > -     int file_percent = 0, total_percent = 0;
> > +struct dl_progress_bar {
> > +     const char *filename;
> > +     off_t xfered;
> > +     off_t total_size;
> > +     uint64_t init_time; /* Time when this download started doing any progress */
> > +     uint64_t sync_time; /* Last time we updated the bar info */
> > +     double rate;
> > +     unsigned int eta; /* ETA in seconds */
> > +     bool completed; /* transfer is completed */
> > +};
> >
>
> OK.
>
> > -     const unsigned short cols = getcols();
> > +/* List of active downloads handled by multibar UI.
> > + * Once the first download in the list is completed it is removed
> > + * from this list and we never redraw it anymore.
> > + * If the download in this list then UI can redraw the progress bar or change
>
> If the download is in this list, then the UI

done

>
> > + * the order of the bars (e.g. completed bar is moved to the top of the list)
>
> e.g. moving completed bars to the top ...
>
>
> > + */
> > +alpm_list_t *active_dls = NULL;
> >
>
> This passes my test for being a global variable...  but can you make the
> name more unique?   pacman_active_downloads?

Done.

I also renamed 'active_bars_no' and this var is tightly coupled with
pacman_active_downloads.

>
> > -     /* Nothing has changed since last callback; stop here */
> > -     if(file_xfered == 0 && file_total == 0) {
> > -             return;
> > -     }
> > +/* Number of active progress bars that multibar UI handles.
> > + * Each progress bar has a corresponding active download.
> > + * Note that some downloads might not have a bar, e.g. optional signature files
> > + * do not have a corresponding bar.
> > + */
> > +size_t active_bars_no = 0;
> >
> > -     if(config->noprogressbar || cols == 0) {
> > -             if(file_xfered == 0 && file_total == -1) {
> > -                     printf(_("downloading %s...\n"), filename);
> > -                     fflush(stdout);
> > -             }
> > -             return;
> > -     }
> > +int multibar_move_complete_top = 0;

Naming is hard. I tried to find a good short name for this global var
but did not find anything better than
multibar_move_complete_top or multibar_move_completed_up

> >
> > -     infolen = cols * 6 / 10;
> > -     if(infolen < 50) {
> > -             infolen = 50;
> > -     }
> > -     /* only use TotalDownload if enabled and we have a callback value */
> > -     if(config->totaldownload && list_total) {
> > -             /* sanity check */
> > -             if(list_xfered + file_total <= list_total) {
> > -                     totaldownload = 1;
> > -             } else {
> > -                     /* bogus values : don't enable totaldownload and reset */
> > -                     list_xfered = 0;
> > -                     list_total = 0;
> > -             }
> > +/* Cursor position relative to the first active progress bar,
> > + * e.g. 0 means the first active progress bar, active_bars_no-1 means the last bar,
> > + * active_bars_no - is the line below all progress bars.
> > + */
> > +int cursor_lineno = 0;
> > +
>
> And the number of global variables is on the rise!  Would it make sense
> to stick these in a single struct?

This change adds 4 static variables:

alpm_list_t *pacman_active_downloads; // list of datastructures for
active downloads/UI bars
size_t pacman_active_downloads_num; // number of active downloads/UI bars
int cursor_lineno; // current cursor position, it used to minimize
cursor movement up and down
int multibar_move_completed_up; // a configuration option that says
whether completed download bars should be moved to the top thus
keeping all active downloads and the bottom of the screen

First 2 vars are tightly coupled. 3 and 4th represent the UI-specific
datastructs.

multibar_move_completed_up needs to be accessed from other parts of
pacman codebase thus it needs to be defined in a header file. Thus it
cannot be part of a struct unless we want to move that struct to
header file as well (but I do not like this idea).

So what I can do it is to create a "struct pacman_multibar_ui" (naming
ideas are welcome) with following fields:

struct pacman_multibar_ui {
  alpm_list_t *downloads; // list of *active* downloads, once the
download is completed it is removed from this list
  size_t downloads_num;
  int cursor_lineno;
}
and then create a static variable of this type.

> > +/* Move console cursor `lines` up */
> > +static void cursor_line_up(unsigned int lines)
> > +{
> > +     assert(lines > 0);
> > +     printf("\x1B[%dF", lines);
> > +}
> > +
>
> OK.
>
> > +/* Move console cursor `lines` down */
> > +static void cursor_line_down(unsigned int lines)
> > +{
> > +     assert(lines > 0);
> > +     printf("\x1B[%dE", lines);
> > +}
> > +
>
> OK
>
> > +/* Erase line from the cursor position till the end of the line */
> > +static void console_erase_line(void)
> > +{
> > +     printf("\x1B[K");
> > +}

I wonder if these cursor_line_* functions should be moved to
src/pacman/util.c instead?

> OK.
>
> > +
> > +/* Goto the line that corresponds to num-th active download */
> > +static void cursor_goto_bar(int num)
> > +{
> > +     if(num > cursor_lineno) {
> > +             cursor_line_down(num - cursor_lineno);
> > +     } else if(num < cursor_lineno) {
> > +             cursor_line_up(cursor_lineno - num);
> >       }
> > +     cursor_lineno = num;
> > +}
>
> OK.
>
> >
> > -     if(totaldownload) {
> > -             xfered = list_xfered + file_xfered;
> > -             total = list_total;
> > -     } else {
> > -             xfered = file_xfered;
> > -             total = file_total;
> > -     }
> > -
> > -     /* this is basically a switch on xfered: 0, total, and
> > -      * anything else */
> > -     if(file_xfered == 0 && file_total == -1) {
> > -             /* set default starting values, ensure we only call this once
> > -              * if TotalDownload is enabled */
> > -             if(!totaldownload || (totaldownload && list_xfered == 0)) {
> > -                     initial_time = get_time_ms();
> > -                     xfered_last = (off_t)0;
> > -                     rate_last = 0.0;
> > -                     get_update_timediff(1);
> > -             }
> > -     } else if(xfered > total || xfered < 0) {
> > -             /* bogus values : stop here */
> > -             return;
> > -     } else if(file_xfered == file_total) {
> > -             /* compute final values */
> > -             int64_t timediff = get_time_ms() - initial_time;
> > -             if(timediff > 0) {
> > -                     rate = (double)xfered / (timediff / 1000.0);
> > -                     /* round elapsed time (in ms) to the nearest second */
> > -                     eta_s = (unsigned int)(timediff + 500) / 1000;
> > -             } else {
> > -                     eta_s = 0;
> > -             }
> > -     } else {
> > -             /* compute current average values */
> > -             int64_t timediff = get_update_timediff(0);
> > +/* Goto the line *after* the last active progress bar */
> > +static void cursor_goto_end(void)
> > +{
> > +     cursor_goto_bar(active_bars_no);
> > +}
>
> OK.
>
> /aside: This patch is really awful...  why did this block appear in the
> middle of a deletion!

It seems git formatter gets confused by a large scattered change like this one.

Github's split view makes it a little bit more readable.
https://github.com/anatol/pacman/commit/a88120fb8de9f750ccc9326c337d653fadcdaf08?diff=split

>
> >
> > -             if(timediff < UPDATE_SPEED_MS) {
> > -                     /* return if the calling interval was too short */
> > -                     return;
> > -             }
> > -             rate = (double)(xfered - xfered_last) / (timediff / 1000.0);
> > -             /* average rate to reduce jumpiness */
> > -             rate = (rate + 2 * rate_last) / 3;
> > -             if(rate > 0.0) {
> > -                     eta_s = (total - xfered) / rate;
> > -             } else {
> > -                     eta_s = UINT_MAX;
> > +/* Returns true if element with the specified name is found, false otherwise */
> > +static bool find_bar_for_filename(const char *filename, int *index, struct dl_progress_bar **bar)
> > +{
> > +     int i = 0;
> > +     alpm_list_t *listitem = active_dls;
> > +     for(; listitem; listitem = listitem->next, i++) {
> > +             struct dl_progress_bar *b = listitem->data;
> > +             if (b->filename == filename /* or strcmp() ? */) {
>
> Hrm...  I thought why is this not strcmp then saw your comment...  I see
> why pointer comparison works, but this needs well documented to not
> become broken if not using strcmp.

Initially I was worrying that having too many string comparisons might
slow down the code. Remember this code runs for each CURL progress
event and we scan through the list of active downloads here. So I was
thinking of using some identifier with a quick comparison function.
Currently the filename field is unique per payload structure and stays
constant. That is why the current implementation works.

I can convert to strcmp() to make the code resistant to the future
changes of the "filename" field lifecycle. In the future we might
think of adding an integer "id" field that will uniquely identify an
active download.


>
> > +                     *index = i;
> > +                     *bar = b;
> > +                     return true;
> >               }
> > -             rate_last = rate;
> > -             xfered_last = xfered;
> >       }
> >
> > -     if(file_total) {
> > -             file_percent = (file_xfered * 100) / file_total;
> > -     } else {
> > -             file_percent = 100;
> > -     }
> > +     return false;
> > +}
> >
> > -     if(totaldownload) {
> > -             total_percent = ((list_xfered + file_xfered) * 100) /
> > -                     list_total;
> > +static void draw_dl_progress_bar(struct dl_progress_bar *bar)
> > +{
> > +     int infolen;
> > +     int filenamelen;
> > +     char *fname, *p;
> > +     /* used for wide character width determination and printing */
> > +     int len, wclen, wcwid, padwid;
> > +     wchar_t *wcfname;
> > +     unsigned int eta_h = 0, eta_m = 0, eta_s = bar->eta;
> > +     double rate_human, xfered_human;
> > +     const char *rate_label, *xfered_label;
> > +     int file_percent = 0;
> >
> > -             /* if we are at the end, add the completed file to list_xfered */
> > -             if(file_xfered == file_total) {
> > -                     list_xfered += file_total;
> > -             }
> > +     const unsigned short cols = getcols();
> > +
> > +     if(bar->total_size) {
> > +             file_percent = (bar->xfered * 100) / bar->total_size;
> > +     } else {
> > +             file_percent = 100;
> >       }
> >
> >       /* fix up time for display */
> > @@ -773,13 +769,13 @@ static void dload_progress_event(const char *filename, off_t file_xfered, off_t
> >       eta_m = eta_s / 60;
> >       eta_s -= eta_m * 60;
> >
> > -     len = strlen(filename);
> > +     len = strlen(bar->filename);
> >       fname = malloc(len + 1);
> > -     memcpy(fname, filename, len + 1);
> > +     memcpy(fname, bar->filename, len + 1);
> >       /* strip package or DB extension for cleaner look */
> >       if((p = strstr(fname, ".pkg")) || (p = strstr(fname, ".db")) || (p = strstr(fname, ".files"))) {
> >               /* tack on a .sig suffix for signatures */
> > -             if(memcmp(&filename[len - 4], ".sig", 4) == 0) {
> > +             if(memcmp(&bar->filename[len - 4], ".sig", 4) == 0) {
> >                       memcpy(p, ".sig", 4);
>
> Hrm - if we are not showing bars for signatures, this can be removed.
>
> >
> >                       /* adjust length for later calculations */
> > @@ -790,6 +786,11 @@ static void dload_progress_event(const char *filename, off_t file_xfered, off_t
> >               fname[len] = '\0';
> >       }
> >
> > +     infolen = cols * 6 / 10;
> > +     if(infolen < 50) {
> > +             infolen = 50;
> > +     }
> > +
> >       /* 1 space + filenamelen + 1 space + 6 for size + 1 space + 3 for label +
> >        * + 2 spaces + 4 for rate + 1 space + 3 for label + 2 for /s + 1 space +
> >        * 8 for eta, gives us the magic 33 */
> > @@ -824,8 +825,8 @@ static void dload_progress_event(const char *filename, off_t file_xfered, off_t
> >
> >       }
> >
> > -     rate_human = humanize_size((off_t)rate, '\0', -1, &rate_label);
> > -     xfered_human = humanize_size(xfered, '\0', -1, &xfered_label);
> > +     rate_human = humanize_size((off_t)bar->rate, '\0', -1, &rate_label);
> > +     xfered_human = humanize_size(bar->xfered, '\0', -1, &xfered_label);
> >
> >       printf(" %ls%-*s ", wcfname, padwid, "");
> >       /* We will show 1.62 MiB/s, 11.6 MiB/s, but 116 KiB/s and 1116 KiB/s */
>
> OK.
>
> > @@ -850,19 +851,169 @@ static void dload_progress_event(const char *filename, off_t file_xfered, off_t
> >       free(fname);
> >       free(wcfname);
> >
> > -     if(totaldownload) {
> > -             fill_progress(file_percent, total_percent, cols - infolen);
> > +     fill_progress(file_percent, file_percent, cols - infolen);
> > +     return;
> > +}
> > +
> > +static void dload_init_event(const char *filename, alpm_download_event_init_t *data)
> > +{
> > +     (void)data;
> > +
> > +     if(!dload_progressbar_enabled()) {
> > +             printf(_(" %s downloading...\n"), filename);
> > +             return;
> > +     }
> > +
> > +     struct dl_progress_bar *bar = calloc(1, sizeof(struct dl_progress_bar));
> > +     assert(bar);
> > +     bar->filename = filename;
> > +     bar->init_time = get_time_ms();
> > +     bar->rate = 0.0;
> > +     active_dls = alpm_list_add(active_dls, bar);
> > +
> > +     cursor_goto_end();
> > +     printf(_(" %s downloading...\n"), filename);
> > +     cursor_lineno++;
> > +     active_bars_no++;
> > +}
>
> OK.
>
> > +
> > +/* Draws download progress */
> > +static void dload_progress_event(const char *filename, alpm_download_event_progress_t *data)
> > +{
> > +     int index;
> > +     struct dl_progress_bar *bar;
> > +     int64_t curr_time = get_time_ms();
> > +     double last_chunk_rate;
> > +     int64_t timediff;
> > +
> > +     if(!dload_progressbar_enabled()) {
> > +             return;
> > +     }
> > +
> > +     assert(find_bar_for_filename(filename, &index, &bar));
> > +
> > +     /* compute current average values */
> > +     timediff = curr_time - bar->sync_time;
> > +
> > +     if(timediff < UPDATE_SPEED_MS) {
> > +             /* return if the calling interval was too short */
> > +             return;
> > +     }
> > +     bar->sync_time = curr_time;
> > +
> > +     last_chunk_rate = (double)(data->downloaded - bar->xfered) / (timediff / 1000.0);
> > +     /* average rate to reduce jumpiness */
> > +     bar->rate = (last_chunk_rate + 2 * bar->rate) / 3;
>
> We may need to come back and adjust this later.  I have seen some weird
> wobble going on in speed output at times - perhaps when we saturate the
> download.
>
> > +     if(bar->rate > 0.0) {
> > +             bar->eta = (data->total - data->downloaded) / bar->rate;
> >       } else {
> > -             fill_progress(file_percent, file_percent, cols - infolen);
> > +             bar->eta = UINT_MAX;
> >       }
> > -     return;
> > +
> > +     /* Total size is received after the download starts. */
> > +     bar->total_size = data->total;
> > +     bar->xfered = data->downloaded;
> > +
> > +     cursor_goto_bar(index);
> > +     draw_dl_progress_bar(bar);
> > +     fflush(stdout);
> >  }
>
> OK.
>
> >
> > +/* download completed */
> > +static void dload_complete_event(const char *filename, alpm_download_event_completed_t *data)
> > +{
> > +     int index;
> > +     struct dl_progress_bar *bar;
> > +     int64_t timediff;
> > +
> > +     if(!dload_progressbar_enabled()) {
> > +             return;
> > +     }
> > +
> > +     assert(find_bar_for_filename(filename, &index, &bar));
> > +     bar->completed = true;
> > +
> > +     /* This may not have been initialized if the download finished before
> > +      * an alpm_download_event_progress_t event happened */
> > +     bar->total_size = data->total;
> > +
> > +     if(data->result == 1) {
> > +             cursor_goto_bar(index);
> > +             printf(_(" %s is up to date"), bar->filename);
> > +             /* The line contains text from previous status. Erase these leftovers. */
> > +             console_erase_line();
> > +     } else if(data->result == 0) {
> > +             /* compute final values */
> > +             bar->xfered = bar->total_size;
> > +             timediff = get_time_ms() - bar->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;
>
> I knew Australia speeds were bad!  It seems yu have super fast internet.
>
> > +
> > +             bar->rate = (double)bar->xfered / (timediff / 1000.0);
> > +             /* round elapsed time (in ms) to the nearest second */
> > +             bar->eta = (unsigned int)(timediff + 500) / 1000;
> > +
> > +             if(multibar_move_complete_top && index != 0) {
> > +                     /* If this item completed then move it to the top.
> > +                      * Swap 0-th bar data with `index`-th one
> > +                      */
> > +                     struct dl_progress_bar *former_topbar = active_dls->data;
> > +                     alpm_list_t *baritem = alpm_list_nth(active_dls, index);
> > +                     active_dls->data = bar;
> > +                     baritem->data = former_topbar;
> > +
> > +                     cursor_goto_bar(index);
> > +                     draw_dl_progress_bar(former_topbar);
> > +
> > +                     index = 0;
> > +             }
> > +
>
> Ok.
>
> > +             cursor_goto_bar(index);
> > +             draw_dl_progress_bar(bar);
> > +     } else {
> > +             cursor_goto_bar(index);
> > +             printf(_(" %s failed to download"), bar->filename);
> > +             console_erase_line();
> > +     }
> > +     fflush(stdout);
> > +
>
> OK.
>
> > +     /* If the first bar is completed then there is no reason to keep it
> > +      * in the list as we are not going to redraw it anymore.
> > +      */
> > +     while(active_dls) {
> > +             alpm_list_t *head = active_dls;
> > +             struct dl_progress_bar *j = head->data;
> > +             if(j->completed) {
> > +                     cursor_lineno--;
> > +                     active_bars_no--;
> > +                     active_dls = alpm_list_remove_item(active_dls, head);
> > +                     free(head);
> > +                     free(j);
> > +             } else {
> > +                     break;
> > +             }
> > +     }
> > +}
>
> OK
>
> > +
> > +/* Callback to handle display of download progress */
> >  void cb_download(const char *filename, alpm_download_event_type_t event, void *data)
> >  {
> > -     if(event == ALPM_DOWNLOAD_PROGRESS) {
> > -             alpm_download_event_progress_t *progress = data;
> > -             dload_progress_event(filename, progress->downloaded, progress->total);
> > +     if(str_endswith(filename, ".sig")) {
> > +             return;
> > +     }
>
> As I replied somewhere else in this patchset, "foo.sig" is a valid
> package name.
>
> > +
> > +     if(event == ALPM_DOWNLOAD_INIT) {
> > +             dload_init_event(filename, data);
> > +     } else if(event == ALPM_DOWNLOAD_PROGRESS) {
> > +             dload_progress_event(filename, data);
> > +     } else if(event == ALPM_DOWNLOAD_COMPLETED) {
> > +             dload_complete_event(filename, data);
> > +     } else {
> > +             pm_printf(ALPM_LOG_ERROR, _("unknown callback event type %d for %s\n"),
> > +                             event, filename);
> >       }
> >  }
> >
> > diff --git a/src/pacman/callback.h b/src/pacman/callback.h
> > index 6d92e86b..916b9dad 100644
> > --- a/src/pacman/callback.h
> > +++ b/src/pacman/callback.h
> > @@ -44,4 +44,7 @@ void cb_download(const char *filename, alpm_download_event_type_t event,
> >  __attribute__((format(printf, 2, 0)))
> >  void cb_log(alpm_loglevel_t level, const char *fmt, va_list args);
> >
> > +/* specify if multibar should move complete bars to the top of the screen */
> > +int multibar_move_complete_top;
> > +
>
> OK.
>
> >  #endif /* PM_CALLBACK_H */
> > diff --git a/src/pacman/sync.c b/src/pacman/sync.c
> > index f7dcb958..d5886f13 100644
> > --- a/src/pacman/sync.c
> > +++ b/src/pacman/sync.c
> > @@ -35,6 +35,7 @@
> >  #include "pacman.h"
> >  #include "util.h"
> >  #include "package.h"
> > +#include "callback.h"
> >  #include "conf.h"
> >
> >  static int unlink_verbose(const char *pathname, int ignore_missing)
> > @@ -824,6 +825,7 @@ int sync_prepare_execute(void)
> >               goto cleanup;
> >       }
> >
> > +     multibar_move_complete_top = 1;
> >       if(alpm_trans_commit(config->handle, &data) == -1) {
> >               alpm_errno_t err = alpm_errno(config->handle);
> >               pm_printf(ALPM_LOG_ERROR, _("failed to commit transaction (%s)\n"),
> >
>
> OK.


More information about the pacman-dev mailing list