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 769602 - files-view: use GFile utilities
files-view: use GFile utilities
Status: RESOLVED FIXED
Product: nautilus
Classification: Core
Component: general
unspecified
Other All
: Low enhancement
: ---
Assigned To: Nautilus Maintainers
Nautilus Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-08-07 08:30 UTC by Ernestas Kulik
Modified: 2016-08-21 16:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
files-view: use GFile utilities (4.03 KB, patch)
2016-08-07 08:30 UTC, Ernestas Kulik
none Details | Review
files-view: use GFile utilities (5.91 KB, patch)
2016-08-18 22:12 UTC, Ernestas Kulik
none Details | Review
files-view: use GFile utilities (6.19 KB, patch)
2016-08-19 07:41 UTC, Ernestas Kulik
none Details | Review
files-view: use GFile utilities (6.56 KB, patch)
2016-08-20 19:49 UTC, Ernestas Kulik
committed Details | Review

Description Ernestas Kulik 2016-08-07 08:30:52 UTC
set_up_scripts_directory_global() currently uses the POSIX file
interface, which is inconsistent with the rest of the code. This commit
rewrites the parts of the function to use the GFile API.
Comment 1 Ernestas Kulik 2016-08-07 08:30:58 UTC
Created attachment 332880 [details] [review]
files-view: use GFile utilities
Comment 2 Razvan Chitu 2016-08-18 18:18:20 UTC
Review of attachment 332880 [details] [review]:

Thanks for doing this and sorry for a long time to review! Still needs a bit of work though. Also, it'd be cool to see the other utility functions like "g_build_filename" be replaced as well.

::: src/nautilus-files-view.c
@@ +2589,3 @@
                 message = _("Nautilus 3.6 deprecated this directory and tried migrating "
                             "this configuration to ~/.local/share/nautilus");
                 if (!g_file_test (updated, G_FILE_TEST_EXISTS)) {

I would suggest replacing this and and the other "Utility Functions" with GFile API. In this case it'd be g_file_query_exists.

@@ +2598,1 @@
+                        if (!g_file_make_directory_with_parents (directory, NULL, NULL)) {

I think this condition is wrong. g_mkdir_with_parents will return 0 if the directory was created OR if it already exists. g_file_make_directory_with_parents will fail if the directory exists, won't it?. You could add an error parameter and also check for IO_ERROR_EXISTS.

@@ +2605,3 @@
+                                                          NULL, NULL);
+
+                                if (g_file_info_has_attribute (info, G_FILE_ATTRIBUTE_UNIX_MODE)) {

You should replace info querying and attribute checking / setting with g_file_set_attribute_uint32.
Comment 3 Ernestas Kulik 2016-08-18 22:12:32 UTC
Created attachment 333595 [details] [review]
files-view: use GFile utilities

set_up_scripts_directory_global() currently uses the POSIX file
interface, which is inconsistent with the rest of the code. This commit
rewrites the parts of the function to use the GFile API.
Comment 4 Ernestas Kulik 2016-08-19 07:41:36 UTC
Created attachment 333614 [details] [review]
files-view: use GFile utilities

set_up_scripts_directory_global() currently uses the POSIX file
interface, which is inconsistent with the rest of the code. This commit
rewrites the parts of the function to use the GFile API.
Comment 5 Carlos Soriano 2016-08-20 18:37:35 UTC
Review of attachment 333614 [details] [review]:

::: src/nautilus-files-view.c
@@ +2586,3 @@
+        scripts_directory = g_file_new_for_path (scripts_directory_path);
+
+        if (g_file_query_file_type (old_scripts_directory,

use variables, the if is ugly with this identation

@@ +2601,3 @@
+                        g_autoptr (GError) error = NULL;
+
+                        if (g_file_make_directory_with_parents (parent, NULL, &error) ||

put this outside and only check for no error or IO_EXISTS. As a rule of thumb don't put too many calls or weight inside if() conditions, rather use variables to explain what is going on.

@@ +2609,3 @@
+                                                             NULL, NULL);
+
+                            g_error_matches (error, G_IO_ERROR, G_IO_ERROR_EXISTS)) {

same as before, and check for errors.

@@ +2636,3 @@
         }
 
+        return (scripts_directory_uri != NULL);

not need for parenteses. Also omg to the previous condition here :D
Comment 6 Ernestas Kulik 2016-08-20 19:49:28 UTC
Created attachment 333771 [details] [review]
files-view: use GFile utilities

set_up_scripts_directory_global() currently uses the POSIX file
interface, which is inconsistent with the rest of the code. This commit
rewrites the parts of the function to use the GFile API.
Comment 7 Carlos Soriano 2016-08-21 13:50:48 UTC
Review of attachment 333771 [details] [review]:

excellent, thanks a lot!
Comment 8 Ernestas Kulik 2016-08-21 16:45:21 UTC
Attachment 333771 [details] pushed as 672ccbc - files-view: use GFile utilities