GNOME Bugzilla – Bug 700500
Use GFile instead of stdio and POSIX I/O in easytag.c
Last modified: 2013-05-18 17:48:39 UTC
Currently EasyTAG uses POSIX IO i.e. functions read, open, write, rename, remove etc.. POSIX IO could be replaced with much better and high level GFile from GIO. This will simplify file handling, name of file handling encoding, could also help in making Asynchronous I/O and with this EasyTAG could be used over NFS and CIFS/Samba shares.
Created attachment 244490 [details] [review] Fixed this Bug Here, I have removed a plenty of functions. remove, rename were used they were removed by g_file_set_display_name and g_file_delete. Copy_File, was replaced by g_file_copy. Make_Dir, was replaced by g_file_make_directory_with_parents Read_Directory_Recursively, was modified to take GFileEnumerator * as its one of the arguments. It is passed GFileEnumerator* not GFile*, because a check is performed before calling this function at line 3152, for opening directory which is possible with g_file_enumerate_children only. Also, in this function I could have used g_file_enumerate_get_child instead of three calls of g_file_info_get_name, g_file_get_child, g_file_enumerator_get_container. but I was getting compiling error for g_file_enumerate_get_child. may it is present only in GIO 2.36 not in GIO 2.32, so I used three functions.
Review of attachment 244490 [details] [review]: This patch breaks the recursive reading of directories; only files within the current directory are found. ::: src/easytag.c @@ +2824,3 @@ + + GFile *cur_file = g_file_new_for_path (cur_filename); + GFile *new_file = g_file_set_display_name (cur_file, new_filename, NULL, &error); You should follow the documentation for g_file_set_display_name() and make sure that the edit name is set as the initial value in the UI.
Created attachment 244544 [details] [review] Created a new et_rename_file function and changes et_core.c Created et_rename_file function. It can rename/move files and can create a directory tree. Also, corrected the recursive searching of files. Also, while testing the changes I found "Glib-CRITICAL assertion g_object_unref failed, G_IS_OBJECT (object) assertion failed". I corrected that one too in this patch, as it is arising after applying this patch. I have compiled and tested the function et_rename_file with different cases and it is working fine. This function uses only GFile, no POSIX File System Interface functions.
Review of attachment 244544 [details] [review]: ::: src/easytag.c @@ +2884,3 @@ +/*Rename or Move file + *Returns TRUE if success Use the gtk-doc syntax for comments above functions. You can see examples of it in recent commits. @@ +2894,3 @@ + GFile *file_new_parent; + + g_return_val_if_fail (error == NULL || *error == NULL, FALSE); You do not unref the GFiles that you just created above. @@ +2897,3 @@ + + /*Check if new file exists*/ + if (g_file_query_exists (file_new, NULL)) Do not check if the file exists before moving it, simply move it and if the move fails report an error. Do not deal with the error in this function, but set a GError and return. @@ +2921,3 @@ + + /*Check if new_filepath's parent directory exists, if not create it*/ + if (!g_file_query_exists (file_new_parent, NULL)) Again, there is little point checking that the directory exists before creating it, so just create the directory with g_file_make_directory_with_parents() and ignore the G_IO_ERROR_EXISTS error. If any other errors are reported, return. @@ +2937,3 @@ + /*Move the file*/ + if (!g_file_move (file_old, file_new, G_FILE_COPY_OVERWRITE, NULL, NULL, + NULL, error)) Wrong indentation here. @@ +3097,3 @@ + else + { + Log_Print (LOG_ERROR, _("Cannot delete file '%s' (%s)"), Do not report the error here, but add a GError argument to this function, set it and return. ::: src/et_core.c @@ +2852,3 @@ g_object_unref (emblem_icon); } + g_object_unref (info); Split this change off into a separate patch.
Created attachment 244602 [details] [review] Did the above said changes.
Review of attachment 244602 [details] [review]: ::: src/easytag.c @@ +3375,1 @@ return file_list; You need to unref the GFileInfo that you created at the start of the loop before returning. @@ +3389,3 @@ + /*Searching for files recrusively*/ + GFile *child_dir = g_file_get_child (g_file_enumerator_get_container (dir_enumerator), + file_name); Wrong indentation. @@ +3401,3 @@ + { + Log_Print (LOG_ERROR, + "Error opening directory '%s' (%s)", This string should be translatable. @@ +3405,3 @@ + g_error_free (child_error); + g_object_unref (child_dir); + continue; You need to unref the GFileInfo you created at the start of the loop before starting another iteration. @@ +3413,3 @@ + g_object_unref (childdir_enumerator); + } + }else if (type == G_FILE_TYPE_REGULAR && Add a newline after the closing curly brace. @@ +3414,3 @@ + } + }else if (type == G_FILE_TYPE_REGULAR && + ET_File_Is_Supported(file_name)) Add a space before the opening parenthesis. @@ +3420,3 @@ + gchar *file_path = g_file_get_path (file); + file_list = g_list_append (file_list, file_path); + g_object_unref (file); You also need to free file_path here.
Created attachment 244606 [details] [review] Done file_path shouldn't be freed, because it is used by g_list. If we free the file_path, then it can be seen that BrowserFileList doesn't show the filenames properly.
Comment on attachment 244606 [details] [review] Done (In reply to comment #7) > file_path shouldn't be freed, because it is used by g_list. If we free the > file_path, then it can be seen that BrowserFileList doesn't show the filenames > properly. Good catch. I get an assert while renaming files: ERROR:src/easytag.c:2929:et_rename_file: assertion failed: (error == NULL || *error == NULL)
Created attachment 244611 [details] [review] Corrected I added g_assert () at that place to check that error is NULL before passing it to other functions in case g_file_make_directory_with_parents fail with G_IO_ERROR_EXISTS. So, if file is renamed in the same location that g_assertion will failed. I used g_clear_error to assign error to NULL. Also, tell me about patch of et_core.c. Should I report a new Bug or upload the patch here?
(In reply to comment #9) > Also, tell me about patch of et_core.c. Should I report a new Bug or upload the > patch here? You should reopen bug 700359 and attach the patch there.
Comment on attachment 244611 [details] [review] Corrected Thanks. I pushed a modified patch to master as commit 5fae33d307cccfdcfbd1594f2c727d51cb38571e.
When renaming audio files in a directory, the files are renamed on the filesystem but the file list does not update to reflect this, instead showing that the files have pending modifications. This is a regression from the previous behaviour.
Created attachment 244620 [details] [review] Fixed it.
Comment on attachment 244620 [details] [review] Fixed it. Pushed a slightly modified patch to master as commit 8072eb66a9561cf3d7bd8690c1726e7f39816f6a, thanks.