Replace UniversalImageLoader with Picasso #5928
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AudricV
added
bug
TobiGr
added
the
codequality
|
Hey @Stypox, thanks for your work 🙏 |
|
Oh no... why does it still crash? Maybe the bitmap downloaded from the website itself is broken? Could you provide me the video url? |
|
This video is one of the rebellious videos. |
|
Mmmh, this is really strange. I tried copying the thumbnail and returning the copy and I got this strange error... What does it mean?!? |
|
@arielshulman could you test this APK? I implemented what you did in the issue, i.e. resizing to Oh, btw, some way or another I was able to cause a native stacktrace even on my phone, but this is probably just caused by me recycling the wrong thing ( |
This is really strange bitmap and those libraries seems not to like working with it, but this error seems to be lack of good info 🤷♂️ I've tried downloading the debug apk but for some reason youtube doesn't work with it. |
|
Tried to backtrace (without my laptop, here ) |
|
@arielshulman please test the APK that should be built by the CI within minutes from now (I have no idea why my apk does not work) |
|
Please don't forget to attribute the new library in the about screen (and to remove the replaced one) |
It seems the apk build failed, or I'm missing something. Thanks! |
Stypox
force-pushed
the
picasso
branch
2 times, most recently
from
3a6c14d
to
0749e1e
Compare
|
@arielshulman for some reason checkstyle was crashing with a totally not understandable and unrelated error, but the real problem was a syntax error caused by rebasing. I don't know how that slipped through. Anyway, now everything should have been built correctly, the apk is here: https://github.com/TeamNewPipe/NewPipe/suites/2389524359/artifacts/50769605 |
|
🤸 Yay! |
|
@Stypox Oh btw, you'll have to rebase or fix the build.gradle confliction from my library rearrangement PR, as a heads up |
Done |
|
@Stypox By the way, I'm curious, why did you decide to go with Picasso over Glide? Nothing against Picasso - it's just that the two libraries' APIs are somewhat similar - but Picasso seems to get more sporadic / spotty maintenance while Glide is more regularly maintained, so I figured I'd ask. |
|
I went with Picasso because it is more lightweight but still has all the features we need. Even if it has less maintenance it comes from the same organization as OkHttp, so I think we can use it safely |
XiangRongLin
reviewed
Apr 3, 2021
app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/info_list/holder/CommentsMiniInfoItemHolder.java
Outdated
Show resolved
Hide resolved
B0pol
reviewed
Apr 5, 2021
|
@jimman2003 I would still switch, Picasso has a better behaviour imho |
@jimman2003 Where do you see that? The last commit is over a year ago. And the message saying the dev will not work from November 27 (presumably 2020) is still there. |
Oops well. Misread the commit log + README😭 |
|
Rebased and applied your first suggestion |
|
Sorry, I forgot about this. I tested this thoroughly. The PR dhould be good to merge after a rebase |
|
|
One thing I wonder about is if it would be possible to remove that gray placeholder which appears till the image is loaded and just leave it blank, as it is currently. It's just my personal taste, but it doesn't look that great. |
|
@tsiflimagas you are right, done. |
|
@Stypox |
TobiGr
previously approved these changes
Aug 13, 2021
|
nooooo.... |
This prevents strange crashes on some devices, fixes TeamNewPipe#4638
|
@TobiGr rebased ;-) |
TobiGr
approved these changes
Aug 24, 2021
evermind-zz
reviewed
Aug 31, 2021
| @@ -117,6 +119,7 @@ public void onTerminate() { | |||
| disposable.dispose(); | |||
| } | |||
| super.onTerminate(); | |||
| PicassoHelper.terminate(); | |||
There was a problem hiding this comment.
Just wondering why are you calling PicassoHelper.terminate() after super.onTerminate() call and not before?
There was a problem hiding this comment.
I don't know if it makes any difference, but I think I chose to do it this way because there could still be something doing image processing before super.onTerminate(), but surely there isn't after it.
There was a problem hiding this comment.
I see it seems it is totally irrelevant what is done in this method on a real device. It is never called according to:
https://developer.android.com/reference/android/app/Application#onTerminate()
There was a problem hiding this comment.
Mmmh... should we remove it? @TobiGr
There was a problem hiding this comment.
I think so. That's at least how I understand the docs
|
@Poolitzer what? #908 is already closed and has nothing to do with this |
@tsiflimagas This has been reverted in #8530, see there for an explanation, sorry 😅 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is it?
Description of the changes in your PR
This PR replaces the outdated Android-Universal-Image-Loader (which has not had any new release in the last 6 years) with the more stable and updated Picasso. This hopefully fixes a nasty unfixed issue in Android-Universal-Image-Loader that was causing #4638. Also, the loading seems smoother to me, and retrieving images from memory cache is now instant, instead of taking a couple of milliseconds (try scrolling fast up and down after all thumbnails have been loaded).
I also added a switch in the debug settings to enable indicators.
Fixes the following issue(s)
Fixes #4638
APK testing
@gulachev @k3a @Sekitan @UserX404 @nonremittal @arielshulman @dboydor @pauseee @astatine (taken from #4638)
Could you test this APK from the CI and see if it fixes the issue? Thank you ;-)
Due diligence