Created attachment 270358 [details] [review]
filesystem: Bail out early for cancelled operations

Created attachment 270359 [details] [review]
filesystem: Better error message when browsing fails

We try to keep the original error domain and code, so that
interested front-ends can act upon the errors. For example,
a G_IO_ERROR/G_IO_ERROR_NOT_MOUNTED error could trigger something
to mount the item.

Comment on attachment 270358 [details] [review]
filesystem: Bail out early for cancelled operations

Attachment 270358 [details] pushed as 44f81e4 - filesystem: Bail out early for cancelled operations

Review of attachment 270359 [details] [review]:

::: src/filesystem/grl-filesystem.c
@@ +1147,3 @@
+    GError *error_new = g_error_new (error->domain,
+                                     error->code,
+                                     _("File %s does not exist"),

While I understand the point of this commit, I think it is enough with keeping the error->code. After all it is prefixed with G_IO_ERROR_ so it is clear the error comes from GIO, and has enough information with this. I would keep the domain as GRL_CORE_ERROR, like originally, because from application pov it is grilo who creates the error. So instead of having G_IO_ERROR/G_IO_ERROR_NOT_MOUNTED  we would have GRL_CORE_ERROR/G_IO_ERROR_NOT_MOUNTED, which provides the information we need.

Also, should we create our own custom "File %s does not exist" message, or rather use the original error message?

(In reply to comment #4)
> Review of attachment 270359 [details] [review]:
> 
> ::: src/filesystem/grl-filesystem.c
> @@ +1147,3 @@
> +    GError *error_new = g_error_new (error->domain,
> +                                     error->code,
> +                                     _("File %s does not exist"),
> 
> While I understand the point of this commit, I think it is enough with keeping
> the error->code. After all it is prefixed with G_IO_ERROR_ so it is clear the
> error comes from GIO, and has enough information with this. I would keep the
> domain as GRL_CORE_ERROR, like originally, because from application pov it is
> grilo who creates the error. So instead of having
> G_IO_ERROR/G_IO_ERROR_NOT_MOUNTED  we would have
> GRL_CORE_ERROR/G_IO_ERROR_NOT_MOUNTED, which provides the information we need.

You can't mix and match error codes from different domains. G_IO_ERROR_NOT_MOUNTED is error code 16. GRL_CORE_ERROR_NOTIFY_CHANGED_FAILED is error code 16 for the GRL_ERROR domain.

> Also, should we create our own custom "File %s does not exist" message, or
> rather use the original error message?

The GIO error message is probably too technical and likely lacks the URL, which is why I simplified it. This is basically the exact same error, but with the original domain and error code so that front-ends can try and fix the problem (such as starting a mount).

(In reply to comment #5)
> The GIO error message is probably too technical and likely lacks the URL, which
> is why I simplified it. This is basically the exact same error, but with the
> original domain and error code so that front-ends can try and fix the problem
> (such as starting a mount).

And is this correct? When developing a library AB that is using internally another library XZ, is it correct to expose GErrors from XZ directly using the domain/code from XZ, instead of AB? Shouldn't we use XZ domain/code, which is what the application actually use?

I think it's fine, if you mention in the API documentation that errors might be from domain AB and from domain XZ. There are a number of APIs in GLib/GIO that do that.

Attachment 270359 [details] pushed as 9abd83d - filesystem: Better error message when browsing fails