GNOME Bugzilla – Bug 775091
file-operations: port from f () function
Last modified: 2017-02-23 10:53:06 UTC
Hard to find, hard to follow :) Here it is: https://git.gnome.org/browse/nautilus/tree/src/nautilus-file-operations.c#n1101 We should slowly port every use case of that function to custom functions. Not only this function is hard to understand and follow, but is also non standard which makes it problematic for translation (where everything is check using c standards to avoid crashing the application if the translator makes some unsuported change). Also vargs....ugh. This is good for newcomers
Created attachment 346177 [details] [review] file-operations: Rename and change the return type of a function The function format_time does not clearly tell anything about the fact that it is supposed to return something. And also it's return type is char *. So, to make it consistent it's return type is changed to gchar *. This patch renames format_time to get_formatted_time.
Created attachment 346184 [details] [review] file-operations: Remove f() function and use custom functions in its place Not only this function is hard to understand and follow, but is also non standard which makes it problematic for translation (where everything is check using c standards to avoid crashing the application if the translator makes some unsuported change). This patch removes every use case of f() function and uses g_strdup_printf and g_strdup in its place and uses custom functions directly to convert required parameters into gchar* types.
Review of attachment 346177 [details] [review]: Looks good to me, thanks for these improvements!
Review of attachment 346184 [details] [review]: Good patch! It needs some work, in the leaks mostly. I reviewed some of them, but basically all that returns a gchar needs to free the memory. Otherwise it looks quite good. ::: src/nautilus-file-operations.c @@ +949,3 @@ +static gchar * +get_basename(GFile *file) space before parentheses @@ +1878,3 @@ secondary = IS_IO_ERROR (error, PERMISSION_DENIED) ? + g_strdup_printf (_("There was an error deleting the folder “%s”."), + get_basename (file)) : This is leaking @@ +1880,3 @@ + get_basename (file)) : + g_strdup_printf (_("You do not have sufficient permissions to delete the folder “%s”."), + get_basename (file)); ditto @@ +1886,3 @@ secondary = IS_IO_ERROR (error, PERMISSION_DENIED) ? + g_strdup_printf (_("There was an error deleting the file “%s”."), + get_basename (file)) : ditto @@ +1888,3 @@ + get_basename (file)) : + g_strdup_printf (_("You do not have sufficient permissions to delete the file “%s”."), + get_basename (file)); ditto @@ +2017,3 @@ nautilus_progress_info_take_status (job->progress, + g_strdup_printf (status, + get_basename ((GFile *) delete_job->files->data))); use the macro G_FILE(), it has type checking safeguards. Also leaking @@ +2093,3 @@ + details = g_strdup_printf (concat_detail, + transfer_info->num_files + 1, source_info->num_files, + get_formatted_time (remaining_time), leaking @@ +2172,3 @@ /* Translators: %B is a file name */ + primary = g_strdup_printf (_("“%s” can’t be put in the trash. Do you want to delete it immediately?"), + get_basename (file)); leaking @@ +2539,3 @@ if (data->eject) { + primary = g_strdup_printf (_("Unable to eject %s"), g_mount_get_name (G_MOUNT (source_object))); g_mount_get_name leaks in here @@ +2543,3 @@ else { + primary = g_strdup_printf (_("Unable to unmount %s"), g_mount_get_name (G_MOUNT (source_object))); ditto @@ +2951,3 @@ + "Preparing to copy %'d files (%s)", + source_info->num_files), + source_info->num_files, g_format_size (source_info->num_bytes)); leaking @@ +2960,3 @@ + "Preparing to move %'d files (%s)", + source_info->num_files), + source_info->num_files, g_format_size (source_info->num_bytes)); ditto @@ +2969,3 @@ + "Preparing to delete %'d files (%s)", + source_info->num_files), + source_info->num_files, g_format_size (source_info->num_bytes)); ditto @@ +3108,3 @@ { + secondary = g_strdup_printf (_("Files in the folder “%s” cannot be handled because you do " + "not have permissions to see them."), get_basename (dir)); leaks @@ +3113,3 @@ { + secondary = g_strdup_printf (_("There was an error getting information about the files in the folder “%s”."), + get_basename (dir)); leak @@ +3163,3 @@ { + secondary = g_strdup_printf (_("The folder “%s” cannot be handled because you do not have " + "permissions to read it."), get_basename (dir)); ditto, but yeah all of those get_basename etc. are leaking @@ +6569,3 @@ else { + secondary = g_strdup_printf (_("There was an error creating the symlink in %s."), g_file_get_parse_name (dest_dir)); put it in two lines, never go more than 80 characters as soft limit (100 hard limit)
Created attachment 346317 [details] [review] file-operations: Remove f() function and use custom functions in its place Not only this function is hard to understand and follow, but is also non standard which makes it problematic for translation (where everything is check using c standards to avoid crashing the application if the translator makes some unsuported change). This patch removes every use case of f() function and uses g_strdup_printf and g_strdup in its place and uses custom functions directly to convert required parameters into gchar* types.
Created attachment 346321 [details] [review] file-operations: Remove f() function and use custom functions in its place Not only this function is hard to understand and follow, but is also non standard which makes it problematic for translation (where everything is check using c standards to avoid crashing the application if the translator makes some unsuported change). This patch removes every use case of f() function and uses g_strdup_printf and g_strdup in its place and uses custom functions directly to convert required parameters into gchar* types.
Created attachment 346369 [details] [review] file-operations: Remove f() function and use custom functions in its place Not only this function is hard to understand and follow, but is also non standard which makes it problematic for translation (where everything is check using c standards to avoid crashing the application if the translator makes some unsuported change). This patch removes every use case of f() function and uses g_strdup_printf and g_strdup in its place and uses custom functions directly to convert required parameters into gchar* types.
Review of attachment 346369 [details] [review]: Hey, are you sure you applied the previous review? The code is still leaking all the gchar arrays.
Created attachment 346419 [details] [review] file-operations: Remove f() function and use custom functions in its place Not only this function is hard to understand and follow, but is also non standard which makes it problematic for translation (where everything is check using c standards to avoid crashing the application if the translator makes some unsuported change). This patch removes every use case of f() function and uses g_strdup_printf and g_strdup in its place and uses custom functions directly to convert required parameters into gchar* types.
Review of attachment 346419 [details] [review]: I think you are very close now to have it done :D ::: src/nautilus-file-operations.c @@ +1500,3 @@ if (file_count == 1) { + gchar* basename; use autopointers: g_autofree gchar *basename = NULL; Same for all of them, so you avoid the g_free. @@ +2037,3 @@ } + + gchar *basename; we define variables at the beggining of the scope. This is due to C89 standard, although we are moving to C11 we still don't have a style for C11, so better keep it how it is now. @@ +2044,3 @@ + + g_free (basename); + spurious new line @@ +3728,3 @@ } + + gchar *basename_dest; ditto
Created attachment 346439 [details] [review] file-operations: Remove f() function and use custom functions in its place Not only this function is hard to understand and follow, but is also non standard which makes it problematic for translation (where everything is check using c standards to avoid crashing the application if the translator makes some unsuported change). This patch removes every use case of f() function and uses g_strdup_printf and g_strdup in its place and uses custom functions directly to convert required parameters into gchar* types.
Created attachment 346486 [details] [review] file-operations: Remove f() function and use custom functions in its place Not only this function is hard to understand and follow, but is also non standard which makes it problematic for translation (where everything is check using c standards to avoid crashing the application if the translator makes some unsuported change). This patch removes every use case of f() function and uses g_strdup_printf and g_strdup in its place and uses custom functions directly to convert required parameters into gchar* types.
Review of attachment 346486 [details] [review]: Excellent work! The title of the commit message is too long, remember to always follow https://wiki.gnome.org/Newcomers/CodeContributionWorkflow#Commit_guidelines But it's okay, I will modify it myself. Congrats on this big patch! ::: src/nautilus-file-operations.c @@ -8714,2 @@ } - There is a style change here that shouldn't be, but it's fine :)
Attachment 346177 [details] pushed as 1182b2c - file-operations: Rename and change the return type of a function