After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 706158 - Add an ownCloud miner
Add an ownCloud miner
Status: RESOLVED FIXED
Product: gnome-online-miners
Classification: Applications
Component: general
unspecified
Other All
: Normal enhancement
: ---
Assigned To: GNOME Online Miners maintainer(s)
GNOME Online Miners maintainer(s)
Depends on: 706074
Blocks: 686527
 
 
Reported: 2013-08-16 18:43 UTC by Debarshi Ray
Modified: 2013-08-20 14:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
data: add a DBus service file for org.gnome.OnlineMiners.ownCloud (1.84 KB, patch)
2013-08-19 07:05 UTC, Debarshi Ray
committed Details | Review
utils: Add .odt, .odp and .ods to gom_filename_to_rdf_type (1.58 KB, patch)
2013-08-19 07:06 UTC, Debarshi Ray
committed Details | Review
src: add a ownCloud miner (17.79 KB, patch)
2013-08-19 07:07 UTC, Debarshi Ray
none Details | Review
src: add a ownCloud miner (17.79 KB, patch)
2013-08-19 10:40 UTC, Debarshi Ray
none Details | Review
src: add a ownCloud miner (17.91 KB, patch)
2013-08-19 12:00 UTC, Debarshi Ray
reviewed Details | Review
src: add a ownCloud miner (18.30 KB, patch)
2013-08-20 13:13 UTC, Debarshi Ray
reviewed Details | Review
src: add a ownCloud miner (18.33 KB, patch)
2013-08-20 13:47 UTC, Debarshi Ray
committed Details | Review

Description Debarshi Ray 2013-08-16 18:43:33 UTC
This is required by gnome-documents to picking up documents from ownCloud accounts.
Comment 1 Debarshi Ray 2013-08-19 07:05:25 UTC
Created attachment 252178 [details] [review]
data: add a DBus service file for org.gnome.OnlineMiners.ownCloud
Comment 2 Debarshi Ray 2013-08-19 07:06:16 UTC
Created attachment 252179 [details] [review]
utils: Add .odt, .odp and .ods to gom_filename_to_rdf_type
Comment 3 Debarshi Ray 2013-08-19 07:07:01 UTC
Created attachment 252180 [details] [review]
src: add a ownCloud miner
Comment 4 Debarshi Ray 2013-08-19 10:40:53 UTC
Created attachment 252196 [details] [review]
src: add a ownCloud miner

Missed a colon in "gd:collection:".
Comment 5 Debarshi Ray 2013-08-19 12:00:52 UTC
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.
Comment 6 Cosimo Cecchi 2013-08-20 11:57:42 UTC
Review of attachment 252178 [details] [review]:

OK
Comment 7 Cosimo Cecchi 2013-08-20 11:58:11 UTC
Review of attachment 252179 [details] [review]:

OK
Comment 8 Cosimo Cecchi 2013-08-20 12:30:55 UTC
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 9 Debarshi Ray 2013-08-20 12:37:52 UTC
Comment on attachment 252178 [details] [review]
data: add a DBus service file for org.gnome.OnlineMiners.ownCloud

Thanks for the review.
Comment 10 Debarshi Ray 2013-08-20 12:38:12 UTC
Comment on attachment 252179 [details] [review]
utils: Add .odt, .odp and .ods to gom_filename_to_rdf_type

Thanks for the review.
Comment 11 Debarshi Ray 2013-08-20 13:09:28 UTC
(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?
Comment 12 Debarshi Ray 2013-08-20 13:13:38 UTC
Created attachment 252415 [details] [review]
src: add a ownCloud miner
Comment 13 Cosimo Cecchi 2013-08-20 13:21:35 UTC
(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.
Comment 14 Cosimo Cecchi 2013-08-20 13:29:46 UTC
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.
Comment 15 Debarshi Ray 2013-08-20 13:47:26 UTC
Created attachment 252423 [details] [review]
src: add a ownCloud miner
Comment 16 Cosimo Cecchi 2013-08-20 13:55:19 UTC
Review of attachment 252423 [details] [review]:

Looks good to me now.
Comment 17 Debarshi Ray 2013-08-20 14:00:50 UTC
(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 18 Debarshi Ray 2013-08-20 14:02:17 UTC
Comment on attachment 252423 [details] [review]
src: add a ownCloud miner

Thanks for the review!