After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 786123 - GtkPlacesSidebar: Add support for libcloudproviders
GtkPlacesSidebar: Add support for libcloudproviders
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: Other
3.22.x
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2017-08-10 20:48 UTC by Julius Härtl
Modified: 2017-11-13 16:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH] gtkplacessidebar: implement libcloudproviders support (45.67 KB, patch)
2017-08-10 20:48 UTC, Julius Härtl
none Details | Review
[PATCH] gtkplacessidebar: implement libcloudproviders support v2 (46.62 KB, patch)
2017-08-24 14:52 UTC, Julius Härtl
none Details | Review
[PATCH] gtkplacessidebar: implement libcloudproviders support v3 (47.49 KB, patch)
2017-08-24 17:46 UTC, Julius Härtl
none Details | Review
[PATCH] gtkplacessidebar: implement libcloudproviders support v4 (47.03 KB, patch)
2017-08-25 07:11 UTC, Julius Härtl
needs-work Details | Review
gtkplacessidebar: implement libcloudproviders support (46.52 KB, patch)
2017-08-25 20:57 UTC, Matthias Clasen
committed Details | Review
SidebarRow: need config.h for HAVE_CLOUDPROVIDERS (911 bytes, patch)
2017-08-27 14:37 UTC, Daniel Boles
committed Details | Review
PlacesSidebar: Avoid warning about unused variable (999 bytes, patch)
2017-08-27 14:37 UTC, Daniel Boles
committed Details | Review
[PATCH] gtkplacessidebar: remove oversight of old code (5.18 KB, patch)
2017-08-27 20:48 UTC, Julius Härtl
committed Details | Review
[PATCH 1/4] gtkplacessidebar: implement libcloudproviders support (46.96 KB, patch)
2017-08-28 08:17 UTC, Julius Härtl
none Details | Review
[PATCH 2/4] PlacesSidebar: Avoid warning about unused variable (1.00 KB, patch)
2017-08-28 08:17 UTC, Julius Härtl
none Details | Review
[PATCH 3/4] SidebarRow: need config.h for HAVE_CLOUDPROVIDERS (917 bytes, patch)
2017-08-28 08:18 UTC, Julius Härtl
none Details | Review
[PATCH 4/4] gtkplacessidebar: remove oversight of old code (4.44 KB, patch)
2017-08-28 08:19 UTC, Julius Härtl
none Details | Review
PlacesSidebar: Reuse strings, mark for translation (1.59 KB, patch)
2017-08-29 19:05 UTC, Daniel Boles
committed Details | Review
[PATCH] gtkplacessidebar: adapt libcloudproviders api rename (9.20 KB, patch)
2017-09-03 15:13 UTC, Julius Härtl
none Details | Review
[PATCH] gtkplacessidebar: implement libcloudproviders support (master) (46.61 KB, patch)
2017-09-03 15:42 UTC, Julius Härtl
none Details | Review
gtkplacessidebar: Adapt to libcloudproviders 0.2.0 (21.04 KB, patch)
2017-10-31 01:11 UTC, Carlos Soriano
committed Details | Review
gtkplacessidebar: Fix new tab/window handling for cloud accounts (2.45 KB, patch)
2017-10-31 01:11 UTC, Carlos Soriano
committed Details | Review

Description Julius Härtl 2017-08-10 20:48:27 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
Comment 1 Matthias Clasen 2017-08-18 12:00:47 UTC
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 */
Comment 2 Piotr Drąg 2017-08-18 17:35:45 UTC
(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.
Comment 3 Matthias Clasen 2017-08-18 18:14:40 UTC
(In reply to Piotr Drąg from comment #2)
> 
> This string is not actually marked for translation.

Lets fix that first!
Comment 4 Julius Härtl 2017-08-24 14:52:28 UTC
Created attachment 358351 [details] [review]
[PATCH] gtkplacessidebar: implement libcloudproviders support v2
Comment 5 Matthias Clasen 2017-08-24 16:14:50 UTC
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
Comment 6 Julius Härtl 2017-08-24 17:46:44 UTC
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.
Comment 7 Matthias Clasen 2017-08-24 21:03:47 UTC
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
Comment 8 Julius Härtl 2017-08-25 07:09:06 UTC
> 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.
Comment 9 Julius Härtl 2017-08-25 07:11:45 UTC
Created attachment 358383 [details] [review]
[PATCH] gtkplacessidebar: implement libcloudproviders support v4
Comment 10 Matthias Clasen 2017-08-25 16:35:04 UTC
Review of attachment 358383 [details] [review]:

ok
Comment 11 Matthias Clasen 2017-08-25 17:12:16 UTC
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
Comment 12 Matthias Clasen 2017-08-25 20:57:21 UTC
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>
Comment 13 Matthias Clasen 2017-08-25 20:59:34 UTC
i fixed it up myself to get it merged
Comment 14 Matthias Clasen 2017-08-25 21:02:34 UTC
Leaving this bug open to deal with forward-porting the patch to master.
Julius, can you do that ?
Comment 15 Carlos Soriano 2017-08-25 21:17:18 UTC
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?
Comment 16 Matthias Clasen 2017-08-25 22:11:39 UTC
thanks for catching these, carlos
Comment 17 Julius Härtl 2017-08-26 14:35:43 UTC
> 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.
Comment 18 Daniel Boles 2017-08-27 14:26:37 UTC
end_icon in update_places() is unused if not HAVE_CLOUDPROVIDERS, causing a compiler warning, which ideally would be suppressed if not applicable.
Comment 19 Daniel Boles 2017-08-27 14:29:56 UTC
(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.
Comment 20 Daniel Boles 2017-08-27 14:37:43 UTC
Created attachment 358511 [details] [review]
SidebarRow: need config.h for HAVE_CLOUDPROVIDERS
Comment 21 Daniel Boles 2017-08-27 14:37:49 UTC
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.
Comment 22 Julius Härtl 2017-08-27 20:48:38 UTC
Created attachment 358532 [details] [review]
[PATCH] gtkplacessidebar: remove oversight of old code
Comment 23 Carlos Soriano 2017-08-28 06:07:59 UTC
Review of attachment 358532 [details] [review]:

Looks good, thanks Julius!
Comment 24 Carlos Soriano 2017-08-28 06:16:15 UTC
Keep it open for master support with meson
Comment 25 Julius Härtl 2017-08-28 08:17:23 UTC
Created attachment 358562 [details] [review]
[PATCH 1/4] gtkplacessidebar: implement libcloudproviders support
Comment 26 Julius Härtl 2017-08-28 08:17:58 UTC
Created attachment 358564 [details] [review]
[PATCH 2/4] PlacesSidebar: Avoid warning about unused variable
Comment 27 Julius Härtl 2017-08-28 08:18:26 UTC
Created attachment 358566 [details] [review]
[PATCH 3/4] SidebarRow: need config.h for HAVE_CLOUDPROVIDERS
Comment 28 Julius Härtl 2017-08-28 08:19:00 UTC
Created attachment 358567 [details] [review]
[PATCH 4/4] gtkplacessidebar: remove oversight of old code
Comment 29 Daniel Boles 2017-08-28 08:48:35 UTC
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?
Comment 30 Daniel Boles 2017-08-28 08:49:00 UTC
Maybe for master you can squash the patches into one; I don't mind losing credit for two one-liners. :P
Comment 31 Matthias Clasen 2017-08-28 14:20:35 UTC
yes, please
Comment 32 Matthias Clasen 2017-08-28 14:32:00 UTC
and the meson parts look fine to me, fwiw
Comment 33 Piotr Drąg 2017-08-28 17:12:57 UTC
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".
Comment 34 Daniel Boles 2017-08-29 19:05:41 UTC
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 35 Daniel Boles 2017-08-29 19:12:13 UTC
Comment on attachment 358716 [details] [review]
PlacesSidebar: Reuse strings, mark for translation

Attachment 358716 [details] pushed as 9f1d57e - PlacesSidebar: Reuse strings, mark for translation
Comment 36 Daniel Boles 2017-08-29 19:40:20 UTC
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.)
Comment 37 Julius Härtl 2017-09-03 15:13:45 UTC
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.
Comment 38 Julius Härtl 2017-09-03 15:42:34 UTC
Created attachment 359030 [details] [review]
[PATCH] gtkplacessidebar: implement libcloudproviders support (master)
Comment 39 Daniel Boles 2017-09-04 08:53:53 UTC
(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.
Comment 40 Carlos Soriano 2017-10-31 01:11:00 UTC
Created attachment 362590 [details] [review]
gtkplacessidebar: Adapt to libcloudproviders 0.2.0

And a few improvements on the way.
Comment 41 Carlos Soriano 2017-10-31 01:11:09 UTC
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.
Comment 42 Carlos Soriano 2017-10-31 01:12:31 UTC
Julius, can you take a quick look to these?
Comment 43 Carlos Soriano 2017-11-13 14:42:49 UTC
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
Comment 44 Carlos Soriano 2017-11-13 16:01:07 UTC
Cherry picked to master too.