GNOME Bugzilla – Bug 647885
Implement basic gpg key selector
Last modified: 2019-02-22 11:59:12 UTC
Now that basic selector support has been merged, we should implement a gpg key selector.
Created attachment 186132 [details] [review] Implementation of GcrGnupgCollection Also: http://git.collabora.co.uk/gitweb/?p=user/stefw/gnome-keyring.git;a=shortlog;h=refs/heads/gnupg-selector
Created attachment 186233 [details] [review] Add some more comments. Add on to previous patch, same git repository and branch as above.
Review of attachment 186132 [details] [review]: ::: configure.in @@ +363,3 @@ + +AC_PATH_PROGS(GNUPG, [gpg gpg2], "gpg") +AC_DEFINE_UNQUOTED(GPG_EXECUTABLE, "$GNUPG", [Path to gpg executable.]) I think “GNUPG” and “"gpg"” should be m4-quoted. Same for “GPG_EXECUTABLE” and “"$GNUPG"”. ::: gcr/gcr-colons.c @@ +52,3 @@ + for (;;) { + if (result->n_columns >= MAX_COLUMNS) { + _gcr_colons_free (result); Perhaps a warning should be printed in this case? @@ +116,3 @@ + + if (column >= colons->n_columns) + return NULL; Print a warning in this case too? @@ +122,3 @@ + +void +_gcr_colons_free (gpointer colons) Why is this a gpointer and not a GcrColons*? ::: gcr/gcr-gnupg-collection.c @@ +43,3 @@ + +struct _GcrGnupgCollectionPrivate { + GHashTable *items; Might be useful to add a comment giving the types of the keys and values in the table. @@ +99,3 @@ + } +} +static void Missing new line. @@ +165,3 @@ + +GcrCollection* +_gcr_gnupg_collection_new (const gchar *directory) A gtk-doc comment would be useful here, explaining what the directory is, what values are typically used, and why I should or shouldn't want to pass it. @@ +180,3 @@ + GString *out_data; + GString *err_data; + GHashTable *difference; Similarly, comments on the types of the items in ->dataset and ->difference here would be useful. @@ +184,3 @@ + +static void +_gcr_gnupg_collection_load_free (gpointer data) Why gpointer rather than GcrGnupgCollectionLoad*? @@ +216,3 @@ + + dataset = load->dataset; + load->dataset = g_ptr_array_new_with_free_func (_gcr_colons_free); What's the purpose of creating a new pointer array here? @@ +357,3 @@ + + /* Remove any keys that we still have in the difference */ + g_hash_table_foreach (load->difference, on_each_difference_remove, load->collection); Might be clearer (and faster) to use a GHashTableIter inline instead of using a callback. @@ +404,3 @@ +void +_gcr_gnupg_collection_load_async (GcrGnupgCollection *self, GCancellable *cancellable, + GAsyncReadyCallback callback, gpointer user_data) gtk-doc comment missing here. @@ +414,3 @@ + + /* Not yet implemented */ + g_return_if_fail (cancellable == NULL); I'm not sure there's any need for this. Client code can implement cancellation if it wants and the libgcr code need not stop it. The libgcr code will just currently not respond to cancellation. @@ +420,3 @@ + g_ptr_array_add (argv, "--list-keys"); + g_ptr_array_add (argv, "--fixed-list-mode"); + g_ptr_array_add (argv, "--with-colons"); gcc's throwing warnings at me for these lines. It unfortunately wants the second parameters to be cast to gpointer. :-( @@ +437,3 @@ + load->collection = g_object_ref (self); + g_hash_table_foreach (self->pv->items, on_each_item_add_keyid_to_difference, + load->difference); Same here: GHashTableIter might be clearer. @@ +448,3 @@ + &spawn_callbacks, + g_object_ref (res), + NULL, &error); Shouldn't g_object_unref be passed as the destroy function for res? @@ +452,3 @@ + if (error) { + g_simple_async_result_set_from_error (res, error); + g_simple_async_result_complete_in_idle (res); error needs to be freed. @@ +461,3 @@ + } + + g_object_unref (res); argv never gets freed. @@ +466,3 @@ +gboolean +_gcr_gnupg_collection_load_finish (GcrGnupgCollection *self, GAsyncResult *result, + GError **error) gtk-doc comment missing here. ::: gcr/gcr-gnupg-key.c @@ +104,3 @@ + if (self->pv->dataset) + g_ptr_array_free (self->pv->dataset, TRUE); + self->pv->dataset = NULL; finalize() is only ever called once, so there's no need to set dataset to NULL. @@ +118,3 @@ + case PROP_DATASET: + g_return_if_fail (!self->pv->dataset); + self->pv->dataset = g_value_dup_boxed (value); Should this use _gcr_gnupg_key_set_dataset() for consistency? @@ +168,3 @@ + g_object_class_install_property (gobject_class, PROP_DATASET, + g_param_spec_boxed ("dataset", "Dataset", "Colon Dataset", + G_TYPE_PTR_ARRAY, G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY)); If this is CONSTRUCT_ONLY, why does _gcr_gnupg_key_set_dataset() exist publicly (I presume it's public, anyway; ignore me if it's not). @@ +188,3 @@ + +GcrGnupgKey* +_gcr_gnupg_key_new (GPtrArray *dataset) gtk-doc comments would be nice for GcrGnupgKey would be nice too, but I think adding them to GcrGnupgCollection is more important. @@ +190,3 @@ +_gcr_gnupg_key_new (GPtrArray *dataset) +{ + return g_object_new (GCR_TYPE_GNUPG_KEY, "dataset", dataset, NULL); dataset isn't allowed to be NULL in _gcr_gnupg_key_set_dataset(). Should the same be true here? @@ +239,3 @@ +{ + static GcrColumn columns[] = { + { "label", G_TYPE_STRING, G_TYPE_STRING, N_("Name"), The string “Name” might want a context or translator comment to disambiguate it from all the other types of name in gnome-keyring. ::: gcr/gcr-selector.c @@ +381,3 @@ + if (self->pv->sort) + g_object_unref (self->pv->sort); + self->pv->sort = NULL; Does this belong in this patch set?
Review of attachment 186233 [details] [review]: Yay! Comments! :-D ::: gcr/gcr-util.c @@ +33,3 @@ + * Neither are sent. + */ + Might be more future-proof to make this a full-blown gtk-doc comment.
Created attachment 186257 [details] [review] Updated after pwithnall's review Awesome. Thanks! Made most of the changes, with the exceptions, comments below. (In reply to comment #3) > @@ +122,3 @@ > + > +void > +_gcr_colons_free (gpointer colons) > > Why is this a gpointer and not a GcrColons*? So that it matches the signature of GDestroyNotify. > @@ +184,3 @@ > + > +static void > +_gcr_gnupg_collection_load_free (gpointer data) > > Why gpointer rather than GcrGnupgCollectionLoad*? Again because of use as GDestroyNotify > @@ +216,3 @@ > + > + dataset = load->dataset; > + load->dataset = g_ptr_array_new_with_free_func (_gcr_colons_free); > > What's the purpose of creating a new pointer array here? A set of lines (each is a GcrColons) becomes part of a key. The pending lines sit in load->dataset until we've got all the ones that are part of that key. There's now a comment in gcr-colons.h that describes the output format. > @@ +448,3 @@ > + &spawn_callbacks, > + g_object_ref (res), > + NULL, &error); > > Shouldn't g_object_unref be passed as the destroy function for res? Oddly enough the g_object_unref is passed as part of the spawn_callbacks. > @@ +239,3 @@ > +{ > + static GcrColumn columns[] = { > + { "label", G_TYPE_STRING, G_TYPE_STRING, N_("Name"), > > The string “Name” might want a context or translator comment to disambiguate it > from all the other types of name in gnome-keyring. Added 'column' context, and put another commit (on master) that will use the appropriate context when running gettext on this string before use. > ::: gcr/gcr-selector.c > @@ +381,3 @@ > + if (self->pv->sort) > + g_object_unref (self->pv->sort); > + self->pv->sort = NULL; > > Does this belong in this patch set? Broke out into its own commit.
Merged with master. Thanks!