-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Comments
@Quicksaver mentioned in #994 that the tokens are included in the |
Pulling thumbnails through the API is still worthwhile because it allows caching. |
This will be necessary for private GitLab repos. |
#1092 will need to be addressed as part of this. |
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? |
Sorry, thought I commented here a while ago - lazy loading makes sense, agreed. |
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. |
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. |
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. |
@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? |
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. |
@Benaiah I thought this was done, no? |
Correct me if I'm wrong, but as far as I can see only the media library uses the API, the editor uses |
@selaux that sounds about right, our primary concern was loading a billion images in the library. We should be pulling via API everywhere, agreed. |
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. |
This was fixed here #2817 |
This looks like it is a specific fix for the github backend. Are you sure this fixes it for for other backends as well? |
This was already done for the other backends: |
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. |
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.The text was updated successfully, but these errors were encountered: