GNOME Bugzilla – Bug 768311
Batch rename
Last modified: 2016-08-29 20:45:06 UTC
Here I'll add the work I did until now in order to be reviewed
Created attachment 330771 [details] [review] Limit the label size Use predefined label functions to limit the label size instead of using memcpy to limit the label size.
Created attachment 330772 [details] [review] Rename nautilus-rename-utilities Rename nautilus-rename-utilities.c/h to nautilus-batch-rename-utilities.c/h since it's a more relevant name.
Created attachment 330773 [details] [review] Handle conflicts If there are any conflicts, instead of the usual label there is used an expander with a scrollable listbox as child. Inside the listbox are displayed all the conflicts.
Created attachment 330774 [details] [review] Move buttons to header-bar
Created attachment 330775 [details] [review] Override constructor The constructor was overriden to set the use-header-bar property. This was needed because the init function was called before the constructor, so the property was set in the ui file, then unset by the constructor. This was also needed because this property is construct only.
Created attachment 330776 [details] [review] Switch append/prepend functions Append was doing what prepend was supposed to do and prepend what append should do.
Created attachment 330777 [details] [review] Simplify setting use-header-bar property Don't override the constructor, since this can be done easier, by setting the property in g_object_new
Created attachment 330778 [details] [review] Add UI for Format mode Add Text mode was merged with the format mode.
Created attachment 330779 [details] [review] Modify UI
Created attachment 330780 [details] [review] Update UI and add functionalities to new UI Now the user can replace text (as before) and the more complex mode only does prepending for now, because the options in the popovers are still not added.
Created attachment 330781 [details] [review] Add sorting options for files names Now the files names can be sorted by using criteria like: original name or last modified date. This is useful when there is used a numbering option and the user is not pleased with the default order of the files and wants to change it. In this commit some warnigns were also fixed.
Created attachment 330782 [details] [review] Change arrow used in listbox Instead of a GtkArrow in the listbox, now it's used a GtkImage.
Created attachment 330783 [details] [review] Use GtkPopoverMenu instead of GtkMenuButton The GtkMenuButton is now a GtkToggleButton that shows a GtkPopoverMenu with the same actions that were before in the menu.
Created attachment 330784 [details] [review] Use a single action Instead of using a separate action for each sort mode, use a single one.
Created attachment 330785 [details] [review] Switch back to GMenu The GtkPopoverMenu is no longer used, since it's not really needed. The sorting by name (ascending and descending) is changed. Old commented code of handling conflicts is removed, because it was used for the previous UI. This commit also fixes some warnings.
Created attachment 330786 [details] [review] Add Replace and Add text modes
Review of attachment 330786 [details] [review]: Hey Alex, great job!! Some smart code that took my brain 100% for several minutes to figure out :D Here's a first review of this patch. It's not complete though, but you can at least start with it. ::: src/nautilus-batch-rename.c @@ +5,3 @@ + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or Version 2 of GPL @@ +48,3 @@ + GtkWidget *replace_box; + + Extra line @@ +70,3 @@ + + entry_text = g_strdup (gtk_entry_get_text (GTK_ENTRY (dialog->name_entry))); + replace_text = g_strdup (gtk_entry_get_text (GTK_ENTRY (dialog->replace_entry))); Leaking this one? Also, feel free to use g_autofree for this simple situations. @@ +100,3 @@ + /*g_hash_table_insert (dialog->view->details->pending_reveal, + file, + GUINT_TO_POINTER (FALSE));*/ you shouldn't need this, the files-changed handling of files view should do it's own thing. Maybe it needs some heuristics for multiple files changing in the files-view class itself though, like selecting the first one of the batch. @@ +248,3 @@ + GtkWidgetClass *widget_class = GTK_WIDGET_CLASS (klass); + + dialog_class->close = batch_rename_dialog_on_closed; why is this necessary? The dialog should do it automatically. ::: src/nautilus-file.c @@ +1531,3 @@ + NautilusFile *file; + + for(l = selection; l != NULL; l = l->next) { space after for @@ +1536,3 @@ + if (!nautilus_file_can_rename (file)) + return FALSE; + new line before return ::: src/nautilus-file.h @@ +267,3 @@ gboolean nautilus_file_can_execute (NautilusFile *file); gboolean nautilus_file_can_rename (NautilusFile *file); +gboolean nautilus_file_can_rename_files (GList *selection); This is taking a list of files, not a file. That means it doesn't go in this class. You have file-utilities.c for that. ::: src/nautilus-rename-utilities.c @@ +15,3 @@ + gchar *result; + + result = malloc (strlen (entry_text) + strlen (file_name) + 1); When this can be null? you have a hardcoded + 1 here (that I also don't know why you want it.) Also use g_malloc. @@ +19,3 @@ + return strdup (file_name); + } + sprintf (result, "%s%s", file_name, entry_text); g_printf, this is insecure for overflow as stated in the docs. @@ +46,3 @@ +{ + gchar *tok = NULL; + gchar *newstr = NULL; use '_' for separating words. Here would be new_str. Although I think we can find a better name. @@ +48,3 @@ + gchar *newstr = NULL; + gchar *oldstr = NULL; + gint skip_chars; spurious space @@ +62,3 @@ + skip_chars = 0; + + while ((tok = strstr (newstr + skip_chars, substr))) { I think here is better to use GString, which allow insertions in a more clean way. Also it's easier to implement. Also I came up with a nice trick :P. How about spliting the original string with the substring as a delimiter, and then create a new string just appending the original chunks of string + replacement for each chunk? Here's a pseudocode: new_string = g_string_new (""); splitted_string = g_strsplit (string, substr, 0); n_splits = g_strv_len (splitted_string); while (i < n_splits) { g_string_append (new_string, splitted_string [i]); /* The splits are managed in the nth-1 iteration */ if (i != n_splits -1) g_string_append (new_string, replacement); } @@ +88,3 @@ + gchar *file_name, + gchar *entry_text, + ...) don't like vargs :( Can we do something different?
Review of attachment 330786 [details] [review]: ::: src/nautilus-files-view.c @@ +6307,3 @@ + else + g_simple_action_set_enabled (G_SIMPLE_ACTION (action), + nautilus_file_can_rename_files (selection));//use nautilus_file_can_rename_files Don't put comments like this, use a new line. In any case the comment here is not needed, doesn't provide any more information. ::: src/nautilus-rename-utilities.c @@ +7,3 @@ +#include <stdarg.h> + +#define MAX_DISPLAY_LEN 40 oh no, we use pango ellipsize in the gtk label itself. The name shouldn't be affected because of the UI. @@ +114,3 @@ + +GList* +get_new_names_list (NautilusBatchRenameModes mode, enums are singular, so NautilusBatchRenameMode
Review of attachment 330771 [details] [review]: Oh this patch should be just the original code. The previous patch is just wrong doing the ".../0"
Review of attachment 330772 [details] [review]: This should be just original code, not a separate patch
Review of attachment 330773 [details] [review]: ::: src/nautilus-batch-rename-utilities.c @@ +1,3 @@ #include "nautilus-batch-rename.h" #include "nautilus-batch-rename-utilities.h" +#include "nautilus-files-view.h" why this include? @@ +176,3 @@ + result = NULL; + +{ it's not a double loop? Do you expect the names to be always ordered? I think it's better to be safe and do a double loop. Also you have the g_list_foreach, that might help here to avoid doing loops. @@ +180,3 @@ + file_name = strdup (nautilus_file_get_name (file)); + + GList *result; I'm not sure you should use the view here... but instead just the name of the file. This is creating a dependency to something completely unrelated, so now the batch renaming cannot be used with a different type of view or standalone on some random directory. @@ +182,3 @@ + if (strcmp (l1->data, file_name) != 0 && file_with_name_exists (view, l1->data) == TRUE) { + result = g_list_prepend (result, + NautilusFile *file; not need for casting to gpointer @@ +191,3 @@ + +gchar* + for (l1 = new_names, l2 = old_names; l1 != NULL && l2 != NULL; l1 = l1->next, l2 = l2->next) { g_strconcat ::: src/nautilus-batch-rename.c @@ +52,3 @@ GtkWidget *replace_box; + GList *listbox_rows;//adauga aici rows de la listbox pt a fi sterse // Vamos a la playa que calienta el sol, chiviriviri, poropopo :) @@ +196,3 @@ static void +activate_expander (GtkExpander *expander, + NautilusBatchRename *dialog) aligment @@ +203,3 @@ + + if (gtk_expander_get_expanded (GTK_EXPANDER (expander))) { + GValue width = G_VALUE_INIT; spurious new line @@ +204,3 @@ + if (gtk_expander_get_expanded (GTK_EXPANDER (expander))) { + + what does this set_width do and why 8? @@ +205,3 @@ + + g_value_set_int (&width, 8); + missing space in &width @@ +209,3 @@ + + g_value_set_int (&width, 5); + ditto @@ +260,3 @@ + +static void + gtk_widget_show (separator); I cannot see this listbox displayed, but I think we decided on a different design which marks them with some color right? @@ +267,3 @@ + + /* clear rows from listbox (if there are any) */ + } use {} when inside there is a {} @@ +283,3 @@ + * a new conflict */ + dialog->listbox_rows = g_list_prepend (dialog->listbox_rows, + GtkWidget *label; not need for gpointer casting. gpointer is just a pointer. @@ +322,3 @@ + + /* check if there is more than one conflict */ + gtk_widget_set_sensitive (dialog->rename_button, FALSE); use {} if else uses {} @@ +379,3 @@ + g_list_free (new_names); + g_list_free (duplicates); this is missing from a previous patch right? ::: src/nautilus-files-view.c @@ +1803,3 @@ +gboolean +file_with_name_exists (NautilusFilesView *view, I think you can do directly the directory_get_file_by_name in the client without need of this wrapper. OR in any case, this function should be in nautilus-directory itself. But as said, the renaming should be independent of the view. For instance, if you are renaming in a search this won't work. You need to check the actual parent directories and work directly with files rather than with the UI @@ +1813,3 @@ + return FALSE; + + here you are leaking the file. ::: src/resources/ui/nautilus-batch-rename-dialog.ui @@ +90,3 @@ </packing> </child> + spurious new line @@ +142,3 @@ </packing> </child> + spurious new line
Review of attachment 330774 [details] [review]: This patch is better if it's just original code. Is kinda hard to review changes that should be on the original code
Hey! So I realized that the patches should be merged, which makes the review harder. Let's talk over IRC about that and then I will continue reviewing :)
Created attachment 331113 [details] [review] Implement batch renaming
Hey Alex, Can you obsolete the patches that are not longer valid due to this last patch? Thanks!
Comment on attachment 330771 [details] [review] Limit the label size >From d6da798ef36d0435d517ba959c2aa6329b92bdbd Mon Sep 17 00:00:00 2001 >From: Alexandru Pandelea <alexandru.pandelea@gmail.com> >Date: Wed, 8 Jun 2016 22:52:28 +0300 >Subject: [PATCH] Limit the label size > >Use predefined label functions to limit the label size instead of >using memcpy to limit the label size. > >https://bugzilla.gnome.org/show_bug.cgi?id=768311 >--- > src/nautilus-batch-rename.c | 4 ++++ > src/nautilus-rename-utilities.c | 3 --- > 2 files changed, 4 insertions(+), 3 deletions(-) > >diff --git a/src/nautilus-batch-rename.c b/src/nautilus-batch-rename.c >index ec137cc..c3c2da3 100644 >--- a/src/nautilus-batch-rename.c >+++ b/src/nautilus-batch-rename.c >@@ -27,6 +27,7 @@ > > #define ADD_TEXT_ENTRY_SIZE 550 > #define REPLACE_ENTRY_SIZE 275 >+#define MAX_DISPLAY_LEN 40 > > struct _NautilusBatchRename > { >@@ -287,6 +288,9 @@ nautilus_batch_rename_new (NautilusFilesView *view) > > gtk_widget_grab_focus (dialog->name_entry); > >+ gtk_label_set_ellipsize (GTK_LABEL (dialog->error_label), PANGO_ELLIPSIZE_END); >+ gtk_label_set_max_width_chars (GTK_LABEL (dialog->error_label), MAX_DISPLAY_LEN); >+ > /* update display text */ > file_names_widget_entry_on_changed (dialog); > >diff --git a/src/nautilus-rename-utilities.c b/src/nautilus-rename-utilities.c >index c9ec0f6..6db7ccb 100644 >--- a/src/nautilus-rename-utilities.c >+++ b/src/nautilus-rename-utilities.c >@@ -159,8 +159,5 @@ get_new_display_name (NautilusBatchRenameModes mode, > > result = get_new_name (mode, file_name, entry_text, replace_text); > >- if (strlen (result) >= MAX_DISPLAY_LEN) >- memcpy (result + MAX_DISPLAY_LEN, "...\0",4); >- > return result; > } >\ No newline at end of file >-- >2.7.4
Review of attachment 331113 [details] [review]: Hey! another batch of reviews. so far is looking better. Be careful that I think some previous reviews were not applied, which kinda beats the point of reviewing. ::: src/nautilus-batch-rename-utilities.c @@ +18,3 @@ + +static gchar* +batch_rename_append (gchar *file_name, I think we are much better using GString to avoid handling the memory ourselves. This applies to almost all functions. @@ +95,3 @@ + } + + return new_name;*/ You can use a difference branch if you want to keep the old code, but the patches shouldn't have this @@ +118,3 @@ + i = 0; + + while (i < n_splits) { use for loops for a whole iteration of a list. @@ +133,3 @@ + gchar *file_name, + gchar *entry_text, + ...) no vargs please. @@ +141,3 @@ + + if (mode == NAUTILUS_BATCH_RENAME_REPLACE) { + superfluous new line @@ +199,3 @@ + gchar *file_name, + gchar *entry_text, + gchar *replace_text) the aligment should be var *var **var @@ +220,3 @@ + result = NULL; + + for (l1 = new_names, l2 = old_names; l1 != NULL && l2 != NULL; l1 = l1->next, l2 = l2->next) { no double loop? I think I already reviewed this one @@ +226,3 @@ + if (strcmp (l1->data, file_name) != 0 && file_with_name_exists (view, l1->data) == TRUE) { + result = g_list_prepend (result, + l1->data); you can use one line here @@ +268,3 @@ + gconstpointer b) +{ + NautilusFile *f1; don't use abreviations please. You can do file1 etc. @@ +298,3 @@ + gconstpointer b) +{ + return *(((CreateDateElem*) a)->position) - *(((CreateDateElem*) b)->position); this is slightly confusing, can you split it? @@ +311,3 @@ +nautilus_batch_rename_sort (GList *selection, + SortingMode mode, + ...) no vargs please @@ +385,3 @@ + gchar *query = "SELECT nfo:fileName(?file) nie:contentCreated(?file) WHERE { ?file a nfo:FileDataObject. "; + + filter1 = g_malloc (MAX_FILTER_LEN); don't use gmalloc for strings. Try to use GStrings or g_sprintf or so. Also what's the pourpose of MAX_FILTER_LEN? @@ +419,3 @@ + sparql, + NULL, + &error); why synchronous? wouldn't it be better to be async? We are in the main UI thread here @@ +421,3 @@ + &error); + + if (error) error || !cursor so you can avoid the next big if @@ +437,3 @@ + while (tracker_sparql_cursor_next (cursor, NULL, &error)) { + value = g_malloc (sizeof(int)); + *value = i++; declare as value, so we don't manage the memory. Then you can use &value when you need a pointer. @@ +463,3 @@ + +gboolean +nautilus_file_can_rename_files (GList *selection) I think I reviewed this one before. This should go to file-utilities.c ::: src/nautilus-batch-rename.c @@ +88,3 @@ + if (g_strcmp0 (target_name, "name-ascending") == 0) { + gtk_label_set_label (GTK_LABEL (dialog->numbering_order_label), + "Original name (Ascending)"); Shouldn't this be done in the .ui file? @@ +132,3 @@ + g_simple_action_set_state (action, value); + + g_signal_emit_by_name (dialog->numbering_order_popover, "closed"); why emitting this signal? If you want to close the popover you should do a explicit call to the buttons toggle. @@ +178,3 @@ + selection = dialog->selection; + + for (l1 = selection, l2 = new_names; l1 != NULL && l2 != NULL; l1 = l1->next, l2 = l2->next) { Is this in order? Just checking :) @@ +244,3 @@ + + gtk_label_set_ellipsize (GTK_LABEL (label_new), PANGO_ELLIPSIZE_END); + gtk_label_set_max_width_chars (GTK_LABEL (label_new), MAX_DISPLAY_LEN); Why do we need to set a max characters? It should just ellipsize if there is not enough space @@ +269,3 @@ + NautilusFile *file; + + /* clear rows from listbox (if there are any) */ An English trick, you can write: if there are any -> if any @@ +310,3 @@ + + if (duplicates != NULL) + single_conflict = (duplicates->next == NULL) ? TRUE:FALSE; (duplicates->next == NULL) ? TRUE:FALSE; is the same as: duplicates->next == NULL Also you have two if statements with this condition few lines underneath. You can merge them. On the other hand, is this boolean ever used? @@ +317,3 @@ + /* check if there are name conflicts and display them if they exist */ + if (duplicates != NULL) { + gtk_widget_set_sensitive (dialog->rename_button, FALSE); if you return here you are leaking all what you free at the end. @@ +338,3 @@ + +static void +batch_rename_dialog_on_closed (GtkDialog *dialog) I think I reviewed this one, shouldn't this be automatically handled? @@ +456,3 @@ + + gtk_widget_class_set_template_from_resource (widget_class, "/org/gnome/nautilus/ui/nautilus-batch-rename-dialog.ui"); + You are missing to handle _finalize where you will handle and free all the private data, right? @@ +488,3 @@ + +GtkWidget* +nautilus_batch_rename_new (NautilusFilesView *view) I think we discussed this and said to not have the view, rather just the directory and the selection as parameters @@ +495,3 @@ + gchar *dialog_title; + + dialog = g_object_new (NAUTILUS_TYPE_BATCH_RENAME,"use-header-bar",1, NULL); TRUE instead of 1 (there is actually disagreement about this with different developers :)) @@ +496,3 @@ + + dialog = g_object_new (NAUTILUS_TYPE_BATCH_RENAME,"use-header-bar",1, NULL); + I think you can do all of the next lines in _init @@ +505,3 @@ + gtk_widget_grab_focus (dialog->name_entry); + + gtk_label_set_ellipsize (GTK_LABEL (dialog->error_label), PANGO_ELLIPSIZE_END); shouldn't this and the next lines be in the .ui file? ::: src/nautilus-files-view.c @@ +1804,3 @@ +gboolean +file_with_name_exists (NautilusFilesView *view, I think we agreed on moving this to nautilus directory no? ::: src/resources/ui/nautilus-batch-rename-dialog.ui @@ +13,3 @@ + <property name="can_focus">True</property> + <property name="use_underline">True</property> + <signal name="clicked" handler="batch_rename_dialog_on_closed" swapped="yes" /> I think this should be handled automatically once you set the action-widgets, like in the new folder dialog
Created attachment 331623 [details] [review] Improve code This commit applies the changes mentioned in the review
Created attachment 332954 [details] [review] Implement batch renaming
Review of attachment 332954 [details] [review]: Just a quick look before retiring to bed. ::: src/nautilus-batch-rename-utilities.c @@ +12,3 @@ #define MAX_DISPLAY_LEN 40 #define MAX_FILTER_LEN 500 +#define MAX_FILE_LEN 1000 Is this referring to the maximum file length? The majority of current filesystems only do 255 bytes. @@ +945,3 @@ + file_name); + + metadata = g_malloc (9 * sizeof (GString*)); This should be “g_malloc (sizeof (FileMetadata))”. @@ +973,3 @@ (GDestroyNotify) g_free); + data = g_malloc (sizeof (NautilusBatchRename*) + g_malloc (sizeof (QueryData)) @@ +983,3 @@ + data->have_season = TRUE; + data->have_creation_date = TRUE; + data->have_season = TRUE; “have_season” set twice ::: src/nautilus-file.c @@ +1654,3 @@ (op->file->details->operations_in_progress, op); + + if (g_list_length (op->files) != 0) { This is unnecessary. @@ +1671,3 @@ nautilus_file_operation_remove (op); + + if (g_list_length (op->files) == 0) “if (op->files == NULL)” is faster (that is what the documentation says for this function). ::: src/nautilus-files-view.c @@ -3945,3 @@ unschedule_display_of_pending_files (view); schedule_timeout_display_of_pending_files (view, UPDATE_INTERVAL_MIN); - Don’t create noise by removing empty lines.
(In reply to Ernestas Kulik from comment #30) > Is this referring to the maximum file length? The majority of current > filesystems only do 255 bytes. Err, file/name/ length.
Created attachment 333013 [details] [review] Implement batch renaming
(In reply to Ernestas Kulik from comment #30) > Review of attachment 332954 [details] [review] [review]: Thanks for taking a look, I updated the patch :)
(In reply to Alexandru Pandelea from comment #33) > Thanks for taking a look, I updated the patch :) Hey, thanks for all the hard work! …and sorry for piling on with such trivialities when things are the most hectic.
Created attachment 333078 [details] [review] Implement batch renaming
(In reply to Ernestas Kulik from comment #34) > (In reply to Alexandru Pandelea from comment #33) > > Thanks for taking a look, I updated the patch :) > > Hey, thanks for all the hard work! …and sorry for piling on with such > trivialities when things are the most hectic. np, those things had to be changed
Review of attachment 333078 [details] [review]: This is first review, I still need to review the shadowing of the rows, and then nautilus-file-undo-operations, but my battery is dying and I want you to not keep waiting for me to start working :) I will review that this afternoon. As a general comment, before merging run valgrind and make sure there are no leaks in the your code. General issues I found: When going from "it's not going to be a unique name" to "the file would conflict" all the rows has the color of conflicts. You can test with files 1, 2, 3, test1 and try to rename the first three files to test[1,2,3]. ::: src/nautilus-batch-rename-utilities.c @@ +10,3 @@ +#include <eel/eel-vfs-extensions.h> + +#define MAX_FILE_LEN 1000 As said in person don't do this. @@ +14,3 @@ +typedef struct { + NautilusFile *file; + gint *position; not need to be a pointer, right? @@ +44,3 @@ +static GString* +batch_rename_replace (gchar *string, + gchar *substr, don't make abreviations @@ +59,3 @@ + } + + if (g_strcmp0 (substr, "") == 0) { g_utf8_strlen() it's faster. @@ +87,3 @@ + +static GString* +escape_ampersand (const gchar *string) g_markup_escape_text @@ +180,3 @@ + if (g_strcmp0 (metadata, "creation_date") == 0 && + file_metadata->creation_date != NULL && + g_strcmp0 (file_metadata->creation_date->str, "")) if (-1) is not equal than if (1) You can use string->len != 0, which doesn't do any computation. @@ +230,3 @@ + + eel_filename_get_rename_region (file_name, + &start_offset, &end_offset); AS said not needed to use this, but rather the other function in eel. @@ +238,3 @@ + added_tag = FALSE; + + if (!added_tag && g_strcmp0 (tag->str, "[Original file name]") == 0) { Can you use constants? ORIGINAL_FILE_NAME_TAG = "[Original file name]" @@ +267,3 @@ + if (count < 10) + g_string_append_printf (new_name, "00%d", count); + else multiple lines uses {} @@ +288,3 @@ + + if (metadata != NULL) { + splitted_date = g_strsplit (metadata, "T", -1); use g_date and convert to the local format @@ +297,3 @@ + } + + if (!added_tag && g_strcmp0 (tag->str, "[Season nr]") == 0) { nr it's not an English abreviation I think. But in any case don't use abbreviations, neither in the UI or the code. @@ +339,3 @@ + if (g_strcmp0 (new_name->str, "") == 0) + new_name = g_string_append (new_name, file_name); + else Use {} if the oneliner actually has a hierarchy inside. @@ +352,3 @@ + GList *selection_metadata, + gchar *entry_text, + gchar *replace_text) too many spaces for identation @@ +374,3 @@ + if (mode == NAUTILUS_BATCH_RENAME_FORMAT) { + result = g_list_prepend (result, + batch_rename_format (file, split this assigning to a variable, it's going to be more clear. @@ +386,3 @@ + replace_text)); + + g_string_erase (file_name, 0, -1); this is slow, instead if you want to reuse the variable create a new string everytime @@ +397,3 @@ +/* If there is a file that generates a conflict, there is a case where that isn't + * actually a conflict. This case is when the file that generates the conflict is + * in the selection and this file changed it's name */ The wording can be improved. How about: There is a case that a new name for a file conflicts with an existing file name in the directory but it's not a problem because the file in the directory that conflicts is part of the batch renaming selection and it's going to change the name anyway. @@ +432,3 @@ + } + + l2 = l2->next; put it inside the for loop l1 = l1->next, l2 = l2->next; @@ +442,3 @@ + +static void +got_files_callback (NautilusDirectory *directory, GList *files, gpointer callback_data) Don't use a one liner and also how about the name "on_call_when_ready" @@ +450,3 @@ + +GList* +list_has_duplicates (NautilusBatchRename *dialog, file_names_list_has_duplicates is a better name I Think. One thing I don't understand is why we pass a cancellable, if this is not async or doesn't have the format of an async function that receives a callback or so. For the cancellable here with the current code, in the worst case we actually not even use it, that is when we dont have the same parent and we need to go to the disk to check. I think what is needed is to pass the cancellable as data to got_files_callback, and make sure is not cancelled. And If not we will have crashes if the directory or the dialog goes away in the meantime. Idelly we would use nautilus_directory_cancel_callback once we know it's cancelled, so you can add a weak reference (g_weak_reference) to the dialog and in case it's destroyed cancel the call_when_ready. So first, I will convert it to an async operation with a callback as a parameter. In this way the client knows that it will take time to process, and we will return the duplicates in the appropiate time, not a NULL list if the parent are not the same. In this way you will avoid also the whole parents_check management in bath-rename, and rather do it here. @@ +451,3 @@ +GList* +list_has_duplicates (NautilusBatchRename *dialog, + NautilusDirectory *model, directory. I never liked the word model for this. @@ +455,3 @@ + GList *selection, + GList *parents_list, + gboolean same_parent, same_parent would be the same as checking if parents_list == NULL no? @@ +464,3 @@ + gboolean is_renameable_desktop_file, have_conflict; + gchar *name; + I would add a check for the lenght of the lists at least. If not nautilus will crash. Something like g_return_val_if_fail (g_list_length (new_names) == g_list_length (selection), NULL)); @@ +470,3 @@ + file_name2 = g_string_new (""); + + if (!same_parent) { Would be good to have a comment here why we need all of this. @@ +487,3 @@ + l3 = selection; + + for (l1 = new_names; l1 != NULL; l1 = l1->next) { if new names and selection are expected in the same order and they just have to go forward one by one, do it in the same for condition. @@ +490,3 @@ + if (g_cancellable_is_cancelled (cancellable)) { + g_list_free_full (result, g_free); + g_list_free_full (new_names, string_free); The GList passed as parameter should have the ownership on the caller, not here. You should never free it, and instead let the caller free it. In case you need to keep it alive in here, copy it. @@ +494,3 @@ + } + + file1 = NAUTILUS_FILE (l3->data); file1 = l3 it's pretty confusing. Since this function is pretty long, try to use more explanatory names, and try to initialize them at the same time, like in the start of the for. @@ +500,3 @@ + have_conflict = FALSE; + + if (!is_renameable_desktop_file && strstr (new_name->str, "/") != NULL) { g_strcmp0 @@ +507,3 @@ + + name = nautilus_file_get_name (file1); + g_string_erase (file_name1, 0, -1); g_string_free. There are multiple of this in the file. @@ +521,3 @@ + g_string_erase (file_name2, 0, -1); + g_string_append (file_name2, name); + g_free (name); just use g_string_new (name), although for what you are doing I'm not sure you need to use GString at all, I don't think you need to allocate and free memory here, just use g_strcpm0 (name, new_name); This is actually that happens in the whole function. @@ +524,3 @@ + + if (g_string_equal (new_name, file_name2) && + !file_name_changed (selection, new_names, new_name, NULL)) { I think this function actually means file_name_conflicts_with_results? If I understood correctly you are checking duplicates with the new name and the result of the other names in the batch renaming. I think we can find a more explanatory name. @@ +525,3 @@ + if (g_string_equal (new_name, file_name2) && + !file_name_changed (selection, new_names, new_name, NULL)) { + result = g_list_prepend (result, strdup (new_name->str)); g_strdup, there are multiple of this in the file @@ +533,3 @@ + + /* check with files that will result from the batch rename, unless + * this file already has a conflict */ I think we actually did that in the file_name_changed just before? @@ +536,3 @@ + if (!have_conflict) + for (l2 = new_names; l2 != NULL; l2 = l2->next) { + file_name3 = l2->data; file_name3 it's not a really explaining name. How about instead of using numbers say what do they represent? In this case new_file_name maybe? @@ +539,3 @@ + + if (l1 != l2 && g_string_equal (new_name, file_name3)) { + result = g_list_prepend (result, strdup (new_name->str)); g_strdup @@ +546,3 @@ + } + + l3 = l3->next; put it in the for loop @@ +554,3 @@ + + if (!g_cancellable_is_cancelled (cancellable)) + g_list_free_full (new_names, string_free); actually if it' cancelled you still need to clean up no? In any case, as said before, a cancellable here doesn't make much sense, since it's not an async operation (but it makes sense for the call_when_ready) @@ +631,3 @@ + elem2 = (CreateDateElem*) b; + + return *(elem1->position) - *(elem2->position); As said before there is not need to use pointers for integers @@ +650,3 @@ +nautilus_batch_rename_sort (GList *selection, + SortingMode mode, + GHashTable *hash_table) the name hash_table doesn't really help me to understand what does it @@ +654,3 @@ + GList *l,*l2; + NautilusFile *file; + GList *createDate_list, *createDate_list_sorted; Don't use mix of CamelCase and underscores @@ +677,3 @@ + for (l = selection; l != NULL; l = l->next) { + CreateDateElem *elem; + elem = g_malloc (sizeof (CreateDateElem)); g_new, so we avoid the sizeof() part @@ +701,3 @@ + } + + g_list_free (createDate_list); as far as I can see you are leaking the createdDateElem objects. Use g_list_free_full. @@ +743,3 @@ + g_clear_error (&error); + g_clear_object (&cursor); + spurious line @@ +745,3 @@ + + + query_finished (data->dialog, data->hash_table, data->selection_metadata); for this, first the name has to contain the prefix of the class, so it would be like nautilus_batch_rename_query_finished, like we do for any public API. But in any case this looks too tied in the code. Nonone apart of batch-rename class could use this function. But there is a reason why this is a utility class separated from the batch-rename class, functions here should be kinda independent. I would use the usual pattern we use for chained async operations, that means providing a callback in the user_data and call the callback here. @@ +760,3 @@ + if (data->have_creation_date){ + value = g_malloc (sizeof(int)); + *value = g_hash_table_size (hash_table); So we can use GINT_TO_POINTER @@ +763,3 @@ + + g_hash_table_insert (hash_table, + strdup (tracker_sparql_cursor_get_string (cursor, 0, NULL)), aligment @@ +767,3 @@ + } + } + file_name = tracker_sparql_cursor_get_string (cursor, 0, NULL); For the column use enums, and for the query use constants like g_srtvprintf (query, METADATA_WHATEVER, METADATA_WHATEVER2...) @@ +786,3 @@ + metadata->creation_date = NULL; + } + } else { if (have->creation_date) { if(tracker_sparql_cursor_get_string (cursor, 1, NULL) == NULL) {} else {} } And same for the rest of the file @@ +808,3 @@ + } + + /* season nr */ as said before don't use abreviations, this repeats along the file @@ +810,3 @@ + /* season nr */ + if (data->have_season && tracker_sparql_cursor_get_string (cursor, 3, NULL) == NULL) { + data->have_season = FALSE; shouldn't this be possible with data->season != NULL? @@ +876,3 @@ + +static void +query_callback (GObject *object, query_callback is too abstract, we have many queries around. Use something more specific like on_tracker_sparql_connection_query. @@ +897,3 @@ + g_error_free (error); + + query_finished (data->dialog, data->hash_table, data->selection_metadata); same here as said before @@ +995,3 @@ + +gboolean +selection_has_single_parent (GList *selection) move it to nautilus-file-utilities and use nautilus_file_is_recent and nautilus_file_is_search, which are the ones that have different parents. ::: src/nautilus-batch-rename.c @@ +1,1 @@ +/* nautilus-batch-rename.c Since this is actually a dialog, I would rename this to batch-rename-dialog, including the class, functions, etc. You can use tools for batch renaming a class :) @@ +38,3 @@ + + GtkWidget *cancel_button; + GtkWidget *left_listbox; In general give more explanatory names, like original_name_listbox, arrow_listbox, result_listbox, etc. but there are more. Try to provide them a name that explain what do they do. @@ +82,3 @@ + + /* check if all files in selection have the same parent */ + gboolean same_parent; You could delete this and check with the function we said before. In general I feel this class has too many private stuff for dealing with batch-rename-utilities missing some part. I think we can do that better (although I know I'm speaking too abstract now, but an example is this same_parent priv data) @@ +83,3 @@ + /* check if all files in selection have the same parent */ + gboolean same_parent; + /* the number of the currently selected conflict */ number -> index? @@ +91,3 @@ + GList *duplicates; + GList *distinct_parents; + GTask *conflict_task; it should be plural, "conflicts" right? @@ +93,3 @@ + GTask *conflict_task; + GCancellable *conflict_cancellable; + gboolean checking_conflicts; probably you can remove this once you modify the functions to check for conflicts to have a proper async operation pattern with callback. Then you just will need to cancel whatever is going on with the cancellable before starting a new conflicts checking. @@ +97,3 @@ + /* starting tag position, -1 if tag is missing and + * -2 if tag can't be added at all */ + gint original_name_tag; use a boolean for saying if it's available or not, although I understand the reasoning behind, giving to the same variable different meanings is confusing. The same for checking if it's added or not. @@ +131,3 @@ + if (g_strcmp0 (target_name, "name-ascending") == 0) { + gtk_label_set_label (GTK_LABEL (dialog->numbering_order_label), + "Original name (Ascending)"); Needs to be translatable (this is actually needed on all the strings). For making it translatable use _("string") @@ +182,3 @@ + + /* update display text */ + file_names_widget_entry_on_changed (dialog); create a helper function instead of calling the _on_. then call the helper functions from here and from the _on_ @@ +194,3 @@ + + dialog = NAUTILUS_BATCH_RENAME (user_data); + cursor_pos = g_malloc (sizeof (int)); Don't allocate memory manually, use &cursor_pos when a @@ +202,3 @@ + strlen ("[Original file name]"), + cursor_pos); + *cursor_pos += strlen ("[Original file name]"); leftover @@ +203,3 @@ + cursor_pos); + *cursor_pos += strlen ("[Original file name]"); + And you are missing to actually set the cursor position in the entry @@ +232,3 @@ + + dialog = NAUTILUS_BATCH_RENAME (user_data); + action_name = g_malloc (strlen ("add-numbering-tag-zero")); OMG :D g_action_get_name. @@ +233,3 @@ + dialog = NAUTILUS_BATCH_RENAME (user_data); + action_name = g_malloc (strlen ("add-numbering-tag-zero")); + cursor_pos = g_malloc (sizeof (int)); the same as before, don;t allocate memory manually. @@ +302,3 @@ + + dialog = NAUTILUS_BATCH_RENAME (user_data); + action_name = g_malloc (strlen ("add-numbering-tag-zero")); The same as before. @@ +327,3 @@ + + add_numbering_action = g_action_map_lookup_action (G_ACTION_MAP (dialog->action_group), + "add-numbering-tag-zero"); identation @@ +343,3 @@ + +const GActionEntry dialog_entries[] = { + { "numbering-order-changed", NULL, "s", "'name-ascending'", numbering_order_changed }, changed is for signals. The action can be numbering-order. @@ +405,3 @@ + gconstpointer b) +{ + return *(int*)a - *(int*)b; *(int *)a - *(int *)b but instead split this into variables to make it more readable. Something like int *tag1 = (int *)a; @@ +410,3 @@ +static GList* +split_entry_text (NautilusBatchRename *dialog, + gchar *entry_text) In general the functions looks pretty hacky. If I understood correctly the code it tries to handle the case where the user deletes part of the tag so we delete the whole tag? In that case, a better approach is to track the "borders" of the tag, with the first and last position, and delete the whole tag if the user's cursor makes a delete in that position. For that you can use for example a list of structs that has the information of the current tags and "border"s of the tags. Also I would name it as get_tags_from_text or something like that. @@ +412,3 @@ + gchar *entry_text) +{ + GString *string, *tag; one declaration per line. This is actually something to change in several places through the patch. @@ +461,3 @@ + } + + g_array_sort (tag_positions, compare_int); This is a little fragile way to keep the order. Also it's not clear what tag comes before the other, and changing @@ +468,3 @@ + tag = g_string_append_len (tag, + entry_text + g_array_index (tag_positions, gint, i), + 3); what's this? :/ why 3? this looks pretty hacky... What you want to do? If you wanted to check wheter it's a tag or another @@ +474,3 @@ + string = g_string_append_len (string, + entry_text + j, + g_array_index (tag_positions, gint, i) - j); what is "j"? The name of the variable doesn't really help. @@ +550,3 @@ + + if (dialog->mode == NAUTILUS_BATCH_RENAME_REPLACE) { + result = get_new_names_list (dialog->mode, I think I revised this in another part, but use the prefix for the classes (at least in the case for batch renaming, since it's very specific) @@ +557,3 @@ + replace_text); + } else { + /* get list of tags and regular text */ Is this comment in general? It's probably confusing because of the name of the function "split_entry_text" which makes it unclear @@ +623,3 @@ + gtk_label_set_ellipsize (GTK_LABEL (label_old), PANGO_ELLIPSIZE_END); + + gtk_list_box_row_set_selectable (GTK_LIST_BOX_ROW (row), TRUE); this is by default already, do you need to set it? @@ +634,3 @@ + +static void +update_rows_height (NautilusBatchRename *dialog) This has two problems, if the height is lower than before it doesn't set it because it's checking only the maximum without reseting it. Also, get_allocation is not the height that the widgets wants to get, but what it takes, which is also affected with height-request. So it's kinda useless in that case. Use get_preferred_height instead. Also move the function so all the create_* are together. Also explain why this function is necessary (the inefficiency of gtksizegroup) in a comment to the function. @@ +756,3 @@ + if (dialog->checking_conflicts) { + dialog->rename_clicked = TRUE; + return; Not doing anything when the user clicks the rename because nautilus is checking on the disk whether there are conflicts is quite meh. I would show a spinner, but we can do that later. @@ +763,3 @@ + + /* if names are all right call rename_files_on_names_accepted*/ + rename_files_on_names_accepted (dialog, dialog->new_names); use a helper function instead of the signal pattern *_on_* @@ +790,3 @@ + + /* add rows to a list so that they can be removed when the renaming + * result changes */ is there a possibility one will be NULL and the other doesn't? In any case, this would crash if one of them is null and the other is not, because you are using ||instead of && @@ +815,3 @@ + +static gboolean +file_has_conflict (NautilusBatchRename *dialog, Don't need this, use g_list_find_custom with a g_strcmp0 in the CompareFunc @@ +833,3 @@ + GList *l, *l2; + GString *file_name, *display_text, *new_name; + gint n, nth_conflict, name_occurances; ocurrences @@ +851,3 @@ + l2 = dialog->selection; + + for (l = dialog->new_names; l != NULL; l = l->next) { I would put this for in a helper function, find_conflict_index or so. @@ +860,3 @@ + if (g_strcmp0 (new_name->str, name) && + g_string_equal (file_name, new_name) && + nth_conflict == 0) if the name is equal and nth_conflict is 0 break? Is it not enough if nth_conlflict == 0? @@ +869,3 @@ + + n++; + l2 = l2->next; all of them into the for loop condition @@ +889,3 @@ + adjustment = gtk_scrolled_window_get_vadjustment (GTK_SCROLLED_WINDOW (dialog->scrolled_window)); + gtk_widget_get_allocation (GTK_WIDGET (l->data), &allocation); + gtk_adjustment_set_value (adjustment, (allocation.height + 1)*n); + 1? also why using the allocation for that when you have the "y" position of the widget? @@ +891,3 @@ + gtk_adjustment_set_value (adjustment, (allocation.height + 1)*n); + + if (strstr (file_name->str, "/") == NULL) { are you trying to check whether the name is correct or not? You will have this along all the files. I would display a warning in a different label, under the GtkEntry and definitely not doing it here. You can use also the validate_file_name function in files view that validates file names, which is more complex than this and we use it for the single rename dialog. For that you can move the function to nautilus-file-utilities. @@ +917,3 @@ + +static void +move_next_conflict_down (NautilusBatchRename *dialog) select_next_conflict_down, same for the other function to go up @@ +960,3 @@ + file_name = g_string_new (l1->data); + + /* the index of the nth conflict in the listbox */ if you need to put a comment to a simple variable like this is probably because the name of the variable is not clear. And I agree that "n" it's not clear enough :) You can use something like nth_conflict_index or something like that. @@ +971,3 @@ + name = nautilus_file_get_name (file); + /* g_strcmp0 is used for not selecting a file that doesn't change + * it's name */ I have the feeling this code is repeated. Why not create a helper function to get the actual conflicts and then you pass along that list to select_nth_conflict and add_conflict_rows_background? @@ +1017,3 @@ + gtk_style_context_remove_class (context, "conflict-row"); + + pos = g_list_position (dialog->listbox_rows_left, l); this is pretty slow, why not setting l2 in the loop iteration like the code does in other places? @@ +1077,3 @@ + dialog->conflicts_number = g_list_length (dialog->duplicates); + + add_conflict_rows_background (dialog); and you have to remove the previous ones, no? I would honestly use a single function that iterates and updates the background color for all. In this way you avoid to do two iterations on the listsbox, one to remove the class and one to add the class @@ +1109,3 @@ + +void +check_conflict_for_file (NautilusBatchRename *dialog, this is similar or does part of what file_has_conflict in this file does no? It's sligtlhy confusing, or I have the feeling it's repeating code @@ +1171,3 @@ +list_has_duplicates_callback (GObject *object, + GAsyncResult *res, + gpointer user_data) aligment @@ +1227,3 @@ + dialog->conflict_task = g_task_new (dialog, dialog->conflict_cancellable, callback, user_data); + + g_task_set_priority (dialog->conflict_task, io_priority); not need for this, let's use the default io priority @@ +1234,3 @@ + +static gint +check_tag (NautilusBatchRename *dialog, check tag regarding what? if it is correct or...? The name is not very explanatory @@ +1277,3 @@ + entry_text = g_string_new (gtk_entry_get_text (GTK_ENTRY (dialog->name_entry))); + + if ((g_strrstr (entry_text->str, "[1, 2, 3]") || As said before I would use some kind of list of tags that are currently used, with the borders of it in the entry etc. This pattern matching is not the most reliable stuff... imagine that we can even change a character there, we will make all stop working. @@ +1384,3 @@ + strlen (tag_name) - 1); + + /* if only a paranthesis was deleted, then remove the rest of the tag */ actually as said before whatever delete event that happens inside a tag should remove the tag, so the code and comment needs to be changed @@ +1404,3 @@ + g_string_free (tag_part, TRUE); + + return return_value; return value is not the most explanatory variable name @@ +1549,3 @@ + +static void +numbering_order_button_clicked (NautilusBatchRename *dialog) shouldn't setting the popover to the button and toggling the button enough to close/open the popover? @@ +1564,3 @@ + +void +query_finished (NautilusBatchRename *dialog, The name it's not very clear about what query finished @@ +1575,3 @@ + g_hash_table_destroy (hash_table); + + if (hash_table == NULL || g_hash_table_size (hash_table) == 0) this is going to crash if the previous condition is true. Destroying all keys and values and freeing the hash table, but here you check for the size. Maybe you wanted to set as NULL? And if it was 0 size, it would enter the previous condition for sure. @@ +1584,3 @@ + "dialog.numbering-order-changed"); + + g_menu_item_set_attribute (first_created, "target", "s", "first-created"); you can set it inside the detailed action like "dialog.numbering-order-changed('first-created')" instead of setting this. Take a look at files-view for this. @@ +1881,3 @@ + files_nr++; + + dialog_title = g_malloc (DIALOG_TITLE_LEN); dont limit the dialog, and don't use manual allocated memory.
Review of attachment 333078 [details] [review]: A little more ::: src/nautilus-batch-rename.c @@ +1650,3 @@ + GtkStateFlags flags; + + if (!GTK_IS_LIST_BOX_ROW (row)) g_return_if_fail would be more appropiate, but it looks like this shouldn't happen internally on the first place. Are you sure this is possible? If so, can we avoid it? @@ +1666,3 @@ + +static gboolean +leave_event_callback (GtkWidget *widget, on_leave_event @@ +1684,3 @@ + +static gboolean +motion_callback (GtkWidget *widget, on_motion @@ +1768,3 @@ + + if (dialog->checking_conflicts) { + g_cancellable_cancel (dialog->conflict_cancellable); and I think you have to unref it right? @@ +1887,3 @@ + nautilus_batch_rename_initialize_actions (dialog); + + dialog->same_parent = selection_has_single_parent (dialog->selection); should this be enough with dialog->distinct_parents != NULL? @@ +1932,3 @@ + + gtk_label_set_ellipsize (GTK_LABEL (self->conflict_label), PANGO_ELLIPSIZE_END); + gtk_label_set_max_width_chars (GTK_LABEL (self->conflict_label), 1); 1? I'm not sure I understand. ::: src/nautilus-file-utilities.c @@ +1151,3 @@ +gboolean +nautilus_file_can_rename_files (GList *selection) *files
Review of attachment 333078 [details] [review]: And last review! :) One thing I noticed is that when the names of the files are long the dialog expands. This shouldn't happen, but rather the dialog shouldn't be resizable and the labels should ellipsize. Good to see this job being done! (although there is work to be done of course, but that was expected) Amazing :) ::: src/nautilus-file-private.h @@ +217,3 @@ + GList *files; + gint renamed_files; + gint skipped_files; is this really necessary here? I think we shouldn't need to modify the nautilus-file data for an operation in a list of files. ::: src/nautilus-file.c @@ +1918,3 @@ + GList *new_names, + NautilusFileOperationCallback callback, + gpointer callback_data) most of this code is repeated with real_rename. Create a helper function for the common code paths. @@ +1958,3 @@ + is_desktop_file (file) && can_rename_desktop_file (file); + + /* Set up a batch renaming operation. */ why those "-"? @@ +2107,3 @@ +void +nautilus_file_batch_rename (GList *files, it's really strange to have this operation here... I know we discussed why it should be here, but maybe we can put this on file-operations and call public API of nautilus-file for renaming single files? if the problem is the undo operation, we can do something like creating two renaming functions in nautilus-file, one with undo and one without undo, sharing most of the code. ::: src/nautilus-files-view.h @@ +368,3 @@ GtkWidget* nautilus_files_view_get_content_widget (NautilusFilesView *view); +gboolean file_with_name_exists (NautilusFilesView *view, public API uses the prefix of the class, as all the other functions.
Created attachment 333941 [details] [review] Implement batch renaming
Review of attachment 333941 [details] [review]: You have some trailing whitespaces, git actually warns about this. You can use a diff tool like gitg to see the whitespaces. Will continue tomorrow with more ::: src/nautilus-batch-rename-dialog.c @@ +31,3 @@ +#define ADD_TEXT_ENTRY_SIZE 550 +#define REPLACE_ENTRY_SIZE 275 +#define TAG_UNAVAILABLE -2 nothing here is necessary anymore right? @@ +32,3 @@ +#define REPLACE_ENTRY_SIZE 275 +#define TAG_UNAVAILABLE -2 +#define HAVE_CONFLICT 1 you can use a boolean directly, not this for this. @@ +116,3 @@ + gboolean set; + gint position; +} TagData; much better :D @@ +391,3 @@ + { "add-numbering-tag-zero", add_numbering_tag }, + { "add-numbering-tag-one", add_numbering_tag }, + { "add-numbering-tag-two", add_numbering_tag }, you can use a single action with a integer target. It's going to be more flexible and clean. @@ +418,3 @@ + index = gtk_list_box_row_get_index (listbox_row); + + if (GTK_WIDGET (box) == dialog->original_name_listbox) { This is fine, but just for your information, for comparing is not need to cast, since you just compare pointers. @@ +446,3 @@ +} + +gint compare_int (gconstpointer a, new line, and also use a more specific name like compare_tag_position or so @@ +546,3 @@ + tag = g_string_new (""); + + string = g_string_new (""); use a better name, I don't know what string is. @@ +549,3 @@ + + string = g_string_append_len (string, + entry_text + tag_end_position, g_utf8_substring @@ +558,3 @@ + if (g_array_index (tag_positions, gint, i) == tag_data->position && tag_data->set) { + tag_end_position = g_array_index (tag_positions, gint, i) + + strlen (ORIGINAL_FILE_NAME); g_utf8_strlen @@ +563,3 @@ + + tag_data = g_hash_table_lookup (dialog->tag_info_table, NUMBERING); + if (g_array_index (tag_positions, gint, i) == tag_data->position && tag_data->set) { interchange the order, there is no point to check the position if it was not set. @@ +628,3 @@ + } + string = g_string_new (""); + string = g_string_append (string, entry_text + tag_end_position); use directly g_string_new (whatever). This happens more times along the patch. @@ +631,3 @@ + + if (g_strcmp0 (string->str, "")) + result = g_list_prepend (result, string); Why this? cannot return NULL to indicate it's empty? @@ +683,3 @@ + +static void +begin_batch_rename_dialog (NautilusBatchRenameDialog *dialog, begin_batch_rename @@ +688,3 @@ + GList *l1; + GList *l2; + GList *l3; It gets confusing with this names, try to use somethign more specific when possible. @@ +699,3 @@ + new_file_name = l1->data; + + for (l3 = dialog->selection; l3 != NULL; l3 = l3->next) { First add a comment before the for explaining what are we going to do and why. Together with better variable names should be enough to get what this is doing. @@ +749,3 @@ +} + +/* This is manually done instead of using GtkSizeGroup because of the complexity of computation complexity. Withouth the "computation" looks like it's complex more than it costs. @@ +756,3 @@ + GList *l; + gint minimum_height; + gint natural_height; current_row_natural_height @@ +757,3 @@ + gint minimum_height; + gint natural_height; + gint new_maximum_height; maximum_height @@ +767,3 @@ + &natural_height); + + if (minimum_height > new_maximum_height) { we use the natural height, not the minimum height, since we have the scroll window. @@ +821,3 @@ + g_object_set_data (G_OBJECT (row), "show-separator", GINT_TO_POINTER (show_separator)); + + label_old = g_object_new (GTK_TYPE_LABEL, use normal API rather than this generic g_object_new. Although I understand is convenient, it's meh to use it in this way. @@ +840,3 @@ +static GtkWidget* +create_result_row_for_label (NautilusBatchRenameDialog *dialog, + const gchar *new_text, this function does the same as the previous one, except that one is added into a different list. I would say merge them and add it manually to a list on the caller. @@ +952,3 @@ + + /* add rows to a list so that they can be removed when the renaming + * result changes */ I think there is not need for this comment, is clear we want to track them @@ +979,3 @@ + dialog->arrow_listbox_rows = g_list_reverse (dialog->arrow_listbox_rows); + dialog->result_listbox_rows = g_list_reverse (dialog->result_listbox_rows); + dialog->listbox_labels_old = g_list_reverse (dialog->listbox_labels_old); why reversing this here, even if this function doesn't touch them?
Review of attachment 333941 [details] [review]: ok reviewed batch-rename-dialog :) going for batch-rename-utilities now ::: src/nautilus-batch-rename-dialog.c @@ +996,3 @@ + GtkAdjustment *adjustment; + GtkAllocation allocation; + ConflictData *data; use more specific names, conflict_data @@ +1003,3 @@ + + /* the conflict that has to be selected */ + file_name = g_string_new (data->name); more specific names, conflict_file_name @@ +1105,3 @@ + } + + for (l = dialog->duplicates; l != NULL; l = l->next) { I think we can make this for outside of the outter for, so we can avoid the n^2 complexity. I was thinking about having a for to remove all the background, then another for to add the background of those that have conflicts. @@ +1141,3 @@ + } + + for (l1 = dialog->selection, l2 = dialog->listbox_labels_old; l1 != NULL && l2 != NULL; l1 = l1->next, l2 = l2->next) { couldn't we avoid doing a second for loop here and merge it with the previous one? @@ +1150,3 @@ + gtk_label_set_label (label, old_name); + } else { + new_name = batch_rename_dialog_replace_label_text (old_name, this has the wrong class sufix. It should be named batch_rename_replace_label_text (although I don't understand the name of this function) @@ +1193,3 @@ + /* if the rename button was clicked and there's no conflict, then start renaming */ + if (dialog->rename_clicked && dialog->duplicates == NULL) { + batch_rename_dialog_on_response (dialog, GTK_RESPONSE_OK, NULL); I think I already review this one. Use a helper function. The pattern *_on_* is for signals, and here you don't have any. Also, sending GTK_RESPONSE_OK form there looks fishy. @@ +1203,3 @@ + +void +check_conflict_for_file (NautilusBatchRenameDialog *dialog, in plurarl no? check conflicts_for_files @@ +1236,3 @@ + (GDestroyNotify) g_free, + (GDestroyNotify) g_free); + a comment before every loop will help to know what is doing @@ +1239,3 @@ + for (l1 = dialog->new_names, l2 = dialog->selection; + l1 != NULL && l2 != NULL; + l1 = l1->next, l2 = l2->next) { this loop is pretty confusing to me, I think we can make it much simpler or readable. Let's discuss it over IRC @@ +1246,3 @@ + table_parent_uri = g_hash_table_lookup (new_names_table, new_name->str); + + if (g_strcmp0 (parent_uri, current_directory) == 0) if the parent of one of the files is not the current_directory, that means is an artificial directory like recent:// or search no? In that case every file has a different parent than the current_directory. So I guess it's enough checking for this before the loop? Or is this intended to check all files with a single parent? @@ +1285,3 @@ + if (g_strcmp0 (parent_uri, current_directory) == 0 && + !g_string_equal (new_name, file_name)) { + exists = GPOINTER_TO_INT (g_hash_table_lookup (directory_files_table, new_name->str)); in the previous loop you insert them and unconditionally put as HAVE_CONFLICT no? why do we check it now this? @@ +1287,3 @@ + exists = GPOINTER_TO_INT (g_hash_table_lookup (directory_files_table, new_name->str)); + + if (exists == HAVE_CONFLICT && exists is equal to have_conflict? @@ +1347,3 @@ + + if (dialog->same_parent) + update_listbox (dialog); if we only do something if it's the same parent, why we call this function in the first place? I guess the check should be in the caller. @@ +1450,3 @@ + } + + for (l1 = new_names, l2 = dialog->selection; l1 != NULL && l2 != NULL; l1 = l1->next, l2 = l2->next) { all of this is the same code as check_conflict_for_file no? Cannot we shared this code somehow? @@ +1516,3 @@ +file_names_list_has_duplicates_async (NautilusBatchRenameDialog *dialog, + GAsyncReadyCallback callback, + gpointer user_data) aligmnet @@ +1526,3 @@ + dialog->conflicts_task = g_task_new (dialog, dialog->conflict_cancellable, callback, user_data); + + g_task_set_priority (dialog->conflicts_task, G_PRIORITY_DEFAULT); this is not needed, it's default by default @@ +1545,3 @@ + tag_data = g_hash_table_lookup (dialog->tag_info_table, tag_name); + + if (g_strrstr (entry_text->str, tag_name) && tag_data->set == FALSE) { hm don't we have the hashtable data precisely for this? We shouldn't rely on the text of the tag to know if it's present or not... if someone writes by hand [1,2,3] this would fail @@ +1561,3 @@ + if (g_strrstr (entry_text->str, tag_name)) { + tag_data->position = g_strrstr (entry_text->str, tag_name) - + entry_text->str; as said before, don't use pointer aritmetics @@ +1585,3 @@ + if ((g_strrstr (entry_text->str, NUMBERING) || + g_strrstr (entry_text->str, NUMBERING0) || + g_strrstr (entry_text->str, NUMBERING00)) && don't use strstr to check if the tag is present, that's why we have the hastable info of the tags. That info should be updated when the users select a tag or so in the menu, and in the key-pressed signal for deleting them etc. we can discuss it over IRC because because I'm not sure I understand why it's done in this way. @@ +1609,3 @@ + if (g_strrstr (entry_text->str, NUMBERING) == NULL && + g_strrstr (entry_text->str, NUMBERING0) == NULL && + g_strrstr (entry_text->str, NUMBERING00) == NULL && dito @@ +1629,3 @@ + + if (g_strrstr (entry_text->str, NUMBERING)) { + tag_data->position = g_strrstr (entry_text->str, NUMBERING) - entry_text->str; not pointer aritmetics @@ +1652,3 @@ + tag_data = g_hash_table_lookup (dialog->tag_info_table, CREATION_DATE); + if (tag_data->available) + check_if_tag_is_used (dialog, the name of this function is kinda misleading. We check if the tag is used, then what? Doesn't sound like we do any action in the function. But as said previously I'm not sure I understood completely the function. @@ +1692,3 @@ + +static gboolean +have_unallowed_character (NautilusBatchRenameDialog *dialog) we have more cases that are not valid. Like renaming all files to . or .., take a look at the new_folder_dialog for what we check about. I tried for example to rename a file to "." and it failed with a critical on the terminal output. @@ +1725,3 @@ + return TRUE; + } else { + gtk_widget_hide (dialog->conflict_box); instead of the conflict box we should use a label with a expander under the entry as we discussed and how is done for other dialogs now. Also I realized the dialog keeps growing if the conflict_box is shown with this patch. @@ +1762,3 @@ + + if (!tag_data->set && !tag_data0->set && !tag_data00->set) { + gtk_label_set_label (GTK_LABEL (dialog->numbering_label), ""); why not hide? @@ +1780,3 @@ +update_display_text (NautilusBatchRenameDialog *dialog) +{ + file_names_widget_entry_on_changed (dialog); do the oposite. A helper function with the common code, and _on_ for signal callbacks @@ +1802,3 @@ + + update_display_text (dialog); + spurious line @@ +1836,3 @@ + +void +nautilus_batch_rename_dialog_query_finished (NautilusBatchRenameDialog *dialog, It's not very clear what query this refers to. We can use a more specific name here. @@ +1857,3 @@ + dialog->create_date = hash_table; + + if (dialog->create_date != NULL) { this can be incorporated with the previous if no? @@ +1874,3 @@ + + tag_data = g_hash_table_lookup (dialog->tag_info_table, CREATION_DATE); + if (metadata->creation_date == NULL || g_strcmp0 (metadata->creation_date->str, "") == 0) { is it possible to have an empty metadata value? I believe it should be null right? @@ +2016,3 @@ + } + + return FALSE; GDK_EVENT_PROPAGATE, the docs are wrong (although GDK_EVENT_PROPAGATE is the same as FALSE) @@ +2050,3 @@ +remove_tag (NautilusBatchRenameDialog *dialog, + gchar *tag_name, + gchar *keyval_name, Use the enum rather than the string. The string could change. @@ +2065,3 @@ + + if (gtk_editable_get_selection_bounds (GTK_EDITABLE (dialog->name_entry), NULL, NULL) && + cursor_position == tag_data->position + strlen (tag_name) && remember to change all strlen to g_utf8_strlen @@ +2066,3 @@ + if (gtk_editable_get_selection_bounds (GTK_EDITABLE (dialog->name_entry), NULL, NULL) && + cursor_position == tag_data->position + strlen (tag_name) && + g_strcmp0(keyval_name, "BackSpace") == 0) What is this if trying to achieve? If it's to avoid when deleting on the border of a tag when something is selected, use intermediate variables to explain. For example: area_selected = gtk_editable_get_selection_bounds (GTK_EDITABLE (dialog->name_entry), NULL, NULL) end_of_tag = tag_data->position + strlen (tag_name) etc. Is't pretty criptic to understand the code if not. @@ +2071,3 @@ + if (gtk_editable_get_selection_bounds (GTK_EDITABLE (dialog->name_entry), NULL, NULL) && + cursor_position == tag_data->position && + g_strcmp0(keyval_name, "Delete") == 0) use the keyval enum @@ +2073,3 @@ + g_strcmp0(keyval_name, "Delete") == 0) + return FALSE; + from now on you check for tag_data->set all the time, couldn't we early return if that's not the case? @@ +2082,3 @@ + tag_data->position); + new_entry_text = g_string_append (new_entry_text, + entry_text->str + tag_data->position + strlen (tag_name)); remember to not use pointer aritmatics, rather get a substring with g_utf8_substring. This happens more times along the patch. @@ +2121,3 @@ + g_strcmp0(keyval_name, "Left") != 0 && + g_strcmp0(keyval_name, "Right") != 0 && + g_strcmp0(keyval_name, "Return") != 0) { and Tab @@ +2143,3 @@ + } + + return FALSE; this function still doesn't manage when the selection has more than one tag selected entirely or partially. This should be managed as well. @@ +2322,3 @@ + + for (l = dialog->selection; l != NULL; l = l->next) + files_number++; g_list_lenght? @@ +2330,3 @@ + nautilus_batch_rename_dialog_initialize_actions (dialog); + + dialog->same_parent = !NAUTILUS_IS_SEARCH_DIRECTORY (directory); nautilus_directory_is_search. Use normal API when possible instead of macros. @@ +2393,3 @@ + (GDestroyNotify) g_free, + (GDestroyNotify) g_free); + tag_data = g_new (TagData, 1); I though we were going to do this in a loop... so we don't repeat the code. Basically you can have an enum and go through the items of the enum. Of course there are special cases, so you can do something like tag_data->available = value == ORIGINAL_FILE || value == whatever.
Review of attachment 333941 [details] [review]: Ok another file :) ::: src/nautilus-batch-rename-dialog.h @@ +77,3 @@ + GList *files); + +gint compare_int (gconstpointer a, this shouldn't be public ::: src/nautilus-batch-rename-utilities.c @@ +114,3 @@ + +GString* +batch_rename_dialog_replace_label_text (gchar *string, string but it's not a string type, it's confusing. Use a more specific parameter name. label? @@ +125,3 @@ + + if (substring == NULL || g_strcmp0 (substring, "") == 0) { + token = g_markup_escape_text (string, strlen (string)); in this case the second argument is in bytes, so you use correctly the strlen here. Be careful with this differentiation, I might even review something wrong too (will try hard not to :)) @@ +142,3 @@ + + n_splits = g_strv_length (splitted_string); + add a comment here to say what we are doing or why @@ +172,3 @@ + FileMetadata *file_metadata; + + for (l = selection_metadata; l != NULL; l = l->next) { missing album name for music. @@ +216,3 @@ + +static GString* +batch_rename_dialog_format (NautilusFile *file, batch_rename_format. There is no dialog anywhere here. @@ +217,3 @@ +static GString* +batch_rename_dialog_format (NautilusFile *file, + GList *tags_list, tags are more than just "tags", but chunks of text introduced by the user + tags no? I would differenciate this in the names of the variables and parameters. This is something that happens along the patch. I would call this list text_chunks or so. @@ +258,3 @@ + + if (!added_tag && g_strcmp0 (tag->str, NUMBERING0) == 0) { + if (count < 10) { printf has a way to pad zeros on demand. %20d will pad 2 zeroes if necesary. This serie of ifs can be just a case statement with a single line of code each with the g_printf. @@ +294,3 @@ + + if (metadata != NULL) { + splitted_date = g_strsplit_set (metadata, "T:-Z", -1); oh no no, use the string as a g_date_set_parse and g_date_valid to get a date from a string. Rule of thumb is never "play" with the date strings at all or dates in general. @@ +371,3 @@ + + if (g_strcmp0 (new_name->str, "") == 0) { + new_name = g_string_append (new_name, file_name); so the extension is not appended in this case? @@ +372,3 @@ + if (g_strcmp0 (new_name->str, "") == 0) { + new_name = g_string_append (new_name, file_name); + } else { else if () in the same line @@ +411,3 @@ + new_name = batch_rename_dialog_format (file, + tags_list, + selection_metadata, identation @@ +414,3 @@ + count++); + result = g_list_prepend (result, new_name); + } use else if when appropiate or else. Doing series of ifs is confusing as in it looks like both things can happen. @@ +419,3 @@ + new_name = batch_rename_dialog_replace (file_name->str, + entry_text, + replace_text); identation @@ +469,3 @@ + } + + /* such a file doesn't exist so there actually is a conflict */ this sentence sounds wrong @@ +579,3 @@ + if (mode == FIRST_MODIFIED) { + return g_list_sort (selection, compare_files_by_first_modified); + } use elses ifs @@ +631,3 @@ + +static void +cursor_callback (GObject *object, us the _on_ pattern @@ +660,3 @@ + + success = tracker_sparql_cursor_next_finish (cursor, result, &error); + if (!success) { woudl be good to display some error with error->message in the output. You can use g_warning(). As a rule of thumb never fail without a message, at least we can break the code path and know where the error is. @@ +669,3 @@ + } + + creation_date = tracker_sparql_cursor_get_string (cursor, 1, NULL); use an enum instead of numbers. I think I mentioned this in the previous review already. @@ +712,3 @@ + } + } else { + g_string_append (metadata->creation_date, append? just free the previous one and create a new one no? @@ +802,3 @@ + + /* title */ + if (data->have_title) { do we need this? I have the feeling that we can just check for NULL everywhere no? @@ +842,3 @@ + + if (error != NULL) { + g_error_free (error); display an error @@ +901,3 @@ + metadata->file_name = g_string_new (file_name); + metadata->creation_date = g_string_new (""); + metadata->equipment = g_string_new (""); I think these can be initialized to NULL. Any reason to create a string that it migth be not used? @@ +952,3 @@ + +GList* +distinct_file_parents (GList *selection) if this is used externally it's missing the class prefix. Also I would name the function something like batch_rename_files_get_distinct_parents or so. ::: src/nautilus-batch-rename-utilities.h @@ +1,2 @@ +#ifndef NAUTILUS_BATCH_RENAME_UTILITIES_H +#define NAUTILUS_BATCH_RENAME_UTILITIES_H you are missing the GPL header etc. @@ +24,3 @@ + GHashTable *creation_date_table); + +gint compare_files_by_last_modified (gconstpointer a, why are all these functions public?
Review of attachment 333941 [details] [review]: and not much left to review finally! But still work to do: ::: src/nautilus-file.c @@ +2030,3 @@ + +static void + error = g_error_new (G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED, so I was looking again at this, and I'm pretty sure we can move the batch operation to file-operations. This process shouldn't be here, and we shouldn't have an operation of multiples files in a single file. I can see that all what is needed is public API, and if not we can make it public. We can make an operation that calls nautilus_file_rename with all the files and that's it. Any reason that we cannot do it?
Review of attachment 333941 [details] [review]: I rethought about moving the rename to file-operations, and it's complex because of the subclasses. So let's left it there for now. ::: src/nautilus-batch-rename-dialog.c @@ +116,3 @@ + gboolean set; + gint position; +} TagData; awesome :)h @@ +400,3 @@ + { "add-artist-name-tag", add_metadata_tag }, + { "add-title-tag", add_metadata_tag }, + spurious line @@ +412,3 @@ + gint index; + + if (!GTK_IS_LIST_BOX_ROW (listbox_row)) can this happen? it should be an error if this happen, with g_return_if_fail(), although we usually don't do this for private functions. @@ +418,3 @@ + index = gtk_list_box_row_get_index (listbox_row); + + if (GTK_WIDGET (box) == dialog->original_name_listbox) { I'm completely fine with this, but just for your information, there is no need to cast for comparison. It actually compares just the pointers. @@ +446,3 @@ +} + +gint compare_int (gconstpointer a, in another line @@ +449,3 @@ + gconstpointer b) +{ + int *number1 = (int*) a; it's more a position right? You can make this function more specific, like compare_position.
Created attachment 334271 [details] [review] Implement batch renaming
Created attachment 334274 [details] [review] Implement batch renaming
Created attachment 334280 [details] [review] batch-rename-dialog: expand the entry
Created attachment 334281 [details] [review] batch-rename-dialog: set max-width and max-height
Review of attachment 334274 [details] [review]: Much better than before, still work to do on polishing the bits we mentioned in preview patches, but the UX experience is amazing, congrats!!! With the current state of the code I'm happy to merge. So marking as accept-commit-now. Happy to see this landing for 3.22! :D
Review of attachment 334280 [details] [review]: my patch
Review of attachment 334281 [details] [review]: my patch
Created attachment 334289 [details] [review] Implement batch renaming
Review of attachment 334289 [details] [review]: yep! now without trailing whitespaces
Created attachment 334321 [details] [review] Implement batch renaming
Review of attachment 334321 [details] [review]: ::: src/nautilus-batch-rename-dialog.c @@ +2280,3 @@ + GdkEventKey *event, + gpointer user_data) +{ I think this could be done much simpler checking for bounds of the cursor (and a bound with no selection would have the same start and end) and make the code generic enough to work in all cases rather than having a list of if elses, which is bad practice. In any case, it's going to work anyway, so let's merge as soon as possible and then we can do big cleanups :) @@ +2316,3 @@ + keyval != GDK_KEY_Tab && + keyval != GDK_KEY_End && + keyval != GDK_KEY_Home))) { as said all of this could be in a loop, and the checking of the keyval in a function or so. @@ +2601,3 @@ + if ((keyval == GDK_KEY_Delete || keyval == GDK_KEY_BackSpace) && + tag_removed) + return TRUE; what it's true? I guess you want to return GDK_EVENT_* no? @@ +2720,3 @@ + return TRUE; + + return GDK_EVENT_PROPAGATE; you return this two lines later... Not sure what the point of this here.
Created attachment 334334 [details] [review] Implement batch renaming
Created attachment 334335 [details] [review] batch-rename-dialog: expand the entry
Created attachment 334336 [details] [review] batch-rename-dialog: set max-width and max-height
Created attachment 334337 [details] [review] batch-rename-dialog: use GIO constants
Created attachment 334338 [details] [review] batch-rename-dialog: simplify key_press_event if's
Created attachment 334340 [details] [review] Implement batch renaming Users can now batch rename files in Nautilus. This operation is launched in the same way as the rename one, when the selectiobn has more than one file. When the batch renaming is launched, a dialog is shown, offering two modes. In the first mode, the user can use metadata (if available), numbering and original file name tags to create the new names. Between the tags, there also can be written normal text, which will be added in the new names. If numbering is used, the order of the files can be modified by using several criteria. In the second mode, the user can replace an existing part of the name.
Created attachment 334341 [details] [review] Implement batch renaming Users can now batch rename files in Nautilus. This operation is launched in the same way as the rename one, when the selection has more than one file. When the batch renaming is launched, a dialog is shown, offering two modes. In the first mode, the user can use metadata (if available), numbering and original file name tags to create the new names. Between the tags, there also can be written normal text, which will be added in the new names. If numbering is used, the order of the files can be modified by using several criteria. In the second mode, the user can replace an existing part of the name.
Created attachment 334342 [details] [review] batch-rename-dialog: expand the entry
Created attachment 334343 [details] [review] batch-rename-dialog: set max-width and max-height
Created attachment 334344 [details] [review] batch-rename-dialog: use GIO constants
Created attachment 334345 [details] [review] batch-rename-dialog: simplify key_press_event if's
Created attachment 334348 [details] [review] Implement batch renaming Renaming multiple files at once has been a missing feature in Nautilus for a long time. This patch implements that feature in the following way: This operation is launched in the same way as the rename one, when the selection has more than one file. When the batch renaming is launched, a dialog is shown, offering two modes. In the first mode, the user can use metadata (if available), numbering and original file name tags to create the new names. Between the tags, there also can be written normal text, which will be added in the new names. If numbering is used, the order of the files can be modified by using several criteria. In the second mode, the user can replace an existing part of the name.
Created attachment 334349 [details] [review] batch-rename-dialog: expand the entry
Created attachment 334350 [details] [review] batch-rename-dialog: set max-width and max-height
Created attachment 334351 [details] [review] batch-rename-dialog: use GIO constants
Created attachment 334352 [details] [review] batch-rename-dialog: simplify key_press_event if's
Attachment 334348 [details] pushed as be12a75 - Implement batch renaming Attachment 334349 [details] pushed as ab504d9 - batch-rename-dialog: expand the entry Attachment 334350 [details] pushed as 3b60eb3 - batch-rename-dialog: set max-width and max-height Attachment 334351 [details] pushed as c5e1635 - batch-rename-dialog: use GIO constants Attachment 334352 [details] pushed as d147f25 - batch-rename-dialog: simplify key_press_event if's
*** Bug 687056 has been marked as a duplicate of this bug. ***