Use view binding in fragments. #4814

Isira-Seneviratne · 2020-11-06T02:07:19Z

What is it?

Bugfix (user facing)
Feature (user facing)
Codebase improvement (dev facing)
Meta improvement to the project (dev facing)

Description of the changes in your PR

Use view binding in the fragments, as Kotlin synthetics have been deprecated in favor of view binding.

APK testing

debug.zip

Due diligence

I read the contribution guidelines.

FunkyMuse · 2020-11-29T12:02:11Z

@Isira-Seneviratne
You need to release the binding inside the fragments, otherwise you're gonna have memory leaks, i wrote an article

Isira-Seneviratne · 2020-11-29T23:24:23Z

@Isira-Seneviratne
You need to release the binding inside the fragments, otherwise you're gonna have memory leaks, i wrote an article

I already did that, did I miss a fragment?

Stypox · 2020-12-12T14:14:13Z

The same I said in #4762 (comment) applies here ;-)

TacoTheDank · 2020-12-19T20:01:40Z

@Stypox Bumping this for examination and merging, as it's been updated.

Stypox

Code looks good, thank you! In the review I'm just asking clarifications for things I am not sure about.
I tested by navigating in many different fragments and everything looked good; I also took a heap dump with LeakCanary and the only leaks that were pointed out were unrelated to view binding. @TobiGr almost ready to be merged

Stypox · 2020-12-20T10:27:36Z

app/src/main/java/org/schabi/newpipe/fragments/MainFragment.java

-            viewPager.setAdapter(null);
+        if (binding != null) {
+            binding.pager.setAdapter(null);
+            binding = null;


Why are you setting binding = null here and not in onDestroyView like all other fragments?

_{(unrelated to this PR, just a note for the future) pagerAdapter should be set to null here, as LeakCanary reports it as leaking.}

Because the pager adapter is being set to null in that method, and the onDestroyView method is called before onDestroy: https://developer.android.com/guide/fragments/lifecycle.

That logic should be in onDestroyView, since the view gets re-created all the time.

onDestroy happens when the fragment is killed, what does that mean?
onDestroyView is called everytime whilst onDestroy isn't.

You should move it to onDestroyView

Stypox · 2020-12-20T10:38:40Z

app/src/main/java/org/schabi/newpipe/local/feed/FeedFragment.kt

+    private var _feedBinding: FragmentFeedBinding? = null
+    private val feedBinding get() = _feedBinding!!


Can't you just make them lateinit to prevent having to use asserting getters? Though I have little experience in Kotlin, so I may be wrong ;-)

lateinit variables can't have null assigned to them, and the variable needs to be set to null to allow the binding to be garbage collected: https://developer.android.com/topic/libraries/view-binding#fragments.

Stypox · 2020-12-20T10:40:05Z

app/src/main/java/org/schabi/newpipe/local/feed/FeedFragment.kt

        } else if (progressState.progressMessage > 0) {
-            loading_progress_text?.setText(progressState.progressMessage)
+            _feedBinding?.loadingProgressText?.setText(progressState.progressMessage)


Why are you not using feedBinding.loadingProgressText. here and below?

I was getting a NullPointerException when using the non-null feedBinding property.

Stypox · 2020-12-20T10:41:09Z

app/src/main/java/org/schabi/newpipe/local/subscription/SubscriptionFragment.kt

+    private var _binding: FragmentSubscriptionBinding? = null
+    private val binding get() = _binding!!


Also here and in other places

Stypox · 2020-12-21T22:07:44Z

@TobiGr didn't we also want to merge this before releasing 0.20.7?

TobiGr · 2020-12-21T22:20:22Z

Yes, I forgot to add this PR to the "to do" section.

Stypox · 2020-12-22T13:39:55Z

I get this binding leak with your latest apk, I don't know if it is a real leak. The heap dump was captured while I was in the VideoDetailFragment, if that counts anyhow:

┬───
│ GC Root: System class
│
├─ org.schabi.newpipe.player.helper.PlayerHolder class
│    Leaking: NO (VideoDetailFragment↓ is not leaking and a class is never
│    leaking)
│    ↓ static PlayerHolder.listener
├─ org.schabi.newpipe.fragments.detail.VideoDetailFragment instance
│    Leaking: NO (MainFragment↓ is not leaking and Fragment#mFragmentManager is
│    not null)
│    playerService instance of org.schabi.newpipe.player.MainPlayer
│    activity instance of org.schabi.newpipe.MainActivity with mDestroyed =
│    false
│    ↓ VideoDetailFragment.mFragmentManager
├─ androidx.fragment.app.FragmentManagerImpl instance
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ FragmentManagerImpl.mBackStack
├─ java.util.ArrayList instance
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ ArrayList.elementData
├─ java.lang.Object[] array
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ Object[].[0]
├─ androidx.fragment.app.BackStackRecord instance
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ BackStackRecord.mOps
├─ java.util.ArrayList instance
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ ArrayList.elementData
├─ java.lang.Object[] array
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ Object[].[0]
├─ androidx.fragment.app.FragmentTransaction$Op instance
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ FragmentTransaction$Op.mFragment
├─ org.schabi.newpipe.fragments.MainFragment instance
│    Leaking: NO (Fragment#mFragmentManager is not null)
│    activity instance of org.schabi.newpipe.MainActivity with mDestroyed =
│    false
│    ↓ MainFragment.binding
│                   ~~~~~~~
├─ org.schabi.newpipe.databinding.FragmentMainBinding instance
│    Leaking: UNKNOWN
│    Retaining 30904 bytes in 600 objects
│    ↓ FragmentMainBinding.rootView
│                          ~~~~~~~~
╰→ android.widget.RelativeLayout instance
     Leaking: YES (ObjectWatcher was watching this because org.schabi.newpipe.
     fragments.MainFragment received Fragment#onDestroyView() callback
     (references to its views should be cleared to prevent leaks))
     Retaining 1537 bytes in 36 objects
     key = 3ca5677d-b29b-4eb9-9230-dd17bda8f58e
     watchDurationMillis = 36738
     retainedDurationMillis = 26725
     View not part of a window view hierarchy
     View.mAttachInfo is null (view detached)
     View.mWindowAttachCount = 1
     mContext instance of org.schabi.newpipe.MainActivity with mDestroyed =
     false

METADATA

Build.VERSION.SDK_INT: 24
Build.MANUFACTURER: HUAWEI
LeakCanary version: 2.5
App process name: org.schabi.newpipe.debug.Useviewbindinginfragments
Stats: LruCache[maxSize=3000,hits=5153,misses=68531,hitRate=6%]
RandomAccess[bytes=3893630,reads=68531,travel=72315166573,range=25866324,size=46
366704]
Analysis duration: 11173 ms

Isira-Seneviratne · 2020-12-22T23:06:17Z

I get this binding leak with your latest apk, I don't know if it is a real leak. The heap dump was captured while I was in the VideoDetailFragment, if that counts anyhow:

┬───
│ GC Root: System class
│
├─ org.schabi.newpipe.player.helper.PlayerHolder class
│    Leaking: NO (VideoDetailFragment↓ is not leaking and a class is never
│    leaking)
│    ↓ static PlayerHolder.listener
├─ org.schabi.newpipe.fragments.detail.VideoDetailFragment instance
│    Leaking: NO (MainFragment↓ is not leaking and Fragment#mFragmentManager is
│    not null)
│    playerService instance of org.schabi.newpipe.player.MainPlayer
│    activity instance of org.schabi.newpipe.MainActivity with mDestroyed =
│    false
│    ↓ VideoDetailFragment.mFragmentManager
├─ androidx.fragment.app.FragmentManagerImpl instance
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ FragmentManagerImpl.mBackStack
├─ java.util.ArrayList instance
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ ArrayList.elementData
├─ java.lang.Object[] array
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ Object[].[0]
├─ androidx.fragment.app.BackStackRecord instance
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ BackStackRecord.mOps
├─ java.util.ArrayList instance
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ ArrayList.elementData
├─ java.lang.Object[] array
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ Object[].[0]
├─ androidx.fragment.app.FragmentTransaction$Op instance
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ FragmentTransaction$Op.mFragment
├─ org.schabi.newpipe.fragments.MainFragment instance
│    Leaking: NO (Fragment#mFragmentManager is not null)
│    activity instance of org.schabi.newpipe.MainActivity with mDestroyed =
│    false
│    ↓ MainFragment.binding
│                   ~~~~~~~
├─ org.schabi.newpipe.databinding.FragmentMainBinding instance
│    Leaking: UNKNOWN
│    Retaining 30904 bytes in 600 objects
│    ↓ FragmentMainBinding.rootView
│                          ~~~~~~~~
╰→ android.widget.RelativeLayout instance
     Leaking: YES (ObjectWatcher was watching this because org.schabi.newpipe.
     fragments.MainFragment received Fragment#onDestroyView() callback
     (references to its views should be cleared to prevent leaks))
     Retaining 1537 bytes in 36 objects
     key = 3ca5677d-b29b-4eb9-9230-dd17bda8f58e
     watchDurationMillis = 36738
     retainedDurationMillis = 26725
     View not part of a window view hierarchy
     View.mAttachInfo is null (view detached)
     View.mWindowAttachCount = 1
     mContext instance of org.schabi.newpipe.MainActivity with mDestroyed =
     false

METADATA

Build.VERSION.SDK_INT: 24
Build.MANUFACTURER: HUAWEI
LeakCanary version: 2.5
App process name: org.schabi.newpipe.debug.Useviewbindinginfragments
Stats: LruCache[maxSize=3000,hits=5153,misses=68531,hitRate=6%]
RandomAccess[bytes=3893630,reads=68531,travel=72315166573,range=25866324,size=46
366704]
Analysis duration: 11173 ms

Maybe it's because I moved the null set operation to onDestroyView?

Stypox · 2020-12-23T12:03:26Z

@Isira-Seneviratne the apk I used is from the PR description, so from 4 days ago by looking at the edits, therefore it is before I did the review and you pushed the change so that's not the cause. Could you provide an updated apk?

FunkyMuse · 2020-12-23T12:27:32Z

I get this binding leak with your latest apk, I don't know if it is a real leak. The heap dump was captured while I was in the VideoDetailFragment, if that counts anyhow:

┬───
│ GC Root: System class
│
├─ org.schabi.newpipe.player.helper.PlayerHolder class
│    Leaking: NO (VideoDetailFragment↓ is not leaking and a class is never
│    leaking)
│    ↓ static PlayerHolder.listener
├─ org.schabi.newpipe.fragments.detail.VideoDetailFragment instance
│    Leaking: NO (MainFragment↓ is not leaking and Fragment#mFragmentManager is
│    not null)
│    playerService instance of org.schabi.newpipe.player.MainPlayer
│    activity instance of org.schabi.newpipe.MainActivity with mDestroyed =
│    false
│    ↓ VideoDetailFragment.mFragmentManager
├─ androidx.fragment.app.FragmentManagerImpl instance
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ FragmentManagerImpl.mBackStack
├─ java.util.ArrayList instance
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ ArrayList.elementData
├─ java.lang.Object[] array
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ Object[].[0]
├─ androidx.fragment.app.BackStackRecord instance
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ BackStackRecord.mOps
├─ java.util.ArrayList instance
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ ArrayList.elementData
├─ java.lang.Object[] array
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ Object[].[0]
├─ androidx.fragment.app.FragmentTransaction$Op instance
│    Leaking: NO (MainFragment↓ is not leaking)
│    ↓ FragmentTransaction$Op.mFragment
├─ org.schabi.newpipe.fragments.MainFragment instance
│    Leaking: NO (Fragment#mFragmentManager is not null)
│    activity instance of org.schabi.newpipe.MainActivity with mDestroyed =
│    false
│    ↓ MainFragment.binding
│                   ~~~~~~~
├─ org.schabi.newpipe.databinding.FragmentMainBinding instance
│    Leaking: UNKNOWN
│    Retaining 30904 bytes in 600 objects
│    ↓ FragmentMainBinding.rootView
│                          ~~~~~~~~
╰→ android.widget.RelativeLayout instance
     Leaking: YES (ObjectWatcher was watching this because org.schabi.newpipe.
     fragments.MainFragment received Fragment#onDestroyView() callback
     (references to its views should be cleared to prevent leaks))
     Retaining 1537 bytes in 36 objects
     key = 3ca5677d-b29b-4eb9-9230-dd17bda8f58e
     watchDurationMillis = 36738
     retainedDurationMillis = 26725
     View not part of a window view hierarchy
     View.mAttachInfo is null (view detached)
     View.mWindowAttachCount = 1
     mContext instance of org.schabi.newpipe.MainActivity with mDestroyed =
     false

METADATA

Build.VERSION.SDK_INT: 24
Build.MANUFACTURER: HUAWEI
LeakCanary version: 2.5
App process name: org.schabi.newpipe.debug.Useviewbindinginfragments
Stats: LruCache[maxSize=3000,hits=5153,misses=68531,hitRate=6%]
RandomAccess[bytes=3893630,reads=68531,travel=72315166573,range=25866324,size=46
366704]
Analysis duration: 11173 ms

Maybe it's because I moved the null set operation to onDestroyView?

Sometimes LeakCanary has false positives, you can see that it cleary suggests that you should release all your resources in onDestryoView and if you do so then it's confused, but if you aren't doing it, then something's wrong.

Isira-Seneviratne · 2020-12-23T13:17:12Z

@Isira-Seneviratne the apk I used is from the PR description, so from 4 days ago by looking at the edits, therefore it is before I did the review and you pushed the change so that's not the cause. Could you provide an updated apk?

Sure.

…Header() and getListFooter() methods.

…r() and getListFooter() methods.

Stypox

I tested again with the apk from the CI and I couldn't reproduce the leak anymore. I also tested more but everything worked well. Thank you :-D

…e_view_binding_in_fragments Use view binding in fragments.

Isira-Seneviratne force-pushed the Use_view_binding_in_fragments branch 2 times, most recently from f46a5f6 to 8650962 Compare November 19, 2020 02:08

Isira-Seneviratne force-pushed the Use_view_binding_in_fragments branch 5 times, most recently from 21b90fc to 6c21e33 Compare November 23, 2020 01:17

Isira-Seneviratne force-pushed the Use_view_binding_in_fragments branch from 6c21e33 to 88e4595 Compare November 28, 2020 12:47

Isira-Seneviratne mentioned this pull request Nov 29, 2020

Switch to Groupie's view binding module. #5029

Merged

5 tasks

Isira-Seneviratne force-pushed the Use_view_binding_in_fragments branch 5 times, most recently from b31ca51 to 08bb597 Compare December 19, 2020 07:54

Isira-Seneviratne force-pushed the Use_view_binding_in_fragments branch from 08bb597 to 2d6831f Compare December 19, 2020 23:07

Stypox requested changes Dec 20, 2020

View reviewed changes

This was referenced Dec 21, 2020

Release 0.20.7 #5246

Merged

Release 0.20.7 #5245

Closed

Isira-Seneviratne force-pushed the Use_view_binding_in_fragments branch from 2d6831f to c8baef0 Compare December 22, 2020 12:47

Isira-Seneviratne force-pushed the Use_view_binding_in_fragments branch from c8baef0 to 8735de7 Compare December 24, 2020 00:06

Isira-Seneviratne force-pushed the Use_view_binding_in_fragments branch from 8735de7 to 0e2e4db Compare January 2, 2021 03:53

Isira-Seneviratne added 11 commits January 14, 2021 11:15

Return ViewBinding instead of View in BaseLocalListFragment's getList…

98be89a

…Header() and getListFooter() methods.

Return ViewBinding instead of View in BaseListFragment's getListHeade…

979102a

…r() and getListFooter() methods.

Use view binding in MainFragment.

910d22d

Use view binding in LicenseFragment.

7c581ec

Use view binding in DownloadDialog.

7682ebd

Use view binding in ChannelFragment.

6039484

Use view binding in SearchFragment.

97672f0

Use view binding in FeedFragment.

f80e1bd

Use view binding in SubscriptionFragment.

83f33a7

Use view binding in FeedGroupDialog.

5994cd8

Use view binding in FeedGroupReorderDialog.

f04b5fd

Isira-Seneviratne force-pushed the Use_view_binding_in_fragments branch from 0e2e4db to f04b5fd Compare January 14, 2021 05:46

Stypox approved these changes Jan 14, 2021

View reviewed changes

Stypox merged commit 94b086d into TeamNewPipe:dev Jan 14, 2021

TobiGr mentioned this pull request Jan 14, 2021

[dev] App closes when trying to backpress from inner fragment #5414

Closed

4 tasks

Isira-Seneviratne deleted the Use_view_binding_in_fragments branch January 15, 2021 00:36

This was referenced Jan 18, 2021

Release 0.20.10 #5455

Closed

Release 0.20.10 #5456

Merged

Stypox mentioned this pull request Jan 21, 2021

Fix channel subscribe button causing crash on closing #5464

Merged

5 tasks

ghost mentioned this pull request Apr 11, 2021

request on when a future update of the legacy version of newpiepe will arrive TeamNewPipe/NewPipe-legacy#72

Open

tossj pushed a commit to tossj/NewPipe-legacy that referenced this pull request Apr 20, 2021

Merge pull request TeamNewPipe/NewPipe#4814 from Isira-Seneviratne/Us…

d1aa44a

…e_view_binding_in_fragments Use view binding in fragments.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use view binding in fragments. #4814

Use view binding in fragments. #4814

Isira-Seneviratne commented Nov 6, 2020 •

edited

FunkyMuse commented Nov 29, 2020

Isira-Seneviratne commented Nov 29, 2020

Stypox commented Dec 12, 2020 •

edited

TacoTheDank commented Dec 19, 2020

Stypox left a comment

Stypox Dec 20, 2020

Isira-Seneviratne Dec 22, 2020

FunkyMuse Dec 22, 2020

Isira-Seneviratne Dec 22, 2020

Stypox Dec 20, 2020

Isira-Seneviratne Dec 22, 2020 •

edited

Stypox Dec 22, 2020

Stypox Dec 20, 2020

Isira-Seneviratne Dec 22, 2020

Stypox Dec 20, 2020

Stypox commented Dec 21, 2020

TobiGr commented Dec 21, 2020

Stypox commented Dec 22, 2020

Isira-Seneviratne commented Dec 22, 2020

Stypox commented Dec 23, 2020

FunkyMuse commented Dec 23, 2020

Isira-Seneviratne commented Dec 23, 2020

Stypox left a comment

		private var _feedBinding: FragmentFeedBinding? = null
		private val feedBinding get() = _feedBinding!!

		private var _binding: FragmentSubscriptionBinding? = null
		private val binding get() = _binding!!

Use view binding in fragments. #4814

Use view binding in fragments. #4814

Conversation

Isira-Seneviratne commented Nov 6, 2020 • edited

What is it?

Description of the changes in your PR

APK testing

Due diligence

FunkyMuse commented Nov 29, 2020

Isira-Seneviratne commented Nov 29, 2020

Stypox commented Dec 12, 2020 • edited

TacoTheDank commented Dec 19, 2020

Stypox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Isira-Seneviratne Dec 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stypox commented Dec 21, 2020

TobiGr commented Dec 21, 2020

Stypox commented Dec 22, 2020

Isira-Seneviratne commented Dec 22, 2020

Stypox commented Dec 23, 2020

FunkyMuse commented Dec 23, 2020

Isira-Seneviratne commented Dec 23, 2020

Stypox left a comment

Choose a reason for hiding this comment

Isira-Seneviratne commented Nov 6, 2020 •

edited

Stypox commented Dec 12, 2020 •

edited

Isira-Seneviratne Dec 22, 2020 •

edited