diff mbox series

[pacman-dev,v2] Remove "total download" callback in favor of generic event callback

Message ID 20210308044841.3776-1-anatol.pomozov@gmail.com
State Superseded, archived
Headers show
Series [pacman-dev,v2] Remove "total download" callback in favor of generic event callback | expand

Commit Message

Anatol Pomozov March 8, 2021, 4:48 a.m. UTC 
Total download callback called right before packages start downloaded.
But we already have an event for such event (ALPM_EVENT_PKG_RETRIEVE_START)
and it is naturally to use the event to pass information about expected
download size.

Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
---
 README                |  1 -
 lib/libalpm/alpm.h    | 32 ++++++++++++--------------------
 lib/libalpm/dload.c   |  2 +-
 lib/libalpm/handle.c  | 13 -------------
 lib/libalpm/handle.h  |  1 -
 lib/libalpm/sync.c    | 25 +++++++++----------------
 src/pacman/callback.c |  9 ++-------
 src/pacman/callback.h |  2 --
 src/pacman/conf.c     |  4 ----
 9 files changed, 24 insertions(+), 65 deletions(-)

Comments

Andrew Gregory March 12, 2021, 4:15 a.m. UTC | #1 
On 03/07/21 at 08:48pm, Anatol Pomozov wrote:
> Total download callback called right before packages start downloaded.
> But we already have an event for such event (ALPM_EVENT_PKG_RETRIEVE_START)
> and it is naturally to use the event to pass information about expected
> download size.
> 
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> ---

WRT this specific patch, it looks like we should do the same for
packages downloaded with -U.  We weren't calling totaldcb for that
before and we won't have a size to pass, but we can still at least
pass the count.

At a higher level though, I would like to start providing front-ends
the full list of payload data.  The existing setup, and this patch,
just provide enough information for pacman's display.  I think
something like the following callback API would be ideal:

/* all_progress - list of all payloads in the transaction
 * changed      - payloads changed since the last cb call
 * ctx          - caller-defined context
 */
int dlprogresscb(alpm_list_t *all_progress, alpm_list_t *changed, void *ctx)

I think we should split the current payload structure into a download
structure and a progress structure.  The full list of download
structures can then be passed to the fetchcb (and event cb), allowing
it to behave much more intelligently than it can now.  The list given
to the progress callback would be the list of progress structures,
which would probably then have a pointer back to the underlying
download structure.
Anatol Pomozov March 15, 2021, 11:34 p.m. UTC | #2 
Hi

On Thu, Mar 11, 2021 at 8:15 PM Andrew Gregory
<andrew.gregory.8@gmail.com> wrote:
>
> On 03/07/21 at 08:48pm, Anatol Pomozov wrote:
> > Total download callback called right before packages start downloaded.
> > But we already have an event for such event (ALPM_EVENT_PKG_RETRIEVE_START)
> > and it is naturally to use the event to pass information about expected
> > download size.
> >
> > Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> > ---
>
> WRT this specific patch, it looks like we should do the same for
> packages downloaded with -U.  We weren't calling totaldcb for that
> before and we won't have a size to pass, but we can still at least
> pass the count.

Sure, it sounds reasonable to me. We already report this event and all
we need to do is to set the number of packages.

> At a higher level though, I would like to start providing front-ends
> the full list of payload data.  The existing setup, and this patch,
> just provide enough information for pacman's display.  I think
> something like the following callback API would be ideal:
>
> /* all_progress - list of all payloads in the transaction
>  * changed      - payloads changed since the last cb call
>  * ctx          - caller-defined context
>  */
> int dlprogresscb(alpm_list_t *all_progress, alpm_list_t *changed, void *ctx)
>
> I think we should split the current payload structure into a download
> structure and a progress structure.  The full list of download
> structures can then be passed to the fetchcb (and event cb), allowing
> it to behave much more intelligently than it can now.  The list given
> to the progress callback would be the list of progress structures,
> which would probably then have a pointer back to the underlying
> download structure.
diff mbox series

Patch

diff --git a/README b/README
index 6aa68374..470ccf3c 100644
--- a/README
+++ b/README
@@ -52,7 +52,6 @@  library is initialized.
 * logcb: The callback function for "log" operations.
 * dlcb: The callback function for download progress of each package.
 * fetchcb: Callback for custom download function.
-* totaldlcb: The callback function for overall download progress.
 * eventcb: Callback for transaction messages.
 * questioncb: Callback for selecting amongst choices.
 * progresscb: Callback to handle display of transaction progress.
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
index 0909fe11..833df829 100644
--- a/lib/libalpm/alpm.h
+++ b/lib/libalpm/alpm.h
@@ -926,6 +926,16 @@  typedef struct _alpm_event_hook_run_t {
 	size_t total;
 } alpm_event_hook_run_t;
 
+/** Packages downloading about to start. */
+typedef struct _alpm_event_pkg_retrieve_t {
+	/** Type of event */
+	alpm_event_type_t type;
+	/** Number of packages to download */
+	size_t num;
+	/** Total size of packages to download */
+	off_t total_size;
+} alpm_event_pkg_retrieve_t;
+
 /** Events.
  * This is a union passed to the callback that allows the frontend to know
  * which type of event was triggered (via type). It is then possible to
@@ -954,6 +964,8 @@  typedef union _alpm_event_t {
 	alpm_event_hook_t hook;
 	/** A hook was ran */
 	alpm_event_hook_run_t hook_run;
+	/** Download packages */
+	alpm_event_pkg_retrieve_t pkg_retrieve;
 } alpm_event_t;
 
 /** Event callback.
@@ -1197,12 +1209,6 @@  typedef void (*alpm_cb_download)(const char *filename,
 		alpm_download_event_type_t event, void *data);
 
 
-/** Total Download callback.
- * @param howmany the number of packages that will be downloaded during \link alpm_trans_commit \endlink.
- * @param total amount that will be downloaded during \link alpm_trans_commit \endlink.
- */
-typedef void (*alpm_cb_totaldl)(size_t howmany, off_t total);
-
 /** A callback for downloading files
  * @param url the URL of the file to be downloaded
  * @param localpath the directory to which the file should be downloaded
@@ -1525,20 +1531,6 @@  alpm_cb_fetch alpm_option_get_fetchcb(alpm_handle_t *handle);
  */
 int alpm_option_set_fetchcb(alpm_handle_t *handle, alpm_cb_fetch cb);
 
-/** Returns the callback used to report total download size.
- * @param handle the context handle
- * @return the currently set total download callback
- */
-alpm_cb_totaldl alpm_option_get_totaldlcb(alpm_handle_t *handle);
-
-/** Sets the callback used to report total download size.
- * @param handle the context handle
- * @param cb the cb to use
- * @return 0 on success, -1 on error (pm_errno is set accordingly)
- */
-int alpm_option_set_totaldlcb(alpm_handle_t *handle, alpm_cb_totaldl cb);
-
-
 /** Returns the callback used for events.
  * @param handle the context handle
  * @return the currently set event callback
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
index 0d0936c7..0423a139 100644
--- a/lib/libalpm/dload.c
+++ b/lib/libalpm/dload.c
@@ -851,7 +851,7 @@  int SYMEXPORT alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
 	const char *cachedir;
 	alpm_list_t *payloads = NULL;
 	const alpm_list_t *i;
-	alpm_event_t event;
+	alpm_event_t event = {0};
 
 	CHECK_HANDLE(handle, return -1);
 	ASSERT(*fetched == NULL, RET_ERR(handle, ALPM_ERR_WRONG_ARGS, -1));
diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c
index d1ea5886..7b8cb1da 100644
--- a/lib/libalpm/handle.c
+++ b/lib/libalpm/handle.c
@@ -174,12 +174,6 @@  alpm_cb_fetch SYMEXPORT alpm_option_get_fetchcb(alpm_handle_t *handle)
 	return handle->fetchcb;
 }
 
-alpm_cb_totaldl SYMEXPORT alpm_option_get_totaldlcb(alpm_handle_t *handle)
-{
-	CHECK_HANDLE(handle, return NULL);
-	return handle->totaldlcb;
-}
-
 alpm_cb_event SYMEXPORT alpm_option_get_eventcb(alpm_handle_t *handle)
 {
 	CHECK_HANDLE(handle, return NULL);
@@ -327,13 +321,6 @@  int SYMEXPORT alpm_option_set_fetchcb(alpm_handle_t *handle, alpm_cb_fetch cb)
 	return 0;
 }
 
-int SYMEXPORT alpm_option_set_totaldlcb(alpm_handle_t *handle, alpm_cb_totaldl cb)
-{
-	CHECK_HANDLE(handle, return -1);
-	handle->totaldlcb = cb;
-	return 0;
-}
-
 int SYMEXPORT alpm_option_set_eventcb(alpm_handle_t *handle, alpm_cb_event cb)
 {
 	CHECK_HANDLE(handle, return -1);
diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h
index 349ef2c7..2ebaafb2 100644
--- a/lib/libalpm/handle.h
+++ b/lib/libalpm/handle.h
@@ -72,7 +72,6 @@  struct __alpm_handle_t {
 	/* callback functions */
 	alpm_cb_log logcb;          /* Log callback function */
 	alpm_cb_download dlcb;      /* Download callback function */
-	alpm_cb_totaldl totaldlcb;  /* Total download callback function */
 	alpm_cb_fetch fetchcb;      /* Download file callback function */
 	alpm_cb_event eventcb;
 	alpm_cb_question questioncb;
diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
index d258b8d1..7fa50a13 100644
--- a/lib/libalpm/sync.c
+++ b/lib/libalpm/sync.c
@@ -749,7 +749,7 @@  static int download_files(alpm_handle_t *handle)
 	const char *cachedir;
 	alpm_list_t *i, *files = NULL;
 	int ret = 0;
-	alpm_event_t event;
+	alpm_event_t event = {0};
 	alpm_list_t *payloads = NULL;
 
 	cachedir = _alpm_filecache_setup(handle);
@@ -760,21 +760,6 @@  static int download_files(alpm_handle_t *handle)
 		goto finish;
 	}
 
-	/* Total progress - figure out the total download size if required to
-	 * pass to the callback. This function is called once, and it is up to the
-	 * frontend to compute incremental progress. */
-	if(handle->totaldlcb) {
-		off_t total_size = (off_t)0;
-		size_t howmany = 0;
-		/* sum up the download size for each package and store total */
-		for(i = files; i; i = i->next) {
-			alpm_pkg_t *spkg = i->data;
-			total_size += spkg->download_size;
-			howmany++;
-		}
-		handle->totaldlcb(howmany, total_size);
-	}
-
 	if(files) {
 		/* check for necessary disk space for download */
 		if(handle->checkspace) {
@@ -800,6 +785,14 @@  static int download_files(alpm_handle_t *handle)
 		}
 
 		event.type = ALPM_EVENT_PKG_RETRIEVE_START;
+
+		/* sum up the number of packages to download and its total size */
+		for(i = files; i; i = i->next) {
+			alpm_pkg_t *spkg = i->data;
+			event.pkg_retrieve.total_size += spkg->download_size;
+			event.pkg_retrieve.num++;
+		}
+
 		EVENT(handle, &event);
 		for(i = files; i; i = i->next) {
 			alpm_pkg_t *pkg = i->data;
diff --git a/src/pacman/callback.c b/src/pacman/callback.c
index 40973340..a28a79a9 100644
--- a/src/pacman/callback.c
+++ b/src/pacman/callback.c
@@ -338,6 +338,8 @@  void cb_event(alpm_event_t *event)
 		case ALPM_EVENT_PKG_RETRIEVE_START:
 			colon_printf(_("Retrieving packages...\n"));
 			on_progress = 1;
+			list_total_pkgs = event->pkg_retrieve.num;
+			list_total = event->pkg_retrieve.total_size;
 			total_enabled = config->totaldownload && list_total;
 			if(total_enabled) {
 				init_total_progressbar();
@@ -696,13 +698,6 @@  void cb_progress(alpm_progress_t event, const char *pkgname, int percent,
 	}
 }
 
-/* callback to handle receipt of total download value */
-void cb_dl_total(size_t howmany, off_t total)
-{
-	list_total_pkgs = howmany;
-	list_total = total;
-}
-
 static int dload_progressbar_enabled(void)
 {
 	return !config->noprogressbar && (getcols() != 0);
diff --git a/src/pacman/callback.h b/src/pacman/callback.h
index 9b5deb6f..8ac9d960 100644
--- a/src/pacman/callback.h
+++ b/src/pacman/callback.h
@@ -35,8 +35,6 @@  void cb_question(alpm_question_t *question);
 void cb_progress(alpm_progress_t event, const char *pkgname, int percent,
                    size_t howmany, size_t remain);
 
-/* callback to handle receipt of total download value */
-void cb_dl_total(size_t howmany, off_t total);
 /* callback to handle display of download progress */
 void cb_download(const char *filename, alpm_download_event_type_t event,
 		void *data);
diff --git a/src/pacman/conf.c b/src/pacman/conf.c
index 96e5790f..a4f2ba35 100644
--- a/src/pacman/conf.c
+++ b/src/pacman/conf.c
@@ -892,10 +892,6 @@  static int setup_libalpm(void)
 		pm_printf(ALPM_LOG_WARNING, _("no '%s' configured\n"), "XferCommand");
 	}
 
-	if(config->totaldownload) {
-		alpm_option_set_totaldlcb(handle, cb_dl_total);
-	}
-
 	alpm_option_set_arch(handle, config->arch);
 	alpm_option_set_checkspace(handle, config->checkspace);
 	alpm_option_set_usesyslog(handle, config->usesyslog);