Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prefetch previous and next images in preview. #1627

Merged
merged 13 commits into from
Nov 15, 2021

Conversation

niubility000
Copy link
Contributor

@niubility000 niubility000 commented Oct 19, 2021

This will auto prefetch the previous and next images in preview. A very useful feature! Most of other projects have this one.
The effects will be more obvious if your Internet speed is low. So using a cellphone (close wifi) to test it. It's very fast to show the previous or next pic when you switch.

Filebrowser uses "lazy load", "cache", "prefetch imgs", and it can disable preview resize, disable type detection by header. All of these make filebrowser the fastest and most stable project compared with others. And it's written in golang, which allows it to run in low-end servers.
Very good.

@mckaygerhard
Copy link

great improvement.. i reviewed the code and looks good.. please @o1egl ..

@niubility000
Copy link
Contributor Author

niubility000 commented Oct 22, 2021

great improvement.. i reviewed the code and looks good.. please @o1egl ..

Yes, I also think it's cool.

frontend/src/views/files/Preview.vue Outdated Show resolved Hide resolved
frontend/src/views/files/Preview.vue Outdated Show resolved Hide resolved
@niubility000
Copy link
Contributor Author

niubility000 commented Oct 25, 2021

All fixed.
I just can't see my reply to o1egl,...it says "pending", weird. I put it here.

#1627 (comment)
Fixed. Redeced to two variables.

#1627 (comment)
previousLink and nextLink are for Router use. In preview.vue, it always starts with /files/....,and it's an relative path.
It's defined in the router/index.js,

...
path: "/files/*",
name: "Files",
component: Files,
...
But img src (raw) needs to use an absolute path. and starts with ${baseURL}/api/preview/big/...or ${baseURL}/api/raw/... in preview.vue.
You can see how we get this "raw" in the computed:{}. "raw" is used for the src of image, video, audio, and blob. and the href in link rel="prefetch" tab needs to use the same path format.

So here we need change the previousLink from /files/... to ${baseURL}/api/preview/big/...., slice(6) will delete the first 6 characters "/files", and ${baseURL}/api/preview/big${this.previousLink.slice(6)}?k=${key}` will get the whole path for the previous and next raw.

You can check all the required path format using console.log.

#1627 (comment)
Fixed.
In strict mode code, functions can only be declared at top level or inside a block. So I put it outside.

@niubility000
Copy link
Contributor Author

#1627 (comment)
I can't figure it out. Is this a direct and faster way to get the URL?
Currently, we create all the URL of raw (including the thumbnails) in this (a little bit) complicated way. If using the file fields to create the URL is faster, we can make another PR to change them thoroughly.

@ramiresviana
Copy link
Contributor

You are using the slice() method, making hard to understand the code. Using the file fields to construct the URL is more descriptive.

@niubility000
Copy link
Contributor Author

You are using the slice() method, making hard to understand the code. Using the file fields to construct the URL is more descriptive.

Yes, we need to use console.log to see the difference of the path formats.

@niubility000
Copy link
Contributor Author

@ramiresviana
@o1egl
Please review it again. There's no bug and it works great.

@o1egl
Copy link
Member

o1egl commented Nov 10, 2021

You are using the slice() method, making hard to understand the code. Using the file fields to construct the URL is more descriptive.

@niubility000 please address that comment before merging this PR

@niubility000
Copy link
Contributor Author

niubility000 commented Nov 10, 2021

You are using the slice() method, making hard to understand the code. Using the file fields to construct the URL is more descriptive.

@niubility000 please address that comment before merging this PR

It's not necessary to use the file fields to construct the URL. As @ramiresviana said, it will not make it faster, just make it more descriptive for the coder to read.

Currently we just calculate the prefetch link in the same way as we calculate the raw path. I think it's fine, it's easy to understand.

@o1egl
Copy link
Member

o1egl commented Nov 11, 2021

You are using the slice() method, making hard to understand the code. Using the file fields to construct the URL is more descriptive.

@niubility000 please address that comment before merging this PR

It's not necessary to use the file fields to construct the URL. As @ramiresviana said, it will not make it faster, just make it more descriptive for the coder to read.

Currently we just calculate the prefetch link in the same way as we calculate the raw path. I think it's fine, it's easy to understand.

I would prefer to explicitly construct URL instead of making manipulations on the current path

frontend/src/views/files/Preview.vue Outdated Show resolved Hide resolved
frontend/src/views/files/Preview.vue Outdated Show resolved Hide resolved
frontend/src/views/files/Preview.vue Outdated Show resolved Hide resolved
niubility000 and others added 3 commits November 12, 2021 22:36
Co-authored-by: Ramires Viana <59319979+ramiresviana@users.noreply.github.com>
Co-authored-by: Ramires Viana <59319979+ramiresviana@users.noreply.github.com>
Co-authored-by: Ramires Viana <59319979+ramiresviana@users.noreply.github.com>
@niubility000
Copy link
Contributor Author

niubility000 commented Nov 12, 2021

Thank you, @ramiresviana,
@o1egl, now it's ok.

@o1egl o1egl merged commit 7401d16 into filebrowser:master Nov 15, 2021
rahul-r pushed a commit to rahul-r/filebrowser that referenced this pull request Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants