GNOME Bugzilla – Bug 686526
Pickup ownCloud accounts from GOA
Last modified: 2013-02-05 17:36:45 UTC
ownCloud accounts offer WebDAV endpoints whose URI is available as the Uri property on the account's org.gnome.OnlineAccounts.Files DBus interface. Since GVfs has a WebDAV backend, it would be good if Nautilus mounted the WebDAV endpoint automatically.
Now that I think of it, GNOME Shell looks like a better place to do this since the AutoMountManager code lives there.
For those of you not following #fedora-desktop on GIMPNet, the GVfs folks suggested that we do a GVfs volume monitor for GOA instead.
I have started working on a GVfsGoaVolumeMonitor in the wip/goa branch: http://git.gnome.org/browse/gvfs/log/?h=wip/goa
Created attachment 229765 [details] [review] Add GVfsGoaVolumeMonitor
Created attachment 229767 [details] [review] gdbus: Do not emit ask-password if the password has already been set
This is not perfect, yet, but a review would be very helpful to improve this further. I found that GDaemonMounts do not emit the pre-unmount and unmounted signals. Therefore, if someone unmounts it via Nautilus, self->mount can not be cleared in GVfsGoaVolume.
Review of attachment 229765 [details] [review]: Generally looks good although this is not a full review. I do have one important suggestion, see below. ::: monitor/goa/goavolumemonitor.c @@ +323,3 @@ + g_signal_connect (self->client, "account-added", G_CALLBACK (account_added_cb), self); + g_signal_connect (self->client, "account-removed", G_CALLBACK (account_removed_cb), self); +} The way you maintain self->accounts seems somewhat flimsy and unreliable. It would probably be better to do what all the other volume monitors are doing, e.g. having an update_stuff() function that does the following - calculate the objects you want - if the remote service (e.g. goa-daemon(8) is not running or reachable), this is the empty list - diff against objects you already have - e.g. diff_sorted_lists() - this one is copied around in 10+ projects/files - would probably be good to have in GLib itself (another bug) - remove objects no longer wanted - and emit ::removed signals for removed objects - add objects now wanted - and emit ::added signals for added objects in particular this allows for corner cases where e.g. goa-daemon(8) is being restarted (or crashes) and it also allows easily adding something like a should_show_account() function for accounts that should be ignored. It's also easier to throttle things this way, e.g. if you don't want too frequent updates, just avoid calling update_stuff() more than every, say, 0.5 seconds. Just seems more resilient and easier to read. Yes, you can say this is "overkill" but it's really not - the rest of the codebase already works this way. Here's an example of this: http://git.gnome.org/browse/gvfs/tree/monitor/udisks2/gvfsudisks2volumemonitor.c?id=1.14.2#n1755
(In reply to comment #5) > Created an attachment (id=229767) [details] [review] > gdbus: Do not emit ask-password if the password has already been set I don't think this is right and we should never support setting the password preemptively. Instead, your code should only set it in response to the ::ask-password signal. That's the way the API works.
Review of attachment 229767 [details] [review]: If I'm not mistaken, this will cause re-using the password once has been set yet the authentication process failed, e.g. due to wrong password. So effectively you'd only get the first prompt and no more.
Tomas, did you want to review the volume monitor itself, or are you waiting for a new patch from Debarshi ?
(In reply to comment #13) > Tomas, did you want to review the volume monitor itself, or are you waiting for > a new patch from Debarshi ? I have asked Debarshi to post an updated patch, I'll do the review afterwards.
Created attachment 233674 [details] [review] Add GVfsGoaVolumeMonitor
Review of attachment 233674 [details] [review]: I've gone through the code only for the moment, didn't test it yet as I need to find working machine first (my attempts to make rawhide running have failed). Will try to complete it on Monday. Code is nicely structured, readable and overall looks fine to me, my comments below are just minor or stylistic things: ::: monitor/goa/goavolume.c @@ +147,3 @@ + g_clear_object (&self->mount); + self->mount = g_file_find_enclosing_mount_finish (root, res, &error); + if (error != NULL) Jan 30 15:36:22 <hadess> i'm really annoyed at people doing "foo = blah (&error); if (error != NULL)" for error checking in gnome Please test for (self->mount == NULL) instead of error, though it's makes no visible difference in this case, just nitpicking. @@ +150,3 @@ + g_simple_async_result_take_error (simple, error); + else + self->auth_failed = FALSE; I guess you need to set auth_failed = TRUE in the opposite case (above test)? @@ +181,3 @@ + g_error_free (error); + /* We carry on because the failure could have been caused + * by a volume that was already mounted. You mean like accidentally spawning two concurrent mount operations from GoaVolumeMonitor inside UI (Nautilus)? Otherwise you should get G_IO_ERROR_ALREADY_MOUNTED in such cases. @@ +275,3 @@ + goa_password_based_call_get_password (passwd_based, id, data->cancellable, get_password_cb, simple); + } +} What will happen when passwd_based == NULL? Shouldn't we call g_simple_async_something() ? @@ +419,3 @@ +g_vfs_goa_volume_mount_finish (GVolume *_self, GAsyncResult *res, GError **error) +{ + GSimpleAsyncResult *simple = G_SIMPLE_ASYNC_RESULT (res);; Stray semicolon at the end ;-)
So I've tested it against gnome-online-accounts-3.7.4 and ownCloud (thanks for providing me that account) and can confirm it's working as expected. Signals are picked just fine when turning Files switch on and off in g-c-c panel, the URI constructed works and password is provided properly. The only thing worth improvement is end user experience. A GVolumeMonitor volume is announced and displayed properly, but once you mount it, another mount will show up. That's kinda expected since we're mounting external URI, not belonging to GOA volume monitor. This is how it looks like after mounting: Volume(0): tbzatek@xxxx Type: GProxyVolume (GProxyVolumeMonitorGoa) uuid=davs://tbzatek@xxxx/remote.php/webdav/ Mount(0): tbzatek on xxxx -> davs://tbzatek@xxxx/remote.php/webdav Type: GProxyMount (GProxyVolumeMonitorGoa) default_location=davs://tbzatek@xxxx/remote.php/webdav is_shadowed=0 Mount(0): tbzatek on xxxx -> davs://tbzatek@xxxx/remote.php/webdav Type: GDaemonMount default_location=davs://tbzatek@xxxx/remote.php/webdav is_shadowed=0 Mount(1): tbzatek on xxxx -> davs://tbzatek@xxxx/remote.php/webdav Type: GProxyMount (GProxyVolumeMonitorGoa) default_location=davs://tbzatek@xxxx/remote.php/webdav is_shadowed=0 Nautilus and computer:// also show two icons in sidebar, according to the output above. Ideally we would want to reuse existing GDaemonMount mount and hide the GOA volume monitor specific one. However, mount handling looks fine from source code perspective, finding existing mounts instead of creating new instances. Perhaps it's class casting making the difference. This is something worth improvement for the future. Then, when you unmount the real mount (GDaemonMount), the GOA GMount remains around but it's unmounted and works just fine for repeated mounting, basically harmless.
(In reply to comment #16) > Review of attachment 233674 [details] [review]: Thanks for the review. > ::: monitor/goa/goavolume.c > @@ +147,3 @@ > + g_clear_object (&self->mount); > + self->mount = g_file_find_enclosing_mount_finish (root, res, &error); > + if (error != NULL) > > [...] > > Please test for (self->mount == NULL) instead of error, though it's makes no > visible difference in this case, just nitpicking. Fixed. > @@ +150,3 @@ > + g_simple_async_result_take_error (simple, error); > + else > + self->auth_failed = FALSE; > > I guess you need to set auth_failed = TRUE in the opposite case (above test)? self->auth_failed is only written to, but never read. I removed the variable. > @@ +181,3 @@ > + g_error_free (error); > + /* We carry on because the failure could have been caused > + * by a volume that was already mounted. > > You mean like accidentally spawning two concurrent mount operations from > GoaVolumeMonitor inside UI (Nautilus)? Otherwise you should get > G_IO_ERROR_ALREADY_MOUNTED in such cases. Fixed. I did not notice G_IO_ERROR_ALREADY_MOUNTED. So, I think, we can only check for G_IO_ERROR_ALREADY_MOUNTED. > @@ +275,3 @@ > + goa_password_based_call_get_password (passwd_based, id, > data->cancellable, get_password_cb, simple); > + } > +} > > What will happen when passwd_based == NULL? Shouldn't we call > g_simple_async_something() ? I have changed it to return G_IO_ERROR_NOT_SUPPORTED if passwd_based == NULL. Actually if that code path is ever executed then it should be a logical error because the volume monitor only uses accounts that have the Files interface (ie. goa_object_peek_files (object) != NULL). See update_accounts in goavolumemonitor.c. The reason is that currently only accounts that use raw passwords are supported because ones that use different authentication methods (eg., OAuth1 or OAuth2 tokens) do not have GVfs backends (eg., Google Drive, SkyDrive, Dropbox, etc.). But, I think you are right. Returning an error looks more robust. > @@ +419,3 @@ > +g_vfs_goa_volume_mount_finish (GVolume *_self, GAsyncResult *res, GError > **error) > +{ > + GSimpleAsyncResult *simple = G_SIMPLE_ASYNC_RESULT (res);; > > Stray semicolon at the end ;-) Fixed.
Created attachment 235205 [details] [review] Add GVfsGoaVolumeMonitor I also took the liberty of making update_accounts and update_volumes follow the same pattern. I don't remember why I had written them differently, but I don't see any problems in making them look similar.
(In reply to comment #18) > > @@ +275,3 @@ > > + goa_password_based_call_get_password (passwd_based, id, > > data->cancellable, get_password_cb, simple); > > + } > > +} > > > > What will happen when passwd_based == NULL? Shouldn't we call > > g_simple_async_something() ? > > I have changed it to return G_IO_ERROR_NOT_SUPPORTED if passwd_based == NULL. > > Actually if that code path is ever executed then it should be a logical error > because the volume monitor only uses accounts that have the Files interface > (ie. goa_object_peek_files (object) != NULL). See update_accounts in > goavolumemonitor.c. > > The reason is that currently only accounts that use raw passwords are supported > because ones that use different authentication methods (eg., OAuth1 or OAuth2 > tokens) do not have GVfs backends (eg., Google Drive, SkyDrive, Dropbox, etc.). > > But, I think you are right. Returning an error looks more robust. Okay, shouldn't be g_simple_async_result_complete_in_idle(simple) called after you set an error and before you return? My original concern was that you're terminating the chain of async operations without calling any callbacks, so what the user gets is a mount timeout after some time. Oh and forgot to mention that some (user visible) error messages should be probably made translatable. (In reply to comment #19) > I also took the liberty of making update_accounts and update_volumes follow the > same pattern. I don't remember why I had written them differently, but I don't > see any problems in making them look similar. Yeah I've noticed that too but it was just a matter of code style, functional-wise it was correct. Everything else looks fine, thanks for fixing.
(In reply to comment #20) > Okay, shouldn't be g_simple_async_result_complete_in_idle(simple) called after > you set an error and before you return? My original concern was that you're > terminating the chain of async operations without calling any callbacks, so > what the user gets is a mount timeout after some time. Sorry, silly mistake. Fixed. > Oh and forgot to mention that some (user visible) error messages should be > probably made translatable. Is it ok to mark the error messages that are going to be passed to GSimpleAsyncResult? I am not 100% sure.
Created attachment 235225 [details] [review] Add GVfsGoaVolumeMonitor
Review of attachment 235225 [details] [review]: (In reply to comment #21) > > Oh and forgot to mention that some (user visible) error messages should be > > probably made translatable. > > Is it ok to mark the error messages that are going to be passed to > GSimpleAsyncResult? I am not 100% sure. Probably, it looks good. Messages for translation can be dealt with later. Thanks for the fast turnaround, approved.
Added the "goa volume monitor" bugzilla component, please report any bugs there.