Skip to content
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

Add support library fragment watcher #1611

Merged
merged 2 commits into from Nov 26, 2019

Conversation

AndreasBoehm
Copy link
Contributor

@AndreasBoehm AndreasBoehm commented Nov 5, 2019

Because there are some projects that can't migrate to AndroidX yet and my main project is one of those I figured that it might make sense to share this code with the community to allow everyone to use leakcanary 2 with the good old support library.

To watch leaking support library fragments and their views the following needs to be added to the build.gradle.

  debugImplementation 'com.squareup.leakcanary:leakcanary-android:2.0-beta-4'
  debugImplementation 'com.squareup.leakcanary:leakcanary-object-watcher-android-support-fragments:2.0-beta-4'

To test this PR you need to add the support-fragments project to the build.gradle.

 debugImplementation project(':leakcanary-object-watcher-android-support-fragments')

@CLAassistant
Copy link

CLAassistant commented Nov 5, 2019

CLA assistant check
All committers have signed the CLA.

@Armaxis
Copy link
Member

Armaxis commented Nov 7, 2019

This looks useful to me; @pyricau what do you think?

Quick (somewhat related) question: @AndreasBoehm do you see AndroidX dependencies being pulled into project when using LeakCanary? I'm wondering if developers who still use Support Lib end up pulling the AndroidX classes via transitive dependency through LeakCanary.

@@ -0,0 +1,58 @@
/*
* Copyright (C) 2018 Square, Inc.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2019 ;) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy & paste 🤦‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Comment on lines 64 to 70
if (classAvailable(ANDROID_SUPPORT_FRAGMENT_CLASS_NAME) &&
classAvailable(ANDROID_SUPPORT_FRAGMENT_DESTROY_WATCHER_CLASS_NAME)
) {
val watcherConstructor = Class.forName(ANDROID_SUPPORT_FRAGMENT_DESTROY_WATCHER_CLASS_NAME)
.getDeclaredConstructor(ObjectWatcher::class.java, Function0::class.java)
@kotlin.Suppress("UNCHECKED_CAST")
fragmentDestroyWatchers.add(
watcherConstructor.newInstance(objectWatcher, configProvider) as (Activity) -> Unit
)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block can probably be extracted to a method, so we can reuse it in previous if statement for ANDROIDX_FRAGMENT_CLASS_NAME - this will remove the code duplication

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually wanted to do this but forget about it after everything worked as expected. Sorry!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed within my latest commit

build.gradle Outdated
@@ -34,6 +34,7 @@ buildscript {
mockito_kotlin: 'com.nhaarman:mockito-kotlin-kt1.1:1.5.0',
okio: 'com.squareup.okio:okio:2.2.2',
robolectric : 'org.robolectric:robolectric:4.0-alpha-3',
android_support: 'com.android.support:support-v4:28.0.0'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this line to be after androidx - it's (more-or-less) alphabetically sorted now 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved :)

@pyricau
Copy link
Member

pyricau commented Nov 8, 2019

Can you also add this module here: https://github.com/square/leakcanary/blob/master/leakcanary-android-core/build.gradle#L7

Essentially this will remove the need to add debugImplementation 'com.squareup.leakcanary:leakcanary-object-watcher-android-support-fragments:2.0-beta-4'. The code for both Android X and support lib will be included but not loaded, and then at runtime we load it if and only if the right classes are available.

Side note: we should make sure this doesn't cause issues with jetifier

Copy link
Member

@pyricau pyricau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving (a few changes still needed but overall looking good)

@AndreasBoehm
Copy link
Contributor Author

Thanks for accepting my PR! I would love to adjust it right now but i am on holidays and i don’t have my computer with me.

I can adjust it in roughly two weeks if this is fine for you?
Fine for me if you want to adjust it on your own.

I wasn’t sure about adding the support library watcher to the core package as i didn’t want to add code for a legacy library to the project. But i agree that it would make things easier for users of the library.

@AndreasBoehm
Copy link
Contributor Author

This looks useful to me; @pyricau what do you think?

Quick (somewhat related) question: @AndreasBoehm do you see AndroidX dependencies being pulled into project when using LeakCanary? I'm wondering if developers who still use Support Lib end up pulling the AndroidX classes via transitive dependency through LeakCanary.

Thats a good question!
I did not check if this is pulling in the AndroidX deps.

@pyricau
Copy link
Member

pyricau commented Nov 9, 2019

I'm ok with waiting, would love to give you the opportunity to finish this.

Once this is ready we should probably use it on a jetified project and make sure Gradle won't complain.

@AndreasBoehm
Copy link
Contributor Author

Thank you!

Will work on it after my holidays.

Talk to you soon.

@AndreasBoehm AndreasBoehm force-pushed the aboehm/android-support-fragments branch from 9a0961d to c6b3a47 Compare November 26, 2019 07:58
@AndreasBoehm
Copy link
Contributor Author

Hi @pyricau,

I addressed your comments but I was not able to test the library on a jetified project.
For my previous tries, I added the library as a source dependency to my project but to test with jetifier I would need to add it via maven.
Could you help me to install the library to my local maven repository?

@Armaxis I will check later today if leakcanary pulled in AndroidX dependencies into my project.

@pyricau
Copy link
Member

pyricau commented Nov 26, 2019

Try this (changing the path to your local repo):

./gradlew uploadArchives -PSNAPSHOT_REPOSITORY_URL=file:///Users/py/.m2/repository

@pyricau
Copy link
Member

pyricau commented Nov 26, 2019

Updated the docs to document this: https://square.github.io/leakcanary/dev-env/#deploying-locally

@pyricau
Copy link
Member

pyricau commented Nov 26, 2019

I checked out the branch, deployed locally, added the support library only to a project then support library + jetifier and it's all been working, no issue.

I'm going to merge :)

@pyricau pyricau merged commit 7c339c0 into square:master Nov 26, 2019
@AndreasBoehm AndreasBoehm deleted the aboehm/android-support-fragments branch November 28, 2019 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants