Refactor: Video detail fragment kotlin conversion #12337
Refactor: Video detail fragment kotlin conversion #12337Stypox merged 18 commits intoTeamNewPipe:refactorfrom
Conversation
Profpatsch
force-pushed
the
video-detail-fragment-kotlin-conversion
branch
from
32e2053 to
2427ad1
Compare
| binding!!.detailThumbnailImageView.setImageDrawable( | ||
| AppCompatResources.getDrawable(requireContext(), R.drawable.not_available_monkey) | ||
| ) |
There was a problem hiding this comment.
| binding!!.detailThumbnailImageView.setImageDrawable( | |
| AppCompatResources.getDrawable(requireContext(), R.drawable.not_available_monkey) | |
| ) | |
| binding!!.detailThumbnailImageView.setImageResource(R.drawable.not_available_monkey) |
There was a problem hiding this comment.
According to the docs setImageResource does decoding on the UI thread while setImageDrawable does not, and I can't understand if AppCompatResources.getDrawable itself does the decoding or if it uses a cache or something like that. Not so important anyway, as this will be ported to Jetpack Compose.
| private val preDrawListener: ViewTreeObserver.OnPreDrawListener = | ||
| object : ViewTreeObserver.OnPreDrawListener { | ||
| override fun onPreDraw(): Boolean { | ||
| val metrics = getResources().getDisplayMetrics() | ||
|
|
||
| if (getView() != null) { | ||
| val height = ( | ||
| if (DeviceUtils.isInMultiWindow(activity)) | ||
| requireView() | ||
| else | ||
| activity.getWindow().getDecorView() | ||
| ).getHeight() | ||
| setHeightThumbnail(height, metrics) | ||
| requireView().getViewTreeObserver().removeOnPreDrawListener(preDrawListener) | ||
| } | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Oh cool! However removeOnPreDrawListener() is called in other places, too, and at the same time it's not always called inside the function body (e.g. if getView() == null), so I don't know if what we have here is precisely one-shot, but in some edge cases it may be 0-shot or multishot.
Ooops I read this only now, I don't know why I did not notice before 😟. I just pushed a commit to fix the kotlin style, feel free to revert it if you have already done it elsewhere. |
There was a problem hiding this comment.
I would argue that the changes in this file are not needed, but I'm fine either way
Stypox
force-pushed
the
video-detail-fragment-kotlin-conversion
branch
from
e012d8d to
d4bf7ed
Compare
Stypox
force-pushed
the
video-detail-fragment-kotlin-conversion
branch
from
f540fe1 to
7558448
Compare
Stypox
left a comment
Stypox
left a comment
There was a problem hiding this comment.
@Profpatsch sorry for making so many changes, I got carried away 😅, and it was probably useless as it's going to get rewritten. Anyway, I reviewed my changes twice just to be sure the same logic was maintained, and I compared the Kotlin code with the Java code and everything seems still correct.
|
ShareASmile
added
code quality
Both are only used once, and it’s easier to see what’s going on if they are just inlined in `onCreate`.
Just the conversion, errors still there for easier rebasing later.
Mostly 1:1, I had to fix a few places where the automatic conversion did not infer the right kotlin types, and places where it tried to convert to `double` instead of using `float` like the original. Everything else is the result of automatic conversion.
mediaSession is now `@NonNull`, so the getter is as well.
Stypox
force-pushed
the
video-detail-fragment-kotlin-conversion
branch
from
1e47816 to
046ea73
Compare
|
Conflicts fixed by looking at 006b4c9...refactor, where 006b4c9 is the commit this PR was based on. These are the differences in the three files with conflicts: 
|
4 participants
What is it?
Description of the changes in your PR
This is a bunch of refactors that convert PlayerHolder and VideoDetailFragment to kotlin.
Please don’t review for “kotlin style”, there’s a lot more commits that are already done that do the fixup of the kotlin style.
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. You can find more info and a video demonstration on this wiki page.
Due diligence