GNOME Bugzilla – Bug 757385
Rename in search results broken for "duplicate" files
Last modified: 2016-05-24 16:34:41 UTC
Created attachment 314520 [details] Example of invalid duplicate reported A rename in search results uses search results to check for duplicate names, instead of each file's actual directory. I.e. if a recursive search finds multiple files with the same file name, it is not possible to rename all of these files to the same new name. Scenario: As an Android developer, I have image files located in resolution specific directories. Those images share the same name. Example: /ldpi/ic_checkmark.png /mdpi/ic_checkmark.png /hdpi/ic_checkmark.png A search for "ic_checkmark" returns all of these files. Because the file name is incorrect, I want to rename all of these files. After I rename one of these files to "ic_checkmark_default.png" I cannot rename the next file to the same name. Gnome reports "A file with that name already exists" even though the name is unique in the file's respective directory. Expected result: I can rename all of the files, as long as there is no duplicate in each file's actual directory. Actual result: I cannot rename a file, if search results contain a file with the same name from a different directory.
Updated, as this bug is still present in Gnome 3.20.
For newcomers with some experience, willing to write more than 100 loc. But shouldn't be hard at all. Good to understand how nautilus files works. I believe only thing we have to do is make sure we check the file name is duplicated in the original location. The problem is that we are in a "invented" directory used for search, therefore we don't have the real parents directory cached, in order to check whether a file exists or not. So what we can do is check the actual directory (not the search one) when we do the request to look if there is a duplicated file. It's still to be decided where to do that, so it's a good way to investigate.
Created attachment 327721 [details] [review] files-view: check parent location when renaming When renaming a file from the popover, the code checks if there is an existing file with the name entered and the name entered does not match the name of the file being renamed. That has a side effect of allowing the original filename to be entered without reporting an error. Some problems arise when the model includes files from different directories (e.g. when searching). The code will not allow a rename if there are files with that name, despite being in separate directories. This commit adds a parent location check to the renaming logic.
Review of attachment 327721 [details] [review]: This is indeed an improvement. It still missing to check if a file exists in the real directory with the same name. However, this was the case before anyway. Also, could you add a short comment in the code explaining why we need to check the parent? along the lines of: /* Allow duplicated names if shown files are from different directories. An example of this is the search directory */ Great work!
Created attachment 327724 [details] [review] files-view: check parent location when renaming When renaming a file from the popover, the code checks if there is an existing file with the name entered and the name entered does not match the name of the file being renamed. That has a side effect of allowing the original filename to be entered without reporting an error. Some problems arise when the model includes files from different directories (e.g. when searching). The code will not allow a rename if there are files with that name, despite being in separate directories. This commit adds a parent location check to the renaming logic.
Comment on attachment 327724 [details] [review] files-view: check parent location when renaming (In reply to Carlos Soriano from comment #4) > Review of attachment 327721 [details] [review] [review]: > > This is indeed an improvement. > It still missing to check if a file exists in the real directory with the > same name. However, this was the case before anyway. Actually, you’re right. That would fix another issue. Is there any reason at all to check the model and not the parent directory? I can’t think of a reason why a collision there would cause issues.
Created attachment 327749 [details] Related issue Okay, so it does make sense to do so when creating folders. But the location nautilus_file_view_get_backing_uri() should rather be checked and not the model directly, because things break like in the screenshot here.
Created attachment 327751 [details] [review] files-view: fix renaming logic When renaming a file from the popover, the code checks if there is an existing file with the name entered and the name entered does not match the name of the file being renamed. That has a side effect of allowing the original filename to be entered without reporting an error. Some problems arise when the model includes files from different directories (e.g. when searching or expanding directories in the list view). When searching, the code will not allow a rename if there are files with the entered name, despite being in separate directories. When renaming a file in an expanded directory, the code will ignore files inside it and only check the currently opened one. This also applies when creating new folders. This commit fixes that by checking for duplicate names inside the parent directory when renaming and checking the directory at nautilus_file_view_get_backing_uri() when creating new folders.
The patch also has a side effect of fixing bug 757163.
(In reply to Ernestas Kulik from comment #6) > Comment on attachment 327724 [details] [review] [review] > files-view: check parent location when renaming > > (In reply to Carlos Soriano from comment #4) > > Review of attachment 327721 [details] [review] [review] [review]: > > > > This is indeed an improvement. > > It still missing to check if a file exists in the real directory with the > > same name. However, this was the case before anyway. > > Actually, you’re right. That would fix another issue. > > Is there any reason at all to check the model and not the parent directory? > I can’t think of a reason why a collision there would cause issues. The parent directory? You want to check the current directory, not the parent. I'm not sure what do you mean here.
(In reply to Ernestas Kulik from comment #7) > Created attachment 327749 [details] > Related issue > > Okay, so it does make sense to do so when creating folders. But the location > nautilus_file_view_get_backing_uri() should rather be checked and not the > model directly, because things break like in the screenshot here. That makes sense to me
Review of attachment 327751 [details] [review]: This is even better! However: The patch is more complex than that, and actually is missing an important concept of nautilus files and nautilus directory. Specifically, the patch does something like: new_directory_that_didn't_go_to_disk_to_sync = nautilus_directory_get_for_file (parent_file_that_is_not_in_cache); give_me_the_children_and_check_if_one_of_them_has_the_same_name (new_directory_that...blabhlabh); So I guess you can see what happen. Creating a file in Nautilus doesn't give you any information until you really synchronise with the disk, in case of nautilus_directories either adding a monitor with nautilus_directory_file_monitor_add or nautilus_directory_call_when_ready. Specifically, what gets the information about the children is start_monitoring_file_list in directory-async.c, which then will get called on an idle on start_or_stop_io which is the main function go get the info. The directory will enumerate_children, and at the end directory_load_done and eventually directory_add_file. So the patch needs to wait for this information to actually say if there is an error or not. You can wait for the information to be ready with the call_when_ready and the wait_for_file_list parameter. Then you have to manage the situation when the view gets destroyed in while waiting for this information or anything else that should cancel the operation. You can check how the new_folder_data_new etc are done to manage when the view gets destroyed. Did you check the situation in search? Is where the parent of the files are not in the cache, and will lead to this misbehaviour. Also, some small situation that needs take care of: ::: src/nautilus-files-view.c @@ +1771,3 @@ + if (data->target_file != NULL) { + parent_location = nautilus_file_get_parent (data->target_file); what happens with '/'? the also called self-owned files in Nautilus.
feel free to reach me on IRC if have some doubt!
(In reply to Carlos Soriano from comment #12) > Review of attachment 327751 [details] [review] [review]: > > This is even better! However: > > The patch is more complex than that, and actually is missing an important > concept of nautilus files and nautilus directory. > Specifically, the patch does something like: > new_directory_that_didn't_go_to_disk_to_sync = > nautilus_directory_get_for_file (parent_file_that_is_not_in_cache); > give_me_the_children_and_check_if_one_of_them_has_the_same_name > (new_directory_that...blabhlabh); > > So I guess you can see what happen. Creating a file in Nautilus doesn't give > you any information until you really synchronise with the disk, in case of > nautilus_directories either adding a monitor with > nautilus_directory_file_monitor_add or nautilus_directory_call_when_ready. > Specifically, what gets the information about the children is > start_monitoring_file_list in directory-async.c, which then will get called > on an idle on start_or_stop_io which is the main function go get the info. > The directory will enumerate_children, and at the end directory_load_done > and eventually directory_add_file. > > So the patch needs to wait for this information to actually say if there is > an error or not. You can wait for the information to be ready with the > call_when_ready and the wait_for_file_list parameter. Then you have to > manage the situation when the view gets destroyed in while waiting for this > information or anything else that should cancel the operation. You can check > how the new_folder_data_new etc are done to manage when the view gets > destroyed. > > Did you check the situation in search? Is where the parent of the files are > not in the cache, and will lead to this misbehaviour. I did, but it all just happened to work for me. *shrug* So what if GFile facilities were used instead? They don’t block, but would that entail the same issues? > Also, some small situation that needs take care of: > > ::: src/nautilus-files-view.c > @@ +1771,3 @@ > > + if (data->target_file != NULL) { > + parent_location = nautilus_file_get_parent > (data->target_file); > > what happens with '/'? the also called self-owned files in Nautilus. I don’t think that’s relevant as we’re not going to be renaming the root directory. Can anything else be self-owned other than root?
(In reply to Ernestas Kulik from comment #14) > (In reply to Carlos Soriano from comment #12) > > Review of attachment 327751 [details] [review] [review] [review]: > > > > This is even better! However: > > > > The patch is more complex than that, and actually is missing an important > > concept of nautilus files and nautilus directory. > > Specifically, the patch does something like: > > new_directory_that_didn't_go_to_disk_to_sync = > > nautilus_directory_get_for_file (parent_file_that_is_not_in_cache); > > give_me_the_children_and_check_if_one_of_them_has_the_same_name > > (new_directory_that...blabhlabh); > > > > So I guess you can see what happen. Creating a file in Nautilus doesn't give > > you any information until you really synchronise with the disk, in case of > > nautilus_directories either adding a monitor with > > nautilus_directory_file_monitor_add or nautilus_directory_call_when_ready. > > Specifically, what gets the information about the children is > > start_monitoring_file_list in directory-async.c, which then will get called > > on an idle on start_or_stop_io which is the main function go get the info. > > The directory will enumerate_children, and at the end directory_load_done > > and eventually directory_add_file. > > > > So the patch needs to wait for this information to actually say if there is > > an error or not. You can wait for the information to be ready with the > > call_when_ready and the wait_for_file_list parameter. Then you have to > > manage the situation when the view gets destroyed in while waiting for this > > information or anything else that should cancel the operation. You can check > > how the new_folder_data_new etc are done to manage when the view gets > > destroyed. > > > > Did you check the situation in search? Is where the parent of the files are > > not in the cache, and will lead to this misbehaviour. > > I did, but it all just happened to work for me. *shrug* Ugh, there must be something strange going on.. > > So what if GFile facilities were used instead? They don’t block, but would > that entail the same issues? I'm not sure I follow here. A GFile is just a string. Everything that queries GFile information creates I/O and therefore blocks. > > > Also, some small situation that needs take care of: > > > > ::: src/nautilus-files-view.c > > @@ +1771,3 @@ > > > > + if (data->target_file != NULL) { > > + parent_location = nautilus_file_get_parent > > (data->target_file); > > > > what happens with '/'? the also called self-owned files in Nautilus. > > I don’t think that’s relevant as we’re not going to be renaming the root > directory. Can anything else be self-owned other than root? Yes, gvfs backends and invented backenda like x-nautilus-desktop:// etc. IIRC you can see we handle then in normal file renaming, so you would need the same here
(In reply to Carlos Soriano from comment #15) > I'm not sure I follow here. A GFile is just a string. Everything that > queries GFile information creates I/O and therefore blocks. The documentation tells me that the relevant GFile functions (g_file_get_*(), except for g_file_get_contents()) do no blocking I/O. > Yes, gvfs backends and invented backenda like x-nautilus-desktop:// etc. > IIRC you can see we handle then in normal file renaming, so you would need > the same here But is there any case in which the user would be able to rename those?
(In reply to Ernestas Kulik from comment #16) > (In reply to Carlos Soriano from comment #15) > > I'm not sure I follow here. A GFile is just a string. Everything that > > queries GFile information creates I/O and therefore blocks. > > The documentation tells me that the relevant GFile functions > (g_file_get_*(), except for g_file_get_contents()) do no blocking I/O. > I thinks you are missing "Note that the file with that specific name might not exist" GFile it's just a string with a set of functions for easy management of this string. Validation and info querying are done by kicking I/O. It wouldn't make sense in the opposite case, no? > > Yes, gvfs backends and invented backenda like x-nautilus-desktop:// etc. > > IIRC you can see we handle then in normal file renaming, so you would need > > the same here > > But is there any case in which the user would be able to rename those? No, that's why you need to handle when the parent is null. If not this code is going to crash in the next line. Although you are right we are not going to show an UI for it, I think better to double check.
Created attachment 328376 [details] [review] files-view: fix renaming logic When renaming a file from the popover, the code checks if there is an existing file with the name entered and the name entered does not match the name of the file being renamed. That has a side effect of allowing the original filename to be entered without reporting an error. Some problems arise when the model includes files from different directories (e.g. when searching or expanding directories in the list view). When searching, the code will not allow a rename if there are files with the entered name, despite being in separate directories. When renaming a file in an expanded directory, the code will ignore files inside it and only check the currently opened one. This also applies when creating new folders. This commit fixes that by checking for duplicate names inside the parent directory when renaming and checking the directory at nautilus_file_view_get_backing_uri() when creating new folders.
In this iteration, the code defaults to checking the backing URI with self-owned files.
Created attachment 328436 [details] [review] files-view: fix renaming logic When renaming a file from the popover, the code checks if there is an existing file with the name entered and the name entered does not match the name of the file being renamed. That has a side effect of allowing the original filename to be entered without reporting an error. Some problems arise when the model includes files from different directories (e.g. when searching or expanding directories in the list view). When searching, the code will not allow a rename if there are files with the entered name, despite being in separate directories. When renaming a file in an expanded directory, the code will ignore files inside it and only check the currently opened one. This also applies when creating new folders. This commit fixes that by checking for duplicate names inside the parent directory when renaming and checking the directory at nautilus_file_view_get_backing_uri() when creating new folders.
Review of attachment 328436 [details] [review]: Looks good now, except some nitpick I didn't catch before. Feel free to attach here and commit after the change. Thanks for the work! ::: src/nautilus-files-view.c @@ +1755,2 @@ static void +file_name_widget_entry_on_changed_directory_cb (NautilusDirectory *directory, I didn't think about this name before. Looks like what changes in the directory, not the entry. The usual pattern for this callbacks names is: object_on_signalname So for instance, a nautilus file emitting a signal named blah-bluh would be: nautilus_file_on_blah_bluh. In this case we want also to put some namespace, file_name_widget_entry. so how about: file_name_widget_entry_on_directory_info_ready? Of course there are different patterns, for example using *_cb. But I have been trying to switch over *_on_* since it seems clearer to me. In any case you were mixing the two here.
Created attachment 328448 [details] [review] files-view: fix renaming logic When renaming a file from the popover, the code checks if there is an existing file with the name entered and the name entered does not match the name of the file being renamed. That has a side effect of allowing the original filename to be entered without reporting an error. Some problems arise when the model includes files from different directories (e.g. when searching or expanding directories in the list view). When searching, the code will not allow a rename if there are files with the entered name, despite being in separate directories. When renaming a file in an expanded directory, the code will ignore files inside it and only check the currently opened one. This also applies when creating new folders. This commit fixes that by checking for duplicate names inside the parent directory when renaming and checking the directory at nautilus_file_view_get_backing_uri() when creating new folders.
Attachment 328448 [details] pushed as a4d139b - files-view: fix renaming logic