Merge lp:~optimisme/pantheon-files/fix-1184604 into lp:~elementary-apps/pantheon-files/trunk

[Albert]

- Local network root overlays
- Network root folder detection
- Open files from "smb" folders

Fixed:

- Local network root overlays
- Network root folder detection
- Open files from "smb" folders
https://bugs.launchpad.net/pantheon-files/+bug/1184977

[Victor Martinez (victored)]

Nice work, as always.

A couple of suggestions (diff lines):

- 26: "else" clause not needed.
- 39, 40: Use g_strdup instead.

I'm not a big fan of the way some things were implemented in GOFFile. Historically, it duplicates a couple of things in memory that are still allocated in GOFFile.info, unnecessarily increasing memory consumption since the latter is not freed. To avoid extending this problem, I'd recommend adding GOFFile.get_target_location_uri to the API, and returning directly the result of: "g_file_info_get_attribute_string (file->info, G_FILE_ATTRIBUTE_STANDARD_TARGET_URI);" so that the code doesn't duplicate that string (do not free this string since it's owned by GOFFile.info).

- 21 - 29: Some coding style corrections are needed here. Also either documentation or more verbose code would be appreciated. I'd suggest moving that portion of code to a separate function (e.g. is_complete_smb_uri), and documenting what that method does on that function. What I've figured by reading the code is that it verifies the completeness of an URI, and if it doesn't appear to be complete it's treated as root.

review: Needs Fixing

[lp:~optimisme/pantheon-files/fix-1184604]

1201. By Albert on 2013-05-28

last fixes

1202. By Albert on 2013-05-28

last fixes

[Albert (optimisme)]

Hi Victor,

I just "small fixed" the source, now I will add the new suggested functions to lower memory and make it more readable.

[lp:~optimisme/pantheon-files/fix-1184604]

1203. By Albert on 2013-05-28

new function to check if an uri is default

[Albert (optimisme)]

Hi Victor,

I have tried to remove the "target_location_uri" and replace it by a function but I am receiving segmentation faults, so I will leave it with the variable.

[lp:~optimisme/pantheon-files/fix-1184604]

1204. By Albert on 2013-05-28

last fixes

1205. By Albert on 2013-05-28

last fixes

1206. By Albert on 2013-05-29

changed variable by function

1207. By Albert on 2013-05-29

changed variable by function

1208. By Albert on 2013-05-29

changed variable by function

[Albert (optimisme)]

I think it accomplishes every suggestion and is ready to be released.

This bug also fixes:
https://bugs.launchpad.net/pantheon-files/+bug/1184977

[Victor Martinez (victored)]

Albert,

This is looking great so far.

I forgot to mention that GOFFile is a critical part of Pantheon Files and its plugin API, so special care must be taken when modifying the members of the structure, since we want to make sure external plugins (i.e. the ones not re-built along with the core library) are not broken; specially the Dropbox and UbuntuOne plugins. The latter is broken after merging this branch due to the ABI break described below.

Modifying the size of a structure is dangerous, but it doesn't seem to break any of the plugins we know so far. However, inserting structure members in the middle of existing ones does cause breaks, since member indexes are displaced resulting in incorrect pointers being de-referenced. Therefore my only request will be moving the new target_location_uri member to the very bottom of the _GOFFile structure.

On another topic, gof_file_is_location_uri_default should not be publicly exposed. Please remove it from gof-file.h and make it a static function in gof-file.c. I like what you did with the implementation of this method; just make sure the comment follows this style: http://bazaar.launchpad.net/~elementary-apps/pantheon-files/trunk/view/head:/libcore/eel-fcts.c#L99

I have not tried this at home, but could you please tell me if this doesn't produce the same results:

112 + if (goffile.is_network_uri_scheme ()) {
113 + status.set_label (goffile.get_target_location ().get_uri ());
114 + } else if (!goffile.is_folder ()) {

...?

review: Needs Fixing

[lp:~optimisme/pantheon-files/fix-1184604]

1209. By Albert on 2013-05-29

static method is_location_uri_default

[Albert (optimisme)]

Hi Victor,

- status.set_label (goffile.get_target_location ().get_uri ()), produces something like "network://dnsssd-domain-Backups._afpovertcp._tcp/", it is just what we are trying to prevent.

- target_location_uri is not in the source anymore, because the new function "gof_file_get_standard_target_uri" is being used

- "gof_file_get_standard_target_uri" has been removed from "gof-file.h", has been made static and moved "up" with the other static functions.

- "gof_file_get_standard_target_uri" the comments has been copied from the suggested source (one of the 3 types of comments there)

[lp:~optimisme/pantheon-files/fix-1184604]

1210. By Albert on 2013-05-29

function moved at the bottom of the .h file

[Albert (optimisme)]

Sorry, the new static function is "gof_file_is_location_uri_default", "gof_file_get_standard_target_uri" has been moved at the bottom of the "gof-file.h" file to prevent plugins problems.

[lp:~optimisme/pantheon-files/fix-1184604]

1211. By Albert on 2013-05-29

get_standard_target_uri moved at the bottom of marlincor-C space

[Victor Martinez (victored)]

Great work Albert!

Final suggestions:

- Remove extra line from comment (diff line 9)

- Add back diff line 53 (revert change at diff line 54)

- get_standard_target_uri => get_display_target_uri. (diff line 63. Also add space before parenthesis).
  * For the implementation, please use "return g_file_info_get_attribute_as_string (file->info, G_FILE_ATTRIBUTE_STANDARD_TARGET_URI);" instead so that the string is properly formatted in UTF-8 as expected by GTK+. There's no need to call g_strdup on it's return value.
  * in marlincore-C.vapi, make sure to define the return value as "unowned" so that no Vala code frees the string. This would look like: "public unowned string get_display_target_uri ();"

It's 3:47 am here so I may go away to sleep for a couple of hours. I'll review and test the branch right away when I get back. I'm sure I'll be marking it as approved :)

Thank you very much for your work!!

[lp:~optimisme/pantheon-files/fix-1184604]

1212. By Albert on 2013-05-29

linal fixes

[Victor Martinez (victored)]

Albert,

Excellent work!

Everything work well here so far. I'm happy because your code doesn't break the core library ABI :)

review: Approve

[Albert (optimisme)]

Thanks Victor, I hope it is released soon.

[Victor Martinez (victored)]

We expect the new package to be ready in less than 2 days.

=== modified file 'libcore/gof-file.c'
--- libcore/gof-file.c	2013-05-28 07:25:28 +0000
+++ libcore/gof-file.c	2013-05-29 10:01:27 +0000
@@ -191,6 +191,30 @@
     return FALSE;
 }
 
+/**
+ * gof_file_is_location_uri_default:
+ *
+ * Returns true if it is an uri at "/"
+ * (example: afp://server.local:123/)
+ *
+**/
+static gboolean
+gof_file_is_location_uri_default (GOFFile *file)
+{
+    char* target_location_uri = gof_file_get_display_target_uri (file);
+    if (target_location_uri != NULL) {
+        gchar **split = g_strsplit (target_location_uri, "/", 4);
+        if (split[3] == NULL || !strcmp (split[3], "")) {
+            g_strfreev (split);
+            g_free (target_location_uri);
+            return TRUE;
+        }
+        g_strfreev (split);
+    }
+    g_free (target_location_uri);
+    return FALSE;
+}
+
 gboolean
 gof_file_is_remote_uri_scheme (GOFFile *file)
 {
@@ -204,8 +228,10 @@
 gboolean
 gof_file_is_root_network_folder (GOFFile *file)
 {
-    return gof_file_is_network_uri_scheme (file)
-        || gof_file_is_smb_uri_scheme (file);
+    if (gof_file_is_network_uri_scheme (file))
+        return TRUE;
+
+    return (gof_file_is_smb_uri_scheme (file) && gof_file_is_location_uri_default (file));
 }
 
 gboolean
@@ -2376,6 +2402,13 @@
     return NULL;
 }
 
+char *
+gof_file_get_display_target_uri (GOFFile* file)
+{
+    return g_file_info_get_attribute_as_string (file->info, G_FILE_ATTRIBUTE_STANDARD_TARGET_URI);
+}
+
+
 const gchar *
 gof_file_get_preview_path(GOFFile* file)
 {

=== modified file 'libcore/gof-file.h'
--- libcore/gof-file.h	2013-05-27 22:20:15 +0000
+++ libcore/gof-file.h	2013-05-29 10:01:27 +0000
@@ -233,7 +233,6 @@
  * Return value: the #GOFFileThumbState for @file.
  **/
 #define gof_file_get_thumb_state(file) (GOF_FILE ((file))->flags & GOF_FILE_THUMB_STATE_MASK)
-
 const gchar* gof_file_get_thumbnail_path (GOFFile *file);
 const gchar* gof_file_get_preview_path (GOFFile *file);
 gboolean        gof_file_can_set_owner (GOFFile *file);
@@ -258,6 +257,7 @@
 gboolean        gof_file_is_smb_uri_scheme (GOFFile *file);
 gboolean        gof_file_thumb_can_frame (GOFFile *file);
 
+char            *gof_file_get_display_target_uri (GOFFile* file);
 G_END_DECLS
 
 #endif /* GOF_DIRECTORY_ASYNC_H */

=== modified file 'libcore/marlincore-C.vapi'
--- libcore/marlincore-C.vapi	2013-05-27 10:54:46 +0000
+++ libcore/marlincore-C.vapi	2013-05-29 10:01:27 +0000
@@ -196,8 +196,11 @@
         public static int compare_by_display_name (File file1, File file2);
 
         public bool is_remote_uri_scheme ();
+        public bool is_root_network_folder ();
         public bool is_network_uri_scheme ();
         public bool is_smb_uri_scheme ();
+
+        public unowned string get_display_target_uri ();
     }
 
     [CCode (cheader_filename = "gof-file.h")]

=== modified file 'src/View/OverlayBar.vala'
--- src/View/OverlayBar.vala	2013-05-12 05:27:30 +0000
+++ src/View/OverlayBar.vala	2013-05-29 10:01:27 +0000
@@ -215,7 +215,9 @@
         private void update_status ()
         {
             if (count == 1) {
-                if (!goffile.is_folder ()) {
+                if (goffile.is_network_uri_scheme ()) {
+                    status.set_label (goffile.get_display_target_uri ());
+                } else if (!goffile.is_folder ()) {
                     status.set_label ("%s - %s (%s)".printf (goffile.info.get_name (), goffile.formated_type, goffile.format_size));
                 } else {
                     status.set_label ("%s - %s".printf (goffile.info.get_name (), goffile.formated_type));