New: Preview file from offline passed file #326

disist · 2017-08-21T16:18:22Z

Hello guys,

It continues of the pull request #254
I have created a new (clean) pull request with our latest agreements

Guys, @tonyjin, @priyajeet Are you ok with this implementation?

To remind our goal: learn previewer to show files from offline storage when we have no internet.

tonyjin

Hey Dmitry, thanks for the PR and apologies for the lack of communication from our end. Both hiring and preparation for our annual developer conference have started ramping up so we've both been a bit busy.

I left comments on the areas you shouldn't need to add from your end since I'll make a new patch updating the function signature for show().

This should be the flow, please check if I'm wrong:

You make the fetch file info call from your end, generate your offline link, and prepare a well-formed file object that substitutes the PDF url_template with your offline link
We modify show() from our end to accept a file object OR a file ID as the first parameter
You pass in your file object and the skipServerCheck option into show()
Preview should load with your offline file

I'll aim to have the show() modification done today hopefully, within the week at latest.

tonyjin · 2017-08-21T19:15:45Z

src/lib/Preview.js

+        // Save reference to the file when a well-formed file object is present in options
+        if (this.previewOptions.file) {
+            this.updateFileCache(this.previewOptions.file);
+        }


Remove this. I'll try and get a different PR in this week for allowing the file object to be directly sent into preview.show() as the first param.

The signature will be like:

/** * Primary function for showing a preview of a file. * * @param {string|Object} fileIdOrFile - Box File ID or well-formed Box File object * @param {string|Function} token - Access token string or generator function * @param {Object} [options] - Optional preview options * @return {void} */

Okay, removed, agree

tonyjin · 2017-08-21T19:15:55Z

src/lib/Preview.js

+        if (this.previewOptions.file) {
+            this.loadPreviewWithTokens({});
+            return;
+        }


Remove this, see above.

Will need a guard for the line below

So, when we pass well-formed file object, we need to call this.loadPreviewWithTokens but we don't pass the token and in lines below, we call this.loadPreviewWithTokens after executing getTokens, so as a proposal with according to your pull request #328, can I use guard here:

if (typeof fileIdOrFile === 'object') { this.loadPreviewWithTokens({}); return; }

?

tonyjin · 2017-08-21T19:16:03Z

src/lib/Preview.js

@@ -613,6 +623,12 @@ class Preview extends EventEmitter {
        // Save the reference to any additional custom options for viewers
        this.options.viewers = options.viewers || {};

+        // Save the reference to file object
+        this.options.file = options.file || null;


Remove this, see above.

Okay, removed

tonyjin · 2017-08-21T19:19:21Z

src/lib/Preview.js

-            this.logPreviewEvent(this.file.id, this.options);
+            if (!this.options.skipServerUpdate) {
+                this.logPreviewEvent(this.file.id, this.options);
+            }


I have conflicting thoughts about this. On one hand I see that your offline use case wouldn't require this logging call but on the other hand there could be generic use case where we don't want to refresh from the server but the developer is not using an offline use case.

I don't think we need a separate option for this, and I'd err on the side of having this logging call fail in your offline use case (ie remove the if check)

Yeah lets remove this if check. And just let the request fail it was offline.

Might still have to do something about line 924 below this.prefetchNextFiles();
Likely add a guard in line 1086 if (this.collection.length < 2 || this.options.skipServerUpdate)

Guys, agree, let the request fail it was offline.
@priyajeet Thanks for good catch about this.prefetchNextFiles(). I've added the guard.

tonyjin · 2017-08-21T22:41:38Z

FYI, the initial PR for updating show() is in code review: #328

disist · 2017-08-22T12:49:43Z

@tonyjin @priyajeet Thanks for the fast response. It's not a problem to wait, it's a life :)
Besides, thanks for the PR with the new signature, I agree with you.

After your comments I have one question regarding one comment on 513 line in Previewer, could you please look here.

And one question regarding you PR #328, should I add original representation on my side? (as it usually did with execution addOriginalRepresentation method in file.js in scope of execution chacheFile)?

Described flow above is correct.

tonyjin · 2017-08-22T19:50:11Z

@disist good catch. Faking the 'original' representation is a construct on Preview's end and isn't something we should expect the API to return. To account for other use cases, I think you should add a cacheFile() in loadPreviewWithTokens() like:

// If file object is well-formed, load with it. Otherwise, fetch file information from the server.
if (checkFileValid(this.file)) {
    // Save file in cache. This also adds the 'ORIGINAL' representation.
    cacheFile(this.file);
    this.loadFromCache();
} else {
    this.loadFromServer();
}

tonyjin · 2017-08-22T19:46:45Z

src/lib/Preview.js

+        if (typeof fileId === 'object') {
+            this.loadPreviewWithTokens({});
+            return;
+        }


Good point. How about this formatting?

if (typeof fileId === 'object') { // Load without token if using well-formed file object this.loadPreviewWithTokens({}); } else { // Fetch tokens before proceeding getTokens(this.file.id, this.previewOptions.token) .then(this.loadPreviewWithTokens) .catch(this.triggerFetchError); }

No problem, thanks

disist · 2017-08-23T15:04:08Z

@tonyjin Thank you, I have added cacheFile() in loadPreviewWithTokens().
For the first time it's worked it's worked approach for us.

And also one question, should we add check on the well-formed file object when I pass file object and not pass token, I mean in show(wellFormedFileObject, null, options)?

tonyjin · 2017-08-23T19:41:19Z

Good thought! We added it to https://github.com/box/box-content-preview/blob/master/src/lib/Preview.js#L503 as part of the show() change. At some point in the future we may be calling load() directly with a file object internally so we thought the validation worked better there.

disist · 2017-08-29T11:56:44Z

Hello, Guys! If everything is ok, let's merge this PR.

tonyjin · 2017-08-29T18:01:20Z

@disist done - thanks for your contribution!

disist and others added 2 commits August 21, 2017 16:15

New: Preview file from well passed file

ddc4573

Merge branch 'master' into display-from-file

c938580

tonyjin reviewed Aug 21, 2017

View reviewed changes

tonyjin mentioned this pull request Aug 21, 2017

New: Support well-formed file object in show() #328

Merged

disist and others added 2 commits August 22, 2017 15:39

Chore: Use well-formed file object which will pass in show directly

dbfdd8c

Merge branch 'master' into display-from-file

56a04f5

tonyjin reviewed Aug 22, 2017

View reviewed changes

disist added 2 commits August 23, 2017 17:14

Merge branch 'master' into offline-file

c9e0562

Chore: Load previewer from well-formed file object. Cache passed file.

f508c65

disist and others added 2 commits August 25, 2017 14:58

Merge branch 'master' into display-from-file

8aeef27

Merge branch 'master' into display-from-file

74f57f6

tonyjin approved these changes Aug 25, 2017

View reviewed changes

Merge branch 'master' into display-from-file

58c8f12

tonyjin merged commit 8547bc8 into box:master Aug 29, 2017

disist deleted the display-from-file branch August 29, 2017 19:51

JustinHoldstock mentioned this pull request Nov 1, 2017

Add caching (for offline mode) #464

Closed

dolgachio mentioned this pull request May 14, 2018

[Bug] The preview of files stored on a device does not work #793

Closed

JustinHoldstock mentioned this pull request Jan 25, 2019

Update: box-annotations to v3.7.1 #897

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: Preview file from offline passed file #326

New: Preview file from offline passed file #326

disist commented Aug 21, 2017

tonyjin left a comment

tonyjin Aug 21, 2017

disist Aug 22, 2017

tonyjin Aug 21, 2017

priyajeet Aug 21, 2017

disist Aug 22, 2017

tonyjin Aug 21, 2017

disist Aug 22, 2017

tonyjin Aug 21, 2017

priyajeet Aug 21, 2017

priyajeet Aug 21, 2017 •

edited

Loading

disist Aug 22, 2017

tonyjin commented Aug 21, 2017 •

edited

Loading

disist commented Aug 22, 2017 •

edited

Loading

tonyjin commented Aug 22, 2017

tonyjin Aug 22, 2017

disist Aug 23, 2017

disist commented Aug 23, 2017

tonyjin commented Aug 23, 2017

disist commented Aug 29, 2017

tonyjin commented Aug 29, 2017

New: Preview file from offline passed file #326

New: Preview file from offline passed file #326

Conversation

disist commented Aug 21, 2017

tonyjin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyajeet Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyjin commented Aug 21, 2017 • edited Loading

disist commented Aug 22, 2017 • edited Loading

tonyjin commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

disist commented Aug 23, 2017

tonyjin commented Aug 23, 2017

disist commented Aug 29, 2017

tonyjin commented Aug 29, 2017

priyajeet Aug 21, 2017 •

edited

Loading

tonyjin commented Aug 21, 2017 •

edited

Loading

disist commented Aug 22, 2017 •

edited

Loading