GNOME Bugzilla – Bug 762126
file-operations: check that trashed files are actually in the trash
Last modified: 2021-06-18 15:32:58 UTC
In Nautilus, restoring files from the trash is done based on the trashed files kept in the undo information. In order to find the matching files that need to be restored, the contents of the trash are ran against the files in the undo info. Enumeration of the trash and matching are currently handled by the same function, which does not allow for any of these operations to be used separately. Add a function for synchronously enumerating the contents of the trash. Add a function for matching files in the undo info with the list of files in the trash.
Created attachment 321348 [details] [review] file-undo-operations: modularize restoring files from trash In Nautilus, restoring files from the trash is done based on the trashed files kept in the undo information. In order to find the matching files that need to be restored, the contents of the trash are ran against the files in the undo info. Enumeration of the trash and matching are currently handled by the same function, which does not allow for any of these operations to be used separately. Add a function for synchronously enumerating the contents of the trash. Add a function for matching files in the undo info with the list of files in the trash.
Created attachment 321349 [details] [review] file-operations: check that trashed files are actually in the trash In Nautilus, trash operations are performed and finalized before the trash is updated. Because of this, the option to undo trashing becomes available even if the trash is not aware of new files. If an undo is then requested, the operation would do nothing, because the files would appear to not exist in the trash. A better solution would be to wait for the trash to notified, and then check that the trashed files are actually there. Only then should a trash operation be considered finalized. Add a signal to the trash-monitor that is emitted when the trash changes. Connect trash operations to this signal. On signal emission, asynchronously check that the files are in the trash. Finalize the job if the check was not cancelled and successful.
Review of attachment 321348 [details] [review]: ::: libnautilus-private/nautilus-file-undo-operations.c @@ +1095,3 @@ +static GList* +trash_enumerate_children (GError **error) trash_enumerate_children_info Also, could you add a comment to the top of the function to explicitly say what it have inside the return value and what the ownership is? Took me a few minutes to realize. Something like: /** * Returns (transfer full): A #GList with full transferred #GFileInfo items as content. Free the returned object with g_list_free_full (list, g_object_unref). */ @@ +1100,2 @@ GFile *trash; + GList *children = NULL; Why a list and not a hash table as other functions are doing? See my next comment for further understanding. @@ +1145,3 @@ + trash = g_file_new_for_uri ("trash:///"); + + for (l = trash_children; l != NULL; l = l->next) { So we iterate over all files in the trash to check whether the files we trashed are really there? This loop looks like the opposite as I was expecting of (not your fault). I think we could iterate over the trashed files, and just ask the files in the trash (assuming they are in a hash table as previously said to optimize it) if it contains them. Also, why this function returns a guint? the return value is not used at all. Same for the error parameter, which is not used.
(In reply to Carlos Soriano from comment #3) > So we iterate over all files in the trash to check whether the files we > trashed are really there? This loop looks like the opposite as I was > expecting of (not your fault). > I think we could iterate over the trashed files, and just ask the files in > the trash (assuming they are in a hash table as previously said to optimize > it) if it contains them. The way I see it, in order to implement the check as a match between two hashes, I would have to build a hash with the files in the trash, with GFiles as the key (so it matches the structure of the hash in the undo info). The problem is that, since the trash can contain multiple files with the same original path, the trash hash would have to contain multiple values with the same key, which is impossible. I think it'd be overkill to try and make this work, since it would not even add a boost to check efficiency.
(In reply to Razvan Chitu from comment #4) > (In reply to Carlos Soriano from comment #3) > > > So we iterate over all files in the trash to check whether the files we > > trashed are really there? This loop looks like the opposite as I was > > expecting of (not your fault). > > I think we could iterate over the trashed files, and just ask the files in > > the trash (assuming they are in a hash table as previously said to optimize > > it) if it contains them. > > The way I see it, in order to implement the check as a match between two > hashes, I would have to build a hash with the files in the trash, with > GFiles as the key (so it matches the structure of the hash in the undo > info). The problem is that, since the trash can contain multiple files with > the same original path, the trash hash would have to contain multiple values > with the same key, which is impossible. I think it'd be overkill to try and > make this work, since it would not even add a boost to check efficiency. Meeeh, you are right :) Could you add a comment so future readers don't fall in the same (maybe stupid) idea I felt? Somehting like: We don't use a hash table because multiple files in the trash can have the same original path.
Review of attachment 321349 [details] [review]: This code is starting to feel modern and clear :) Some small comments following, I will review better tomorrow. ::: libnautilus-private/nautilus-file-operations.c @@ +2185,3 @@ +static void +trash_enumerate_children_cb (GObject *source, + GAsyncResult *res, wrong identation ::: libnautilus-private/nautilus-trash-monitor.c @@ +82,3 @@ + signals[TRASH_UPDATED] = g_signal_new + ("trash-changed", hm trash-changed or trash updated :) I would prefer trash changed. btw, this is not only cosmetic, it's important to use the names correctly. In this case is not a problem because the change is made only in the enum, which is just an integer. But for other cases where GObject is involved, composed names like "trash-changed" generate also names like trash_changed and TRASH_CHANGED functions types etc. Just to keep it in mind, name conventions are important for GObject.
Created attachment 321835 [details] [review] file-undo-operations: modularize restoring files from trash In Nautilus, restoring files from the trash is done based on the trashed files kept in the undo information. In order to find the matching files that need to be restored, the contents of the trash are ran against the files in the undo info. Enumeration of the trash and matching are currently handled by the same function, which does not allow for any of these operations to be used separately. Add a function for synchronously enumerating the contents of the trash. Add a function for matching files in the undo info with the list of files in the trash.
Created attachment 321836 [details] [review] file-operations: check that trashed files are actually in the trash In Nautilus, trash operations are performed and finalized before the trash is updated. Because of this, the option to undo trashing becomes available even if the trash is not aware of new files. If an undo is then requested, the operation would do nothing, because the files would appear to not exist in the trash. A better solution would be to wait for the trash to notified, and then check that the trashed files are actually there. Only then should a trash operation be considered finalized. Add a signal to the trash-monitor that is emitted when the trash changes. Connect trash operations to this signal. On signal emission, asynchronously check that the files are in the trash. Finalize the job if the check was not cancelled and successful.
Review of attachment 321835 [details] [review]: this one LGTM now, thanks!
Comment on attachment 321835 [details] [review] file-undo-operations: modularize restoring files from trash Attachment 321835 [details] pushed as 39798e2 - file-undo-operations: modularize restoring files from trash
Created attachment 322185 [details] [review] file-operations: check that trashed files are actually in the trash In Nautilus, trash operations are performed and finalized before the trash is updated. Because of this, the option to undo trashing becomes available even if the trash is not aware of new files. If an undo is then requested, the operation would do nothing, because the files would appear to not exist in the trash. A better solution would be to wait for the trash to notified, and then check that the trashed files are actually there. Only then should a trash operation be considered finalized. Add a signal to the trash-monitor that is emitted when the trash changes. Connect trash operations to this signal. On signal emission, asynchronously check that the files are in the trash. Finalize the job if the check was not cancelled and successful.
Review of attachment 322185 [details] [review]: /me dancing in happiness Now this is looking good, I need to test it more, but quite happy with the result, very clean! As a overview, try to do good indentation and use always that you can spaces instead of tabs, also for copy pasted code. If you want to us tabs to preserve the file code style of using tabs, I'm completely fine with that. But I think both of us agree tabs makes it more difficult at the end. ::: libnautilus-private/nautilus-file-operations.c @@ +2186,3 @@ +/** + * Returns: TRUE if all the files were matched, FALSE otherwise + */ the function name should be clear enough to explain this withouth needing a comment. Maybe you want to change the function name? I'm fine with the current one (or another you came with that you find enough clear to not need a comment). Basically comments should be needed only when there is no other way around to explain why the code is needed or what the code does. @@ +2279,3 @@ +trash_enumerate_children_cb (GObject *source, + GAsyncResult *res, + gpointer user_data) wrong identation @@ +2288,3 @@ + trash = G_FILE (source); + enumerator = g_file_enumerate_children_finish (trash, res, &error); + /* If the enumeration was cancelled, enumerator will be NULL */ this is part of the documentation, so not sure this comment is needed @@ +2411,3 @@ if (to_trash_files != NULL) { to_trash_files = g_list_reverse (to_trash_files); + Explain a little why we need this. Something like: /* Wait until the trash acknowledge the files were trashed to finish the operation, since the trash usually takes some time to acknowledge the file movement. */ ::: libnautilus-private/nautilus-file-operations.h @@ +87,3 @@ +gboolean trash_match_files (GList *trash_children, + GHashTable *trashed, + GHashTable *matched_files); wrong identation ::: libnautilus-private/nautilus-trash-monitor.c @@ +82,3 @@ + signals[TRASH_CHANGED] = g_signal_new + ("trash-changed", wrong identation
Created attachment 322237 [details] [review] file-operations: check that trashed files are actually in the trash In Nautilus, trash operations are performed and finalized before the trash is updated. Because of this, the option to undo trashing becomes available even if the trash is not aware of new files. If an undo is then requested, the operation would do nothing, because the files would appear to not exist in the trash. A better solution would be to wait for the trash to notified, and then check that the trashed files are actually there. Only then should a trash operation be considered finalized. Add a signal to the trash-monitor that is emitted when the trash changes. Connect trash operations to this signal. On signal emission, asynchronously check that the files are in the trash. Finalize the job if the check was not cancelled and successful.
Review of attachment 322237 [details] [review]: ::: libnautilus-private/nautilus-file-operations.c @@ +2187,3 @@ +trash_files_match (GList *trash_children, + GHashTable *trashed, + GHashTable *matched_files) wrong ident @@ +2249,3 @@ +trash_enumerate_next_files_cb (GObject *source, + GAsyncResult *res, + gpointer user_data) wrong ident @@ +2265,3 @@ + g_main_context_invoke (NULL, + delete_task_done, + job); use a whole line @@ +2276,3 @@ +trash_enumerate_children_cb (GObject *source, + GAsyncResult *res, + gpointer user_data) wrong ident @@ +2315,3 @@ +static void +trash_changed_cb (NautilusTrashMonitor *trash_monitor, + gpointer user_data) wrogn ident
Created attachment 322316 [details] [review] file-operations: check that trashed files are actually in the trash In Nautilus, trash operations are performed and finalized before the trash is updated. Because of this, the option to undo trashing becomes available even if the trash is not aware of new files. If an undo is then requested, the operation would do nothing, because the files would appear to not exist in the trash. A better solution would be to wait for the trash to notified, and then check that the trashed files are actually there. Only then should a trash operation be considered finalized. Add a signal to the trash-monitor that is emitted when the trash changes. Connect trash operations to this signal. On signal emission, asynchronously check that the files are in the trash. Finalize the job if the check was not cancelled and successful.
The UI lock ups we had was due to https://bugzilla.gnome.org/show_bug.cgi?id=762677, so not your patch or so. That is fixed now. Trying your patch, I could make it crash once, I think because of a thread race or so of the trashed hash. Not sure where, since I couldn't catch a backtrace due to the previous bug. Another thing I noticed is that redo is not available at all with, do oyu have any insights about it?
Review of attachment 322316 [details] [review]: After discussing on IRC with Razvan, this needs further digging into gvfs trash, so I'm to reject this patch for now...
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version of Files (nautilus), then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/nautilus/-/issues/ Thank you for your understanding and your help.