GNOME Bugzilla – Bug 706158
Add an ownCloud miner
Last modified: 2013-08-20 14:02:41 UTC
This is required by gnome-documents to picking up documents from ownCloud accounts.
Created attachment 252178 [details] [review] data: add a DBus service file for org.gnome.OnlineMiners.ownCloud
Created attachment 252179 [details] [review] utils: Add .odt, .odp and .ods to gom_filename_to_rdf_type
Created attachment 252180 [details] [review] src: add a ownCloud miner
Created attachment 252196 [details] [review] src: add a ownCloud miner Missed a colon in "gd:collection:".
Created attachment 252206 [details] [review] src: add a ownCloud miner We were not adding the MIME for files that were not inside any sub-directories. Adjust the if statements accordingly.
Review of attachment 252178 [details] [review]: OK
Review of attachment 252179 [details] [review]: OK
Review of attachment 252206 [details] [review]: Looks mostly good. I have some comments below. ::: src/gom-owncloud-miner.c @@ +44,3 @@ + G_FILE_ATTRIBUTE_TIME_MODIFIED + +/* GVfs marks regular files on remote shares as UNKNOWN */ Is this expected behavior or a bug in the WebDAV backend? I don't think it's expected. @@ +255,3 @@ + } + + if (type == G_FILE_TYPE_DIRECTORY) Using "else if" here would make the code flow a bit clearer. @@ +269,3 @@ + + out: + g_clear_error (error); If you get here with a *error != NULL && is_root, it means a fatal error has been encountered. I don't think you should clear it in that case, as the error is passed in from the parent class, which will expect it to be set when something does wrong in the query process. A way to ensure that would be to pass down a different error pointer to this function when called from query_owncloud(), and then use something like g_error_propagate(). @@ +340,3 @@ + + data.job = job; + volumes = g_volume_monitor_get_volumes (priv->monitor); These are leaked. @@ +357,3 @@ + if (!found) + { + context = g_main_context_get_thread_default (); I don't understand why you need to create another nested main loop here (and below). The miner main() already runs a mainloop, no? Is it to make the code flow sync here?
Comment on attachment 252178 [details] [review] data: add a DBus service file for org.gnome.OnlineMiners.ownCloud Thanks for the review.
Comment on attachment 252179 [details] [review] utils: Add .odt, .odp and .ods to gom_filename_to_rdf_type Thanks for the review.
(In reply to comment #8) > Review of attachment 252206 [details] [review]: > ::: src/gom-owncloud-miner.c > @@ +44,3 @@ > + G_FILE_ATTRIBUTE_TIME_MODIFIED > + > +/* GVfs marks regular files on remote shares as UNKNOWN */ > > Is this expected behavior or a bug in the WebDAV backend? I don't think it's > expected. I will check and file a bug against GVfs. > @@ +255,3 @@ > + } > + > + if (type == G_FILE_TYPE_DIRECTORY) > > Using "else if" here would make the code flow a bit clearer. Are you sure "else if" would be logically correct? What I am trying to do is call _process_file if it is a regular file or directory, and then call _traverse_dir if it is a directory. So if the file is a directory we want to execute both _process_file and _traverse_dir. > @@ +269,3 @@ > + > + out: > + g_clear_error (error); > > If you get here with a *error != NULL && is_root, it means a fatal error has > been encountered. I don't think you should clear it in that case, as the error > is passed in from the parent class, which will expect it to be set when > something does wrong in the query process. > A way to ensure that would be to pass down a different error pointer to this > function when called from query_owncloud(), and then use something like > g_error_propagate(). You are right. I hope I got it right this time. > @@ +340,3 @@ > + > + data.job = job; > + volumes = g_volume_monitor_get_volumes (priv->monitor); > > These are leaked. Sorry. Fixed. > @@ +357,3 @@ > + if (!found) > + { > + context = g_main_context_get_thread_default (); > > I don't understand why you need to create another nested main loop here (and > below). The miner main() already runs a mainloop, no? Is it to make the code > flow sync here? Yes, I want to make it sync. For the first case, I want to wait till the monitor emits "volume-added" with the correct GVolume. I don't know if I should be using a thread default context in this case or not. For the second case, I couldn't find a sync equivalent of g_volume_mount. Maybe we should add it to GIO?
Created attachment 252415 [details] [review] src: add a ownCloud miner
(In reply to comment #11) > Are you sure "else if" would be logically correct? What I am trying to do is > call _process_file if it is a regular file or directory, and then call > _traverse_dir if it is a directory. > > So if the file is a directory we want to execute both _process_file and > _traverse_dir. Oh, my bad...in that case you're right, feel free to ignore my comment. > > @@ +357,3 @@ > > + if (!found) > > + { > > + context = g_main_context_get_thread_default (); > > > > I don't understand why you need to create another nested main loop here (and > > below). The miner main() already runs a mainloop, no? Is it to make the code > > flow sync here? > > Yes, I want to make it sync. > > For the first case, I want to wait till the monitor emits "volume-added" with > the correct GVolume. I don't know if I should be using a thread default context > in this case or not. > > For the second case, I couldn't find a sync equivalent of g_volume_mount. Maybe > we should add it to GIO? Yeah, we're missing a sync equivalent of that function. Unfortunately I have the feeling adding it to GIO is easier said than done, unless you similarly wrap the async op into a sync one inside GIO itself (rather than adding the sync version all the way down to GVfs). Anyway, I don't see anything particularly wrong with using that code, but refactoring it to be async might make it more in line with the style used elsewhere in GNOME.
Review of attachment 252415 [details] [review]: ::: src/gom-owncloud-miner.c @@ +275,3 @@ + out: + if (local_error != NULL && is_root) + g_propagate_error (error, local_error); I still don't like this too much. account_miner_job_traverse_dir() should always set the passed-in error when it encounters a fatal error. It's up to the caller to decide whether that error is fatal to the outer processing or not. In this case, the error is fatal for the whole query process when it happens in the topmost invocation of the function (i.e. can't traverse root) and non-fatal when it happens on a subdirectory (in which case we can just skip that). Concretely (regardless of is_root), an error in all the enumerate_* calls is fatal to the function, and an error in process_file() or in a nested traverse_dir() is not fatal.
Created attachment 252423 [details] [review] src: add a ownCloud miner
Review of attachment 252423 [details] [review]: Looks good to me now.
(In reply to comment #13) > Yeah, we're missing a sync equivalent of that function. Unfortunately I have > the feeling adding it to GIO is easier said than done, unless you similarly > wrap the async op into a sync one inside GIO itself (rather than adding the > sync version all the way down to GVfs). Yeah, that is what I had in mind -- to wrap the async into a sync. But I guess it is too late to do it this cycle. > Anyway, I don't see anything particularly wrong with using that code, but > refactoring it to be async might make it more in line with the style used > elsewhere in GNOME. Currently the query vfunc is run as a sync operation within a thread, so each subclass has to implement a sync method. This isolates the async machinery in the parent class, and doesn't put the burden on all the subclasses. Since sync operations are easier to read and write this is quite nice. We do have these corner cases where there is no sync operation (eg., g_volume_mount), or the sync operation doesn't work within a thread (eg., Grilo), and we have to wrap the asyncs ourselves. But to me, the benefits of not asking each subclass to implement an async outweighs these problems.
Comment on attachment 252423 [details] [review] src: add a ownCloud miner Thanks for the review!