GNOME Bugzilla – Bug 752941
Add generic GoaProvider::show_account() implementation
Last modified: 2017-05-29 18:18:39 UTC
This patch series adds a generic implementation of GoaProvider::show_account() and use it in the various providers rather than having duplicated code in each of them.
Created attachment 308257 [details] [review] owncloud: Add uri_to_string_with_path() helper This extracts a bit of repeated code out of GoaOwncloudProvider::build_object() and makes the high level code flow somewhat easier to follow.
Created attachment 308258 [details] [review] owncloud: Introduce get_webdav_uri() helper Move a bit more code out of GoaOwncloudProvider::build_object().
Created attachment 308259 [details] [review] provider: Unify default vfunc name The default vfunc implementations in GoaProvider are named _real except for goa_provider_get_provider_icon_default(). This commit changes the "_default" suffix to "_real" for better consistency.
Created attachment 308260 [details] [review] provider: Add default implementation for GoaProvider::show_account()
Created attachment 308261 [details] [review] provider: Sort show_account() entries depending on locale
Created attachment 308262 [details] [review] providers: Use default implementation of ::show_account() This commit removes quite a bit of duplicated code from the various providers now that we have a generic default GoaProvider::show_account() implementation.
*** Bug 696077 has been marked as a duplicate of this bug. ***
Review of attachment 308257 [details] [review]: Thanks for the nice clean-up. ::: src/goabackend/goaowncloudprovider.c @@ +88,3 @@ +uri_to_string_with_path (SoupURI *soup_uri, const gchar *path) +{ + gchar *uri; Nitpick: how about calling it uri_string? It is used elsewhere in the file for an URI expressed as a gchar *, while uri is also used for a SoupURI. @@ +95,3 @@ + + uri_tmp = soup_uri_to_string (soup_uri, FALSE); + uri= g_strconcat (uri_tmp, path, NULL); Missing a space before the '='.
Review of attachment 308258 [details] [review]: Looks good apart from one small detail: ::: src/goabackend/goaowncloudprovider.c @@ +86,2 @@ /* ---------------------------------------------------------------------------------------------------- */ + This new line should have been in the previous patch. I won't complain if you decide to squash them together due to this. ;)
Review of attachment 308259 [details] [review]: ++
Review of attachment 308260 [details] [review]: This exposes some mnemonic conflicts, but I am not too fussed about those because they are not used in conjunction at the moment. ::: src/goabackend/goaprovider-priv.h @@ +79,1 @@ void (*show_account) (GoaProvider *self, There is no need to preserve the order of the pointers because this header is decisively not part of the public API since commit 583b9d0f63b1a We should have also dropped the padding, and consolidated all the pure virtual and virtual with default paragraphs as part of that commit. ::: src/goabackend/goaprovider.c @@ +471,3 @@ + +static ShowAccountItem show_account_items[] = { + { How about using the same structure / style as 'ordered_builtins_map' further down in the file? @@ +495,3 @@ + .property = "documents-disabled", + .blurb = N_("_Documents"), + }, Thanks for keeping the ordering unchanged! @@ +556,3 @@ + item = show_account_items; + + while (item->feature != GOA_PROVIDER_FEATURE_INVALID) In that case, this would be a for loop using indices.
Review of attachment 308261 [details] [review]: I am not sure about this. Do we sort things like this elsewhere? For example, the entries in GtkPlacesSidebar are not sorted. Maybe there are some counter-examples too? Does the documentation team have an opinion about sorting UI controls like this? I can imagine a support call across multiple locales where the ability to say "click the second switch" can be useful.
Review of attachment 308262 [details] [review]: Looks mostly good, apart from two nitpicks: a) the telepathy-account-widgets hunk shouldn't be a part of this change, and ... ::: src/goabackend/goatelepathyprovider.c @@ +875,3 @@ G_GNUC_UNUSED GtkGrid *dummy) { + GoaProviderClass *parent_class = GOA_PROVIDER_CLASS (goa_telepathy_provider_parent_class); We use goa_telepathy_provider_parent_class directly with the cast elsewhere in the file.
Thanks for doing this, Christophe!
Review of attachment 308258 [details] [review]: ::: src/goabackend/goaowncloudprovider.c @@ +86,2 @@ /* ---------------------------------------------------------------------------------------------------- */ + Oh it's easy enough to move this to the right commit, thanks for spotting this :) Tweaking these 2 patches made me realize they probably were unrelated to this bug. Well, I had to attach them somewhere :)
Review of attachment 308261 [details] [review]: No strong feeling about that one, on the one hand, the unsorted list feels a bit weird to me, on the other hand, sorting things from probably most important to less important rather than alphabetically is probably more sensible. It's independant of the 5th patch, so I've moved this one last in the series, it can be pushed later if it needs more discussion.
Review of attachment 308262 [details] [review]: Ah, the telepathy-account-widgets is already gone, noticed it during a rebase.
Review of attachment 308260 [details] [review]: ::: src/goabackend/goaprovider.c @@ +471,3 @@ + +static ShowAccountItem show_account_items[] = { + { Do you mean an anonymous structure (see below) or something different? static struct { ... } show_account_items[] = { ... } I'd prefer to keep the .label = xxx; style for initialization though, as this makes things a bit more readable imo.
Review of attachment 308260 [details] [review]: ::: src/goabackend/goaprovider.c @@ +471,3 @@ + +static ShowAccountItem show_account_items[] = { + { Actually, not having a typedef would break the sorting patch as it puts some of these items in a list, sorts the list and then iterates over the list and casts the list elements to the right type. I'll keep the typedef for now, if we drop the sorting patch, I don't mind changing that one to what you suggested.
Review of attachment 308260 [details] [review]: ::: src/goabackend/goaprovider-priv.h @@ +79,1 @@ void (*show_account) (GoaProvider *self, Arguably, one could have some code compiled pre-583b9d (aka 3.11.4~2) which directly dereferences the function pointers rather than using the corresponding function wrapper, so moving these fields around is technically an ABI break. This is probably not a usecase we care about though?
Created attachment 308488 [details] [review] owncloud: Add uri_to_string_with_path() helper This extracts a bit of repeated code out of GoaOwncloudProvider::build_object() and makes the high level code flow somewhat easier to follow.
Created attachment 308489 [details] [review] owncloud: Introduce get_webdav_uri() helper Move a bit more code out of GoaOwncloudProvider::build_object().
Created attachment 308490 [details] [review] provider: Unify default vfunc name The default vfunc implementations in GoaProvider are named _real except for goa_provider_get_provider_icon_default(). This commit changes the "_default" suffix to "_real" for better consistency.
Created attachment 308491 [details] [review] provider: Add default implementation for GoaProvider::show_account()
Created attachment 308492 [details] [review] providers: Use default implementation of ::show_account() This commit removes quite a bit of duplicated code from the various providers now that we have a generic default GoaProvider::show_account() implementation.
Created attachment 308493 [details] [review] Reorder GoaProviderClass fields as it's private now We can group all pure virtual functions together, and drop the structure padding as GoaProvider is not publicly exposed anymore.
Created attachment 308494 [details] [review] Fix typo in comment in goaprovider.c There is an extra 'is' in the sentence.
Created attachment 308495 [details] [review] provider: Sort show_account() entries depending on locale
Review of attachment 308260 [details] [review]: ::: src/goabackend/goaprovider.c @@ +471,3 @@ + +static ShowAccountItem show_account_items[] = { + { On second thought, I can remove the typedef from this commit, and move it to the 'sorting' patch. If you prefer that approach, just let me know.
Review of attachment 308488 [details] [review]: ++
Review of attachment 308489 [details] [review]: ++
Review of attachment 308490 [details] [review]: ++
(In reply to Christophe Fergeau from comment #20) > Review of attachment 308260 [details] [review] [review]: > > ::: src/goabackend/goaprovider-priv.h > @@ +79,1 @@ > void (*show_account) (GoaProvider *self, > > Arguably, one could have some code compiled pre-583b9d (aka 3.11.4~2) which > directly dereferences the function pointers rather than using the > corresponding function wrapper, so moving these fields around is technically > an ABI break. This is probably not a usecase we care about though? Right, we don't care about that use case. The libgoabackend subset of the API has always been a semi-private / semi-privileged one that is only meant to be used from gnome-initial-setup or gnome-control-center because it lets you add / remove / modify an account. Other applications or libraries are meant to use the libgoa subset of the API that is a thin convenience wrapper around goa-daemon's D-Bus API. In a sandboxed / app bundle world we will only expose this later subset to the world.
(In reply to Christophe Fergeau from comment #18) > Review of attachment 308260 [details] [review] [review]: > > ::: src/goabackend/goaprovider.c > @@ +471,3 @@ > + > +static ShowAccountItem show_account_items[] = { > + { > > Do you mean an anonymous structure (see below) or something different? > > static struct > { > ... > } show_account_items[] = { > ... > } Yes, the anonymous structure. In the other instance where it is used in the file, I found it convenient to avoid inventing a decent name for an ad-hoc type. :) But I see the limitations that it imposes if we want to sort the list. So, let's leave it the way it is currently in your patch. We can either make it anonymous later, or invent a name for the other structure. > I'd prefer to keep the .label = xxx; style for initialization though, as > this makes things a bit more readable imo. I don't mind having those. Although, since designated initializers [1] are a C99 invention, I wonder what the general consensus is. A quick scan indicates that it might be an issue with g++, but we are not using it here. [1] https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
Review of attachment 308494 [details] [review]: Oops. Thanks for catching it.
Review of attachment 308491 [details] [review]: ::: src/goabackend/goaprovider.c @@ +465,3 @@ +typedef struct { + guint feature; Sorry for not asking this earlier. Shouldn't this be a GoaProviderFeatures instead of a guint? @@ +467,3 @@ + guint feature; + const gchar *property; + const gchar *blurb; Looks we have an extra space between 'gchar' and '*'. @@ +474,3 @@ + * important because it affects the order in which they are + * displayed in the show_account() UI + */ Thanks for adding the comment.
Review of attachment 308492 [details] [review]: ++
Review of attachment 308493 [details] [review]: ++
Review of attachment 308491 [details] [review]: ::: src/goabackend/goaprovider.c @@ +465,3 @@ +typedef struct { + guint feature; No idea :) I'll change it. @@ +467,3 @@ + guint feature; + const gchar *property; + const gchar *blurb; Ah, good catch, changed too
Created attachment 308575 [details] [review] provider: Add default implementation for GoaProvider::show_account()
Created attachment 308578 [details] [review] provider: Add default implementation for GoaProvider::show_account()
Created attachment 308579 [details] [review] provider: Sort show_account() entries depending on locale
Ok, here are 2 variations of the "Add default implementation for GoaProvider::show_account()" patch, first one is the same as the previous version with your comments addressed. The second variations implements the 'anonymous struct' suggestion that you made. I've attached an updated 'sorted patch' to go with the 'anonymous struct' variation.
Comment on attachment 308488 [details] [review] owncloud: Add uri_to_string_with_path() helper Pushed to master.
Comment on attachment 308489 [details] [review] owncloud: Introduce get_webdav_uri() helper Pushed to master.
Comment on attachment 308494 [details] [review] Fix typo in comment in goaprovider.c Pushed to master.
Review of attachment 308578 [details] [review]: Looks good to me.
Comment on attachment 308493 [details] [review] Reorder GoaProviderClass fields as it's private now Pushed to master.
Comment on attachment 308492 [details] [review] providers: Use default implementation of ::show_account() Pushed to master.
Review of attachment 308579 [details] [review]: I am still hesitant about this one. Sometimes people attach screenshots from different locales and a static ordering of the buttons makes it easier to identify them. Also comment 12. On the other hand, I am not strongly against it either. Leaving it as "reviewed" for the time being.