What kind of documents are you trying to access? I can open PDFs stored on OneDrive.

mkrajnak confirmed that the problem is with ODFs, OOXMLs, etc. and I can reproduce it. Patch coming up.

Created attachment 349003 [details] [review]
documents: Track nfo:fileName for later use

Created attachment 349004 [details] [review]
documents: Add property to denote the URI to be loaded by the preview

Created attachment 349005 [details] [review]
documents: Use logError to report loading failures

Created attachment 349006 [details] [review]
documents: Let everybody provide their own download implementation

Created attachment 349007 [details] [review]
documents: Implement the downloadImpl vfunc for SkydriveDocument

Created attachment 349008 [details] [review]
documents: Fix previewing of LOKDocView-supported OneDrive documents

Unfortunately, communication with OneDrive is extremely flaky for me today. It makes it very hard for me to test this as well I'd like to. So any help in that regard is very welcome.

Review of attachment 349003 [details] [review]:

OK

Review of attachment 349005 [details] [review]:

OK

Review of attachment 349004 [details] [review]:

This looks good but at the same time, if different backends have different network capabilities (some that can load from the network, and some that can't), it feels a bit awkward to go necessarily through an URI for all of them.

Review of attachment 349006 [details] [review]:

::: src/documents.js
@@ +399,3 @@
+                }));
+        } else {
+                        info = object.query_info_finish(res);

Nitpick: prefer checking for this case upfront and returning early.

@@ +423,3 @@
+                             'Cannot set mtime on the cache file; cache will not be valid after the file has ' +
+                             'been viewed.');
+                    }

No semicolon needed here.

Review of attachment 349007 [details] [review]:

OK

Review of attachment 349008 [details] [review]:

Looks good.

(In reply to Cosimo Cecchi from comment #12)
> Review of attachment 349004 [details] [review] [review]:
> 
> This looks good but at the same time, if different backends have different
> network capabilities (some that can load from the network, and some that
> can't), it feels a bit awkward to go necessarily through an URI for all of
> them.

I used an URI instead of a path simply because an URI is more flexible, and, hence, more future-proof. However, right now they are only going to be file:// URIs.

I also want to improve the network efficiency of gnome-documents (and also gnome-photos) a bit. For example, even if some remote content has native support in GIO/GVfs, I'd like to have an offline cache. As far as I know, GVfs doesn't do caching at the moment, so it is up to the application, and I think we already doing that for ownCloud and the other remote types. (Apart from some bugs where we hit the network even though we don't need to.)

So far all the caching was encapsulated inside GdPdfLoader. However we now have preview widgets other than EvView so things need not be tied to PDF anymore. So, I want to separate the caching from the previewing technology. The widgets only load from this property that points to the local/offline copy of the document, or the 'docModel'. It's upto the DocCommon subclasses to deal with the caching and ensuring that the property and the docModel are valid.

Then going forward, we can drastically simplify GdPdfLoader by removing all the unoconv bits, moving the network communication to JavaScript, and just having an introspectable API that loads a EvDocumentModel from a URI (assuming that's not introspectable).

However, I have a feeling that I misunderstood you. So, if this is not what you meant, could you expand a bit? :)

I just got this along with the "Oops! Unable to load ..." message:
GDBus.Error:org.gtk.GDBus.UnmappedGError.Quark._rest_2dproxy_2derror_2dquark.Code5: Connection terminated unexpectedly

The corresponding backtrace is:
SkydriveDocument<._createZpjEntry/<@resource:///org/gnome/Documents/js/documents.js:1179
main@resource:///org/gnome/Documents/js/main.js:47
run@resource:///org/gnome/gjs/modules/package.js:192
@/opt/bin/gnome-documents:6

So, it is libzapojit trying to talk to goa-daemon, which is using librest to do something that failed.

I don't know if it is up to libzapojit to strip the error or gnome-documents.

Created attachment 349060 [details] [review]
documents: Let everybody provide their own download implementation

Created attachment 349061 [details] [review]
documents: Strip the D-Bus error name from the UI string

(In reply to Cosimo Cecchi from comment #13)
> Review of attachment 349006 [details] [review] [review]:
> 
> ::: src/documents.js
> @@ +399,3 @@
> +                }));
> +        } else {
> +                        info = object.query_info_finish(res);
> 
> Nitpick: prefer checking for this case upfront and returning early.
> 
> @@ +423,3 @@
> +                             'Cannot set mtime on the cache file; cache
> will not be valid after the file has ' +
> +                             'been viewed.');
> +                    }
> 
> No semicolon needed here.

Fixed both.

Communication with OneDrive over the deprecated Live SDK implemented by libzapojit is still very flaky for me. It is ominous! We should port to the new MS Graph API as soon as possible.

Review of attachment 349008 [details] [review]:

accepted-commit_now -> reviewed:

::: src/documents.js
@@ +1265,3 @@
+                                        }
+
+                                        this.loadLocal(passwd, cancellable, callback);

We need to handle EvDocument.DocumentError.ENCRYPTED in case the PDF was encrypted. Would be nice if can avoid duplicating code between loading on the first and second attempts.

(In reply to Debarshi Ray from comment #16)
> However, I have a feeling that I misunderstood you. So, if this is not what
> you meant, could you expand a bit? :)

No, that makes sense. Looking forward to the PDF loader overhaul :-)

Review of attachment 349060 [details] [review]:

::: src/documents.js
@@ +395,3 @@
+                cacheMtime /= 1000000;
+
+                                   cancellable,

Instead of changing the mtime of the cache file to match the mtime of the original file, would it not be simpler to invalidate the cache when the mtime of the original file is newer than the one in the cache?

Review of attachment 349061 [details] [review]:

Not sure if it's up to gnome-documents to do this honestly. Libzapojit should probably have its own error domain and avoid forwarding raw dbus errors.

(In reply to Cosimo Cecchi from comment #24)
> Review of attachment 349060 [details] [review] [review]:
> 
> ::: src/documents.js
> @@ +395,3 @@
> +                cacheMtime /= 1000000;
> +
> +                                   cancellable,
> 
> Instead of changing the mtime of the cache file to match the mtime of the
> original file, would it not be simpler to invalidate the cache when the
> mtime of the original file is newer than the one in the cache?

Umm... what do you mean? You mean we should also delete the cached copy if it has gone stale?

The downloading / caching JavaScript code in this patch set is a close imitation of the similar C code in GdPdfLoader. Did I miss something?

(In reply to Debarshi Ray from comment #22)
> Review of attachment 349008 [details] [review] [review]:
> 
> accepted-commit_now -> reviewed:
> 
> ::: src/documents.js
> @@ +1265,3 @@
> +                                        }
> +
> +                                        this.loadLocal(passwd, cancellable,
> callback);
> 
> We need to handle EvDocument.DocumentError.ENCRYPTED in case the PDF was
> encrypted. Would be nice if can avoid duplicating code between loading on
> the first and second attempts.

Never mind. I was confused.

I managed to test this with an encrypted OneDrive PDF. There isn't anything to "handle" apart from passing an error to the callback. The only reason we "handle" it the first time is because we force a download if the cache was corrupted.

Restoring accepted-commit_now.

Review of attachment 349008 [details] [review]:

As per comment 27

(In reply to Debarshi Ray from comment #26)
> > Instead of changing the mtime of the cache file to match the mtime of the
> > original file, would it not be simpler to invalidate the cache when the
> > mtime of the original file is newer than the one in the cache?
> 
> Umm... what do you mean? You mean we should also delete the cached copy if
> it has gone stale?
> 
> The downloading / caching JavaScript code in this patch set is a close
> imitation of the similar C code in GdPdfLoader. Did I miss something?

I was just thinking out loud... Right now we set the mtime of the cache file to the mtime of the original file, and consider it invalid when they differ. We could instead leave the mtime of the cache file to when the cache file itself is created, and consider it invalid when the mtime of the original file is newer than the cache file's mtime.
Feel free to ignore this suggestion if you don't think it's a good idea :-)

(In reply to Cosimo Cecchi from comment #29)
> (In reply to Debarshi Ray from comment #26)
> > > Instead of changing the mtime of the cache file to match the mtime of the
> > > original file, would it not be simpler to invalidate the cache when the
> > > mtime of the original file is newer than the one in the cache?
> > 
> > Umm... what do you mean? You mean we should also delete the cached copy if
> > it has gone stale?
> > 
> > The downloading / caching JavaScript code in this patch set is a close
> > imitation of the similar C code in GdPdfLoader. Did I miss something?
> 
> I was just thinking out loud... Right now we set the mtime of the cache file
> to the mtime of the original file, and consider it invalid when they differ.
> We could instead leave the mtime of the cache file to when the cache file
> itself is created, and consider it invalid when the mtime of the original
> file is newer than the cache file's mtime.

Ok, I see. Sure.

Created attachment 349255 [details] [review]
documents: Let everybody provide their own download implementation

Created attachment 349256 [details] [review]
documents: Implement the downloadImpl vfunc for SkydriveDocument

Created attachment 349303 [details] [review]
documents, pdf-loader: Fix previewing of ODFs and OOXMLs on OneDrive

Also remove the unused GdPdfLoader code paths.

Review of attachment 349255 [details] [review]:

++

Review of attachment 349256 [details] [review]:

++

Review of attachment 349303 [details] [review]:

Looks good with one minor comment.

::: src/documents.js
@@ +1262,3 @@
+                                    }));
+                            } else {
+                                    function(fromCache, error) {

Can you fold this case into the first one, and turn the second one into if (fromCache && !error.matches(...))?

Created attachment 349461 [details] [review]
documents, pdf-loader: Fix previewing of ODFs and OOXMLs on OneDrive

Review of attachment 349461 [details] [review]:

Looks good!

Thanks for all the reviews, Cosimo!

Review of attachment 349255 [details] [review]:

::: src/documents.js
@@ +393,3 @@
+
+                let cacheMtime = info.get_attribute_uint64(Gio.FILE_ATTRIBUTE_TIME_MODIFIED);
+                cacheMtime /= 1000000;

Eek! Gio.FILE_ATTRIBUTE_TIME_MODIFIED returns a value in seconds, not microseconds.

Created attachment 350108 [details] [review]
documents: Fix the retrieval of the cache's modification time

Review of attachment 350108 [details] [review]:

Whoops, good catch!