-
-
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
Allow selecting image quality among multiple images #10062
Conversation
bd3d6ef
to
7351ff7
Compare
This comment was marked as resolved.
This comment was marked as resolved.
That's unrelated to this PR and is fixed in the channel tabs PR. I don't know why suddenly that issue started arising, as the code there didn't change... Probably a kotlin update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me overall, I checked on an Android 5.0 emulator and the image loading seems to be working well. I've suggested a few small changes.
app/src/main/java/org/schabi/newpipe/util/image/ImageStrategy.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/util/image/ImageStrategy.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/util/image/ImageStrategy.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/util/image/ImageStrategy.java
Outdated
Show resolved
Hide resolved
This comment was marked as duplicate.
This comment was marked as duplicate.
There is nothing we can really do I am afraid. The extractor returns the thumbnails that YouTube returns on its desktop website for search results. It is not planned to build custom thumbnails ourselves on YouTube outside of RSS feeds and mixes, on which only one thumbnail or no thumbnail is available, as YouTube provides thumbnails without black borders and these thumbnails require signatures and parameters we are not able to generate. Also, not every thumbnail resolution is available on all videos (look at the extractor PR for more details). Using different thumbnails would create a different behavior compared to YouTube official clients in more places than RSS feeds and mixes, and I think we like to avoid that as much as we can. |
Alright, I can't complain anymore, you already did your best to provide us with such a great app. Thank you again. |
69ee195
to
79b6ede
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just some small things
app/src/main/java/org/schabi/newpipe/fragments/detail/DescriptionFragment.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/util/image/PicassoHelper.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/util/image/PicassoHelper.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/util/image/PicassoHelper.java
Outdated
Show resolved
Hide resolved
79b6ede
to
bfb00fc
Compare
Now using multiple comparison steps instead of magic values
imageListToDbUrl should be used if the URL is going to be saved to the database, to avoid saving nothing in case at the moment of saving the user preference is to not show images.
9f3f9e5
to
f8b756c
Compare
Kudos, SonarCloud Quality Gate passed! |
What is it?
Description of the changes in your PR
getThumbnailUrl()
), the extractor now provides aList<Image>
(e.g.getThumbnails()
), and each image haswidth
,height
andresolutionLevel
attributes that allow determining an image's quality.List<Image>
. The saved URL is the preferred one at the time of saving. This keeps it simple, avoids needing to do database migrations (not really a big deal, but still) and saves storage. Otherwise we would probably double the database size just to store a whole lot of images that will never be all used anyway. The only downside is that upon changing preferred resolution level in settings, only newly fetchedList<Image>
s will have the correct level, while videos in history/feed will not. I don't think it's such a big deal, also because when opening a (remote) video (or playlist), the thumbnail(s) would get updated to the newly set preferred resolution level.Before/After Screenshots/Screen Record
TODO
PlaylistRemoteEntity.isIdenticalTo
behave? - it's fine if the thumbnail is updated in the database if the current preference has changed (since that implieschoosePreferredImage
returns a different url than the one saved in the database, and henceisIdenticalTo
returnsfalse
)ImageStrategy
Fixes the following issue(s)
Relies on the following changes
TeamNewPipe/NewPipeExtractor#889
APK testing
The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.
At the moment the highest quality thumbnails will load everywhere, and that's the only noticeable difference with normal NewPipe, until a quality setting is added.
Due diligence