GNOME Bugzilla – Bug 548353
Finish implementing GFile interface (mostly asynchronous functions)
Last modified: 2018-05-28 11:46:39 UTC
As it stands right now, there are a few functions missing in the GFile interface that just haven't been implemented yet. From the short conversation I had with Havoc Pennington on IRC, I can see that at least the following functions are missing, and should be filled in: * g_file_make_directory_(async,finish) * g_file_make_symbolic_link_(async,finish) * g_file_trash_(async,finish) * g_file_delete_(async,finish) * g_file_query_settable_attributes_(async,finish) * g_file_query_writable_namespaces_(async,finish)
Also missing is g_file_move_(async,finish).
*** Bug 638752 has been marked as a duplicate of this bug. ***
Created attachment 177661 [details] [review] First attempt to implement g_file_delete_async() and g_file_delete_finish() Here is my attempt to implement the asynchronous version of g_file_delete(). I have tested it only on Ubuntu linux on local files. This is my first attempt so please indicate if there are any problems. One question: is it necessary to check for if iface->delete_file == NULL before calling it? Some functions check the interface pointer before calling it, but other's assume it has been implemented. For example g_file_replace() checks, but g_file_replace_async() does not. Is this a bug? If this patch is accepted it should be trivial for me to add the asynchronous version of g_file_trash() as well.
(In reply to comment #3) > One question: is it necessary to check for if iface->delete_file == NULL before > calling it? Some functions check the interface pointer before calling it, but > other's assume it has been implemented. For example g_file_replace() checks, > but g_file_replace_async() does not. Is this a bug? No; it's because g_file_default_init() fills in the replace_async pointer itself, so it knows it's going to be non-NULL regardless of what the subclass does. Likewise in your patch, you're filling in a default delete_async() implementation, so you know it will always be non-NULL
(In reply to comment #4) > No; it's because g_file_default_init() fills in the replace_async pointer > itself, so it knows it's going to be non-NULL regardless of what the subclass > does. Likewise in your patch, you're filling in a default delete_async() > implementation, so you know it will always be non-NULL That's not exactly what I mean. Yes, you are right there is no point in checking if iface->replace_async is NULL because it is always filled in by g_file_default_init(). In g_file_replace() there is a check: if (iface->replace == NULL) ... However g_file_replace_async() calls iface->replace_async which is set to g_file_real_replace_async(). Then g_file_real_replace_async() calls replace_async_thread() in a new thread, and replace_async_thread() calls iface->replace() without ever checking if it is NULL.
er... yes, that's a bug
Created attachment 234925 [details] [review] Add missing details in GFile documentation
Created attachment 234926 [details] [review] Add async version of g_file_trash()
Comment on attachment 177661 [details] [review] First attempt to implement g_file_delete_async() and g_file_delete_finish() The async version of g_file_delete() has been implemented.
(In reply to comment #5) > In g_file_replace() there is a check: if (iface->replace == NULL) ... > > However g_file_replace_async() calls iface->replace_async which is set to > g_file_real_replace_async(). Then g_file_real_replace_async() calls > replace_async_thread() in a new thread, and replace_async_thread() calls > iface->replace() without ever checking if it is NULL. (In reply to comment #6) > er... yes, that's a bug (Ah, I should have read this before submitting the above patches…) To fix this bug, we can simply call g_file_replace() in replace_async_thread(). Or the other solution is to copy/pase the code of g_file_replace(). The former is a bit less efficient. What do you prefer? I can do that for all *_async_thread().
ping
(In reply to comment #10) > To fix this bug, we can simply call g_file_replace() in replace_async_thread(). > Or the other solution is to copy/pase the code of g_file_replace(). The former > is a bit less efficient. > > What do you prefer? > > I can do that for all *_async_thread(). I'd say just call g_file_replace() in replace_async_thread().
Comment on attachment 234926 [details] [review] Add async version of g_file_trash() >+ * Virtual: trash_async >+ * Virtual: trash_finish are these annotations actually needed? >+ if (g_async_result_legacy_propagate_error (result, error)) >+ return FALSE; That's a compatibility mechanism for old APIs that assumed all implementations would be using GSimpleAsyncResult. You don't need it in newly-added APIs. >+ if (iface->trash (file, cancellable, &error)) (same issue as with replace, etc here) >+GLIB_AVAILABLE_IN_2_36 will need to be updated now
(In reply to comment #13) > (From update of attachment 234926 [details] [review]) > >+ * Virtual: trash_async > > >+ * Virtual: trash_finish > > are these annotations actually needed? It can be useful for knowing the corresponding field in the struct GFileIface. The annotations are written for the other functions too, so it was for consistency. I've fixed the commit and pushed it, thanks for the review.
Created attachment 241740 [details] [review] GFile: fix the *_async_thread() In the *_async_thread() functions, call the corresponding synchronous function instead of calling the interface vfunc, which can be NULL. In some cases the check for the vfunc == NULL was done, but to be consistent it is better to always call the synchronous version (and the code is simpler).
Created attachment 241753 [details] [review] Add async version of g_file_make_directory()
Comment on attachment 241740 [details] [review] GFile: fix the *_async_thread() looks good (assuming "make check" still passes)
Comment on attachment 241753 [details] [review] Add async version of g_file_make_directory() >+ void (* make_directory_async) (GFile *file, >+ int io_priority, >+ GCancellable *cancellable, >+ GAsyncReadyCallback callback, >+ gpointer user_data); >+ gboolean (* make_directory_finish) (GFile *file, >+ GAsyncResult *result, >+ GError **error); Use spaces, not tabs, to do the alignment in here and in the prototype below otherwise looks good
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/157.
Comment on attachment 234926 [details] [review] Add async version of g_file_trash() This (or a tweaked version of it) was committed as commit 733bf962023d9b22.