[packagekit] [PATCH] intial 'box' support added

[Richard Hughes]

On 31/08/2007, Grzegorz Dąbrowski <gdx at o2.pl> wrote:
> I wrote initial support for box package management used in Pingwinek
> GNU/Linux. Currently only searching is supported with filters (except
> gui/text). Is uses libbox library[1] and box tool[2].

Quality! I notice it's a C binding, maybe it can share some of the
common code with the apt backend?

Some comments about the patch:

+	actions = pk_task_action_build (PK_TASK_ACTION_INSTALL,
+				        PK_TASK_ACTION_REMOVE,
+				        PK_TASK_ACTION_UPDATE,
+				        PK_TASK_ACTION_GET_UPDATES,
+				        PK_TASK_ACTION_REFRESH_CACHE,
+				        PK_TASK_ACTION_UPDATE_SYSTEM,
+				        PK_TASK_ACTION_SEARCH_NAME,
+				        PK_TASK_ACTION_SEARCH_DETAILS,
+				        PK_TASK_ACTION_SEARCH_GROUP,
+				        PK_TASK_ACTION_SEARCH_FILE,
+				        PK_TASK_ACTION_GET_DEPS,
+				        PK_TASK_ACTION_GET_DESCRIPTION,
+				        0);

Do you really support all those actions? The idea of GetActions is to
hide in the UI what you don't support. I think you just want
PK_TASK_ACTION_SEARCH_NAME and PK_TASK_ACTION_SEARCH_DETAILS right?

+	pk_task_change_job_status (task, PK_TASK_STATUS_QUERY);
+
+    db = box_db_open("/");
+    box_db_attach_repo(db, "/", "core");
+    box_db_repos_init(db);

You need to use tabs, not spaces. There's quite a lot of this in the
patch. Please set the tab spacing to 8 chars. Minor thing, but it
really annoys me, sorry. :-)

+static void
+parse_filter(const gchar *filter,  gboolean *installed,  gboolean
*available,  gboolean *devel,
+        gboolean *nondevel, gboolean *gui, gboolean *text)

I can add something more robust in pk-task-common if you wish - it's
okay for now tho.

+    gboolean nondevel;
+    gboolean gui;
+    gboolean text;
+
+	g_return_val_if_fail (task != NULL, FALSE);
+	g_return_val_if_fail (PK_IS_TASK (task), FALSE);

More spaces for tabs (search replace "    " with "\t" should fix the
majority...).

+        } else if (installed == TRUE)
+        {
+            list = box_db_repos_packages_search_installed(db, (gchar
*)search, devel_filter);
+        } else if (available == TRUE)
+        {

The braces are on the wrong lines:

} else if (foo == TRUE) {
    do_bar ();
} else if (baz == FALSE}
   do raz ();
}

Again, this falls into the style issue but I think it's important to
be consistent.

+/**
+ * pk_task_search_group:
+ **/
+gboolean
+pk_task_search_group (PkTask *task, const gchar *filter, const gchar *search)
+{
+    return find_packages (task, search, filter);
+}

That's not correct. You need to use pk_task_not_implemented_yet.

+	task->package = strdup (package);
+	pk_task_change_job_status (task, PK_TASK_STATUS_QUERY);
+	pk_task_package (task, 1, "glib2;2.14.0;i386;fedora",
+			 "The GLib library");
+	pk_task_package (task, 1, "gtk2;gtk2-2.11.6-6.fc8;i386;fedora",
+			 "GTK+ Libraries for GIMP");
+	pk_task_finished (task, PK_TASK_EXIT_SUCCESS);
....
+	pk_task_change_job_status (task, PK_TASK_STATUS_QUERY);
+	pk_task_description (task, "gnome-power-manager;2.6.19;i386;fedora",
PK_TASK_GROUP_PROGRAMMING,
+"Scribus is an desktop open source page layout program with "
+"the aim of producing commercial grade output in PDF and "
"http://live.gnome.org/GnomePowerManager");
+	pk_task_finished (task, PK_TASK_EXIT_SUCCESS);
.....
+	task->priv->progress_percentage = 0;
+	g_timeout_add (1000, pk_task_install_timeout, task);

You don't want to do this. This sort of thing belongs in the dummy
backend, just use pk_task_not_implemented_yet like before and keep the
box backend clean.

Also PackageKit is meant to be 100% asynchronous, which means
SearchName has to return immediately, and the we get a Finished signal
when it's completed. I might be being naive, but looking at this code:

+    list = box_db_repos_packages_for_upgrade(db);
+    for (li = list; li != NULL; li = li->next)
+    {
+        package = (PackageSearch*)li->data;
+        pk_task_package (task, 1, package->package, package->description);
+    }
+    box_db_repos_package_list_free(list);

...won't it block and make everything synchronous? If you have a look
in the apt backend we've solved this using a thread, which although
tricky, turns a sync API into an async API.

There's probably going to be some common code for the thread stuff
between apt and box, but we can worry about that later.

Sorry if it appears I've ripped your patch apart, but really it's very
good.  :-)

Thanks.

Richard.