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 787128 - Re-add FUSE network mounts in local-only mode
Re-add FUSE network mounts in local-only mode
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkFileChooser
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
: 727538 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2017-09-01 13:54 UTC by Colin Leroy
Modified: 2018-08-28 19:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch against git master (2.11 KB, patch)
2017-09-01 13:54 UTC, Colin Leroy
none Details | Review
Updated patch with coding styles issues fixed (2.52 KB, patch)
2017-09-02 06:58 UTC, Colin Leroy
none Details | Review
Better patch to fix the problem (1.97 KB, patch)
2017-09-07 15:17 UTC, Colin Leroy
none Details | Review
patch v3 addressing Bastien's remarks (2.59 KB, patch)
2017-09-12 13:39 UTC, Colin Leroy
committed Details | Review

Description Colin Leroy 2017-09-01 13:54:24 UTC
Created attachment 358936 [details] [review]
Patch against git master

Hi,

The attached patch re-adds the network mounts that are accessible locally (via FUSE) to GtkFileChooser when in local-only mode.

A lot of people miss this a lot while major applications (Firefox, Chromium, etc) gradually switch to GTK+3.

The patch applies to today's git master.

Hope this helps,

Colin
Comment 1 Daniel Boles 2017-09-02 06:37:23 UTC
Review of attachment 358936 [details] [review]:

I can't speak for whether the behaviour is desired, but I *can* post style nitpicks so no one else has to.

::: gtk/gtkplacessidebar.c
@@ +894,3 @@
+      if (path != NULL)
+        {
+  base_file = g_mount_get_root (mount);

missing space before parenthesis

@@ +1300,3 @@
+          mount = l->data;
+
+  if (sidebar->local_only)

ditto

@@ +1308,3 @@
+    }
+
+          mount = l->data;

I'm not sure we need a 3-line comment when the code explains the same thing pretty unambiguously already.

@@ +1347,3 @@
           mount = l->data;
+
+          if (sidebar->local_only && !is_mount_locally_accessible(mount))

another missing space before parenthesis

@@ +1348,3 @@
+
+          if (sidebar->local_only && !is_mount_locally_accessible(mount))
+              continue;

This should be 2 spaces of indentation, not 4
Comment 2 Daniel Boles 2017-09-02 06:39:50 UTC
Thanks again to Bugzilla/Splinter for showing my additions below the wrong lines in comments in 50% of cases. That's really helpful. Check the review for what I was really commenting on.
Comment 3 Colin Leroy 2017-09-02 06:58:09 UTC
Created attachment 358969 [details] [review]
Updated patch with coding styles issues fixed

Hi Daniel,

Thanks for your comments. I have updated the patch accordingly :)
Comment 4 Daniel Boles 2017-09-03 10:13:35 UTC
Thanks for that!
Comment 5 Colin Leroy 2017-09-07 15:17:36 UTC
Created attachment 359361 [details] [review]
Better patch to fix the problem

Hello,

Here is an updated patch. I based my first one on a patch I did against GTK+3.14, which did not have an "Other Locations" entry in the sidebar, and it did add the remote shares to the sidebar.

Apparently the new way is to put them in Places View.

This patch does that.

Hope this helps.
Comment 6 Colin Leroy 2017-09-11 08:16:23 UTC
*** Bug 727538 has been marked as a duplicate of this bug. ***
Comment 7 Bastien Nocera 2017-09-12 12:51:06 UTC
Review of attachment 359361 [details] [review]:

This should be git-formatted with an explanation as to why you're making this change.

::: gtk/gtkplacesview.c
@@ +1905,3 @@
+  base_file = g_mount_get_root (mount);
+
+  if (base_file != NULL)

Why not carry on with the "return early" way?
if (base_file == NULL)
  return FALSE;

if (path == NULL)
...

@@ +1907,3 @@
+  if (base_file != NULL)
+    {
+      char *path = g_file_get_path (base_file);

Don't assign results of a function when declaring.

@@ +1912,3 @@
+      if (path != NULL)
+        {
+          g_free(path);

Space before parenthesis.

@@ +1948,3 @@
+      g_object_get(G_OBJECT (placesviewrow), "mount", &mount, NULL);
+
+      if (mount != NULL)

Is this necessary? You already do the check in the function.

@@ +1951,3 @@
+        {
+          is_local = is_mount_locally_accessible (mount);
+          g_object_unref(mount);

Space before parenthesis as well. You could also use g_clear_object and avoid the NULL check.
Comment 8 Colin Leroy 2017-09-12 13:39:21 UTC
Created attachment 359630 [details] [review]
patch v3 addressing Bastien's remarks

Hi Bastien,

Thanks for the review. Here is an updated patch.
Comment 9 Colin Leroy 2017-10-12 09:11:03 UTC
Hi,

Any news on this patch?

Thanks in advance,

Colin
Comment 10 Erwin ter Hofstede 2017-11-23 20:35:56 UTC
Hi,

I'm not a linux guru, sorry. Can someone please explain to me how I can apply this patch?
And does it also work with Linux Mint 18.2 Cinnamon 64bit?

Thank you very much!
Kind regards,
Erwin ter Hofstede
Comment 11 Daniel Boles 2017-11-23 23:40:15 UTC
It's not Linux-specific. This is a source patch, so to apply it, you need to use a compatible program (in this case Git) to incorporate its changes, then recompile a new version of library including it. That'll work on any OS that can compile/run this version of GTK+ 3. Google for JHbuild for an easy way to build.

As for whether there's any news on the patch getting into GTK+ upstream, not until a maintainer OKs it. You can ping IRC or the mailing list with a reminder.
Comment 12 Matthias Clasen 2017-12-08 02:00:38 UTC
Review of attachment 359630 [details] [review]:

looks fine to me
Comment 13 Matthias Clasen 2017-12-08 02:01:36 UTC
Review of attachment 359630 [details] [review]:

looks fine to me
Comment 14 Matthias Clasen 2017-12-08 02:01:50 UTC
Review of attachment 359630 [details] [review]:

looks fine to me
Comment 15 Junior Menegatti 2018-08-28 19:43:22 UTC
Good afternoon. I'm using Debian Stretch, it have the same problem. Could you give me a step by step how to use this patch to fix the problem?