GNOME Bugzilla – Bug 786123
GtkPlacesSidebar: Add support for libcloudproviders
Last modified: 2017-11-13 16:01:07 UTC
Created attachment 357357 [details] [review] [PATCH] gtkplacessidebar: implement libcloudproviders support This is a patch to allow cloud providers to add items to the GtkPlacesSidebar. The sidebar will show icon, name and sync status of the cloud providers. The exported menu is rendered as a GtkPopover. The sidebar will be updated if the list of cloudproviders changes e.g. by adding or removing an account. If any cloud provider changes detailed information like sync status only the individual sidebar row gets updated. Related nautilus bug: https://bugzilla.gnome.org/show_bug.cgi?id=731627 The source code of libcloudproviders is currently at this repo, but will be moved to the GNOME Gitlab: https://github.com/juliushaertl/libcloudproviders Here is also an example of how this looks like with the Nextcloud sync client implementing the cloud providers DBus API: https://github.com/nextcloud/gsoc_client/pull/12
Review of attachment 357357 [details] [review]: The dependency on cloudproviders needs to be conditional, otherwise we will have issues building on windows or os x. ::: gtk/gtkplacessidebar.c @@ +453,3 @@ const gchar *name, + GIcon *left_icon, + GIcon *right_icon, I'd prefer to use start/end terminology here @@ +912,3 @@ +static void +cloud_row_update(CloudProviderProxy *cloud_provider_proxy, + GtkWidget *cloud_row) Space before ( and please line up parameters @@ +935,3 @@ + } + + gtk_sidebar_row_set_right_icon(GTK_SIDEBAR_ROW(cloud_row), right_icon); Spaces before (, please. There's more places where this needs fixing @@ +1115,3 @@ + } + + tooltip = g_strdup_printf ("Open %s", name); Please add a translator comment like this, before this line: /* translators: %s is the name of a cloud provider for files */
(In reply to Matthias Clasen from comment #1) > @@ +1115,3 @@ > + } > + > + tooltip = g_strdup_printf ("Open %s", name); > > Please add a translator comment like this, before this line: > > /* translators: %s is the name of a cloud provider for files */ This string is not actually marked for translation.
(In reply to Piotr Drąg from comment #2) > > This string is not actually marked for translation. Lets fix that first!
Created attachment 358351 [details] [review] [PATCH] gtkplacessidebar: implement libcloudproviders support v2
Review of attachment 358351 [details] [review]: Some more cleanups needed. For forward-porting this to master, it will need meson support, but we can look at that when the time comes. ::: configure.ac @@ +1356,3 @@ + CLOUDPROVIDER_PACKAGES="" +fi + I would prefer to have an explicit --enable-cloudproviders option as well, and error out if it is requested but not found. ::: gtk/gtkplacessidebar.c @@ +999,3 @@ GIcon *new_bookmark_icon; +#ifdef HAVE_CLOUDPROVIDERS + GList *cloud_provider_proxys; plural should be: proxies ::: gtk/gtksidebarrow.c @@ +37,3 @@ + GIcon *right_icon; + GtkWidget *left_icon_widget; + GtkWidget *right_icon_widget; Can we please use start/end terminology all the way ? @@ +308,3 @@ + NULL); + + context = gtk_widget_get_style_context (GTK_WIDGET (self)); Misindentation here @@ +548,3 @@ + (G_PARAM_READWRITE | + G_PARAM_STATIC_STRINGS)); +#endif I think it is a bit iffy to have the api (ie the existence of a property) depend on configuration like this. One alternative I could think of is to make this a property of type GObject, and do a runtime check to ensure that it is of the right type. ::: gtk/gtksidebarrowprivate.h @@ +57,1 @@ start/end here as well, please
Created attachment 358371 [details] [review] [PATCH] gtkplacessidebar: implement libcloudproviders support v3 See the attached patch for the changes requested. > For forward-porting this to master, it will need meson support, but we can look at that when the time comes. Yes, I can add meson support for master as well.
Review of attachment 358371 [details] [review]: Looks fine now, if you answer the one question below. ::: gtk/Makefile.am @@ +1444,3 @@ libgtk_3_la_LIBADD = $(libadd) libgtk_3_la_DEPENDENCIES = $(deps) +libgtk_3_la_DEPENDENCIES += $(gtk_deps) is this necessary ? seems curious, in particular since I don't see gtk_deps getting set anywhere in my Makefile
> is this necessary ? seems curious, in particular since I don't see gtk_deps getting set anywhere in my Makefile You are right, this is not needed anymore. Build is also running fine without it.
Created attachment 358383 [details] [review] [PATCH] gtkplacessidebar: implement libcloudproviders support v4
Review of attachment 358383 [details] [review]: ok
Review of attachment 358383 [details] [review]: trying this out in practice yields lots of (lt-gtk3-demo:27549): Gtk-WARNING **: GtkBox does not have a child property called top_attach the sidebar row is a box, not a grid
Created attachment 358448 [details] [review] gtkplacessidebar: implement libcloudproviders support Add integration of the libcloudproviders DBus API to the GtkPlacesSidebar by showing name and sync status of the cloud providers. The exported menu is rendered as a GtkPopover. The sidebar will be updated if the list of cloudproviders changes e.g. by adding or removing an account. If any cloud provider changes detailed information like sync status only the individual sidebar row gets updated. Co-authored-by: Carlos Soriano <csoriano@gnome.org>
i fixed it up myself to get it merged
Leaving this bug open to deal with forward-porting the patch to master. Julius, can you do that ?
Review of attachment 358448 [details] [review]: Hey, I just found some things, it might be expected or an oversight. ::: gtk/gtkplacessidebar.c @@ +1431,2 @@ } + mount_uri = "network:///"; I guess this is an oversight right? This was removed long time ago, in favor of the Other Locations. ::: gtk/gtkplacessidebarprivate.h @@ +33,3 @@ SECTION_MOUNTS, + SECTION_CLOUD, + SECTION_DEVICES, why the creation of DEVICES and NETWORK?
thanks for catching these, carlos
> I guess this is an oversight right? This was removed long time ago, in favor of the Other Locations. Yes, those seem to be leftovers from the original poc patch. I'll create a patch to revert those. > Leaving this bug open to deal with forward-porting the patch to master. Julius, can you do that ? Yes, I'll do that after the weekend. I also found a bug with the build flag, it seems that the HAVE_CLOUDPROVIDERS is not available as a preprocessor variable in the gtksidebarrow.c file, in the gtkplacessidebar.c it works just fine. I currently don't have a clue why that is the case.
end_icon in update_places() is unused if not HAVE_CLOUDPROVIDERS, causing a compiler warning, which ideally would be suppressed if not applicable.
(In reply to Julius Härtl from comment #17) > I also found a bug with the build flag, it seems that the > HAVE_CLOUDPROVIDERS is not available as a preprocessor variable in the > gtksidebarrow.c file, in the gtkplacessidebar.c it works just fine. I > currently don't have a clue why that is the case. HAVE_CLOUDPROVIDERS is defined (or not) in config.h. gtkplacessidebar.c does #include "config.h" to get this. gtksidebarrow.c does not.
Created attachment 358511 [details] [review] SidebarRow: need config.h for HAVE_CLOUDPROVIDERS
Created attachment 358512 [details] [review] PlacesSidebar: Avoid warning about unused variable end_icon is only used if HAVE_CLOUDPROVIDERS is defined, so only declare it under the same condition.
Created attachment 358532 [details] [review] [PATCH] gtkplacessidebar: remove oversight of old code
Review of attachment 358532 [details] [review]: Looks good, thanks Julius!
Keep it open for master support with meson
Created attachment 358562 [details] [review] [PATCH 1/4] gtkplacessidebar: implement libcloudproviders support
Created attachment 358564 [details] [review] [PATCH 2/4] PlacesSidebar: Avoid warning about unused variable
Created attachment 358566 [details] [review] [PATCH 3/4] SidebarRow: need config.h for HAVE_CLOUDPROVIDERS
Created attachment 358567 [details] [review] [PATCH 4/4] gtkplacessidebar: remove oversight of old code
Review of attachment 358562 [details] [review]: ::: gtk/gtkplacessidebar.c @@ +3513,3 @@ + GMenu *menu = g_menu_new (); + GMenuItem *item; + item = g_menu_item_new ("_Open", "row.open"); Is this intentionally not translated? @@ +3514,3 @@ + GMenuItem *item; + item = g_menu_item_new ("_Open", "row.open"); + g_menu_item_set_action_and_target_value (item, "row.open", g_variant_new_int32(GTK_PLACES_OPEN_NORMAL)); It'd be nice to break such long lines, and the g_variant_new_int32 has a missing space before the parenthesis. @@ +3554,3 @@ + g_object_get (row, "cloud-provider", &cloud_provider_proxy, NULL); + + if (cloud_provider_proxy) { GTK+ style is that braces get their own lines and own level of indentation ::: gtk/gtksidebarrow.c @@ +533,3 @@ + properties [PROP_CLOUD_PROVIDER] = + g_param_spec_object ("cloud-provider", + "CloudProviderProxy", Obviously not your fault, but I just realised that none of the properties in this file are marked translatable. Is that intentional? ::: gtk/ui/gtksidebarrow.ui @@ +60,3 @@ </child> </template> + <!-- We need it to not make the row smaller when the eject button is hidden --> Timm previously removed this, saying it doesn't work at all. Did you intend to add it back, and does it actually achieve anything?
Maybe for master you can squash the patches into one; I don't mind losing credit for two one-liners. :P
yes, please
and the meson parts look fine to me, fwiw
These need to be marked for translation: + item = g_menu_item_new ("_Open", "row.open"); + item = g_menu_item_new ("Open in new tab", "row.open-other"); + item = g_menu_item_new ("Open in new window", "row.open-other"); and the latter two possibly changed to "Open in New _Tab" and "Open in New _Window".
Created attachment 358716 [details] [review] PlacesSidebar: Reuse strings, mark for translation -- (In reply to Piotr Drąg from comment #33) > and the latter two possibly changed to "Open in New _Tab" and "Open in New > _Window". Definitely, to match the strings for the buttons just a bit down from that.
Comment on attachment 358716 [details] [review] PlacesSidebar: Reuse strings, mark for translation Attachment 358716 [details] pushed as 9f1d57e - PlacesSidebar: Reuse strings, mark for translation
Review of attachment 358562 [details] [review]: ::: gtk/ui/gtksidebarrow.ui @@ +61,3 @@ </template> + <!-- We need it to not make the row smaller when the eject button is hidden --> + <object class="GtkSizeGroup"> This does nothing in the tests I can conjure up. Unless you found that it changed/improved anything, then there's no need to add it back. (As Timm explained, invisible widgets request no size, so this doesn't ensure the row still requests size for the hidden button.)
Created attachment 359028 [details] [review] [PATCH] gtkplacessidebar: adapt libcloudproviders api rename This patch will address the libcloudprovider rename changes for 3.22 I'll create a single patch containing all fixes for master, once this is in.
Created attachment 359030 [details] [review] [PATCH] gtkplacessidebar: implement libcloudproviders support (master)
(In reply to Daniel Boles from comment #36) > Review of attachment 358562 [details] [review] [review]: > > ::: gtk/ui/gtksidebarrow.ui > @@ +61,3 @@ > </template> > + <!-- We need it to not make the row smaller when the eject button is > hidden --> > + <object class="GtkSizeGroup"> > > This does nothing in the tests I can conjure up. Unless you found that it > changed/improved anything, then there's no need to add it back. > > (As Timm explained, invisible widgets request no size, so this doesn't > ensure the row still requests size for the hidden button.) I think we can still remove this. If someone *did* want to fix the issue it claimed to fix, so that space is always reserved for the eject button even when not shown, they could take inspiration from my fix at Bug 772348. However, I don't think that's useful hor SidebarRow. It was for PlacesViewRow, as that has 3 columns, so we needed to avoid offsetting the others to the left. However, the SidebarRow just has a label and an eject button, and I think forcing the labels of non-ejectable items to be truncated and have empty space next to them would be a regression.
Created attachment 362590 [details] [review] gtkplacessidebar: Adapt to libcloudproviders 0.2.0 And a few improvements on the way.
Created attachment 362591 [details] [review] gtkplacessidebar: Fix new tab/window handling for cloud accounts It wasn't taking into account whether the sidebar had support for them or not, resulting in a file chooser with open in new tab/window menu items when it's not supported. To fix it, do as with the other menus and check for the availability of new tab/window flags.
Julius, can you take a quick look to these?
Pushed the gtk+-3 branch. Let me know if you find something Julius (or anyone). Attachment 362590 [details] pushed as e672c02 - gtkplacessidebar: Adapt to libcloudproviders 0.2.0 Attachment 362591 [details] pushed as 7e49a02 - gtkplacessidebar: Fix new tab/window handling for cloud accounts
Cherry picked to master too.