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

Pull media library images through API #990

Closed
tech4him1 opened this issue Jan 5, 2018 · 20 comments
Closed

Pull media library images through API #990

tech4him1 opened this issue Jan 5, 2018 · 20 comments

Comments

@tech4him1
Copy link
Contributor

tech4him1 commented Jan 5, 2018

Currently, we use GitHub's raw.githubusercontent.com URLs, instead of pulling the media through the API. This doesn't really allow caching, and won't work with other new backends, like GitLab, since they don't necessarily have a download URL that will work for private repos.

@tech4him1
Copy link
Contributor Author

tech4him1 commented Jan 9, 2018

@Quicksaver mentioned in #994 that the tokens are included in the download_urls, so it's not an issue on private GitHub repos.

@erquhart
Copy link
Contributor

Pulling thumbnails through the API is still worthwhile because it allows caching.

@tech4him1
Copy link
Contributor Author

This will be necessary for private GitLab repos.

@tech4him1
Copy link
Contributor Author

#1092 will need to be addressed as part of this.

@tech4him1 tech4him1 self-assigned this Jun 5, 2018
@tech4him1
Copy link
Contributor Author

I just about have a PR ready for this. When first loading images, however, they must all be downloaded through the API before showing anything. Currently, using URLs instead of through the API, we show all the image filenames, and then load the thumbnails when ready. I'm thinking that is a good idea to do here as well. Maybe we can just load thumbnails with a lazy-loader when they come into view?

@erquhart @Benaiah @talves Any input on this?

@erquhart
Copy link
Contributor

Sorry, thought I commented here a while ago - lazy loading makes sense, agreed.

@tech4him1
Copy link
Contributor Author

tech4him1 commented Jun 12, 2018

This has been implemented in the main GitLab PR. Instead of using a lazy-loader, Benaiah and I ended up just loading and caching all images in the background the first time the media library was opened, using a semaphore to prevent overloading the network connection (images are large). I think it is fairly common to scroll (or search) through a media library, so this may be a better idea than using a lazy-loader. We'll have to see how well it works in real life, and then backport it to the GitHub backend.

@erquhart
Copy link
Contributor

As we finish rounding out this improvement, we should ensure image caching is happening now that we're pulling from API's. We could even cache the local file object to avoid the round trip for the user that does the initial upload.

@owenhoskins
Copy link

In my use-case I have 6000+ (~1.5gb) worth of image assets in the library, so loading them all upfront and caching seems overwhelming, although I am not familiar with how a semaphore would be put to use in this case.

@tech4him1
Copy link
Contributor Author

@owenhoskins That is a valid point. @erquhart Maybe we should wait to port the code back to the GitHub backend until thumbnail creation is in place?

@erquhart
Copy link
Contributor

Hmm there's actually no need to download all asset blobs ever, thumbnails or not. We do this with entries because their contents are subject to search, but only the titles of blobs are searched and we get those en masse.

@Benaiah is working on virtualization (I think you did too @owenhoskins just not sure where it landed) so that only visible images would load their blobs - we can move forward with that even before getting thumbnail generation in place.

@tech4him1 to answer your exact question, though, yes, we should wait on either thumbnail generation or virtualization before porting images-via-API to GitHub, agreed.

@erquhart
Copy link
Contributor

@Benaiah I thought this was done, no?

@selaux
Copy link
Contributor

selaux commented Dec 7, 2018

Correct me if I'm wrong, but as far as I can see only the media library uses the API, the editor uses getAsset(), which does not have any API involvement. It also does not recognize when the image has in fact already been loaded when visiting the media library.

@erquhart
Copy link
Contributor

@selaux that sounds about right, our primary concern was loading a billion images in the library. We should be pulling via API everywhere, agreed.

@stale
Copy link

stale bot commented Oct 29, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status: stale label Oct 29, 2019
@erezrokah
Copy link
Contributor

This was fixed here #2817

@selaux
Copy link
Contributor

selaux commented Nov 12, 2019

This looks like it is a specific fix for the github backend. Are you sure this fixes it for for other backends as well?

@selaux
Copy link
Contributor

selaux commented Nov 12, 2019

Sorry, my question was incomplete, I wanted to refer to both backends and areas where media is displayed. Basically I wanted to emphasize my previous comment here where this issue is also there when editing an entry.

@erezrokah
Copy link
Contributor

erezrokah commented Nov 12, 2019

Sorry @selaux , I should have linked to #1750.
I think that's what left of this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants