-
Notifications
You must be signed in to change notification settings - Fork 1.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
Use AndroidX #665
Open
cristan
wants to merge
20
commits into
facebook:main
Choose a base branch
from
cristan:master
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Use AndroidX #665
Changes from 8 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
9e20ab0
Upgrade gradle a little
cristan 0bcf638
Update commented out code
cristan 6771eba
Update minSdkVersion to 14
cristan 35ef0e4
Use AndroidX dependencies
cristan 806d12b
Update dependencies a little
cristan b5f43c6
Use AndroidX imports
cristan 241a0df
Add dependency to androidx.fragment:fragment
cristan 2adc71c
Add missing AndroidX import
cristan 708a283
Remove unused AppCompat dependency
7c16c44
Bump AndroidX Fragment version to 1.1.0
96369b0
Take advantage of minSdkVersion 14
c1d12e7
Take advantage of better nullability information
f58146f
Annotate nullability of newly annotated parameters/functions
780486d
Use correct class names for reflection
80dd153
Explain why the Fragment dependency is optional
0d4e4f8
Don't use reflection to test the existence of optional dependencies
9d3d929
Depend on Fragment 1.0.0
1a7aaa5
Remove more obsolete code now we only support 14 and up
bb3480f
Bump min/targetSdkVersion to 29 and add missing non-null annotations
3065dde
Remove redundant casts at findViewById
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
POM_NAME=Stetho | ||
POM_ARTIFACT_ID=stetho | ||
POM_OPTIONAL_DEPS=com.android.support:appcompat-v7 | ||
POM_OPTIONAL_DEPS=androidx.appcompat:appcompat | ||
POM_PACKAGING=aar |
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
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
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
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
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
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
Oops, something went wrong.
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.
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.
If it's optional it should be
compileOnly
.AppCompat is actually used nowhere in the code. Only Core and Fragment.
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.
compileOnly is probably a no go? https://issuetracker.google.com/issues/109894265
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.
That should only be an issue if you're trying to work with Android resources from the library, which stetho doesn't, at least as far as I can see. As long as the error doesn't pop up during compilation we're golden.
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.
the resources still contribute to the
R.class
file generation which "could" lead to issues for consumers maybe at build time maybe at runtime maybe in a lint run?I agree it is probably not anything to worry about, but I am more making the argument that if we know it isn't correct it would be best to just avoid it and star the aosp issue to hopefully move it up in the priority list.
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.
Oh. That. Yeah, that could be an issue for some unfortunate souls.
Defensively, call it a breaking change and raise major version. If in exchange I get rid of hard dependency of AppCompat, I'd consider it an acceptable price.
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.
Can be, we need AndroidX Core, which until now was pulled in automatically as a dependency of Fragment. This should work:
The "everywhere" part wasn't helpful at all. Knowing which imports didn't resolve (and maybe seeing that all were from androidx.core package) would be.
Stop assuming failure, this is going to work ;)
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.
Do we actually need AndroidX core? The only places with
import androidx.core
areAccessibilityUtil
andAccessibilityNodeInfoWrapper
.AccessibilityUtil
is only used inAccessibilityNodeInfoWrapper
is only used fromViewDescriptor
where we check whether we can accessandroidx.core.view.accessibility.AccessibilityNodeInfoCompat.class.getClass()
. So, as far as I can see, core is just as optional as the Fragment dependency. Is there anywhere else we use core that I missed?Good point: it actually isn't everywhere. It's everywhere where we use any transient dependencies like core.
Adding all the transient dependencies works:
-edit: core was lost because of a formatting issue, it's now back again.
Not using
@aar
also gets rid of the errors:compileOnly 'androidx.fragment:fragment:1.1.0
What shall we use?
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.
I don't think we need all the dependencies of Fragment. We only need those that we need to compile, whose API we use in our code. Fragment checks out. Core, as you mentioned, too.
If the accessibility stuff is optional,
compileOnly
is fine (with its ownReflectionUtil
check). Otherwiseimplementation
is the way to go. So this is it?Mind the
1.0.0
as we don't want to accidentally access something that's not in v1. That way the consumer is free to choose anything from [1.0.0, 2.0.0).Plus implementation annotations, of course.
Awesome!
I'd add proguard rules to ignore missing Fragment and Core.
Otherwise consumers using Proguard would get fails, if they don't use Fragment or Core, about missing classes.
It would be better to name concrete classes or entry points that are referenced in code, but this should be fine.
Or the other way around, specify the accessing classes in this project, that should work. So many possibilities!
A potential issue is that the whole module is free to reference classes from Core and Fragment but it's no longer guaranteed to be in runtime classpath. These "optional" accessors should be in separate modules, which won't leak their
compileOnly
dependency to the main Stetho library.That touches publishing, which may be a problem.
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.
Just using Core and Fragment as
@aar
dependencies doesn't work:You'll get the following errors:
If you want AndroidX Fragment 1.0.0 as an AAR, you'll need to define the following to solve this:
Anyway: making the optional dependencies actually optional hasn't got anything to do with AndroidX: your master branch has the same issues. I've therefore just updated the comment.
I suggest to merge this PR and create a separate issue to make the dependency actually optional (or go the non-optional route and remove the realtime checks).
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.
I've just removed the last code which still didn't assume a
minSdkLevel
of 14. I've removed the documentation aboutActivityTracker.add
andremove
: the AutomaticTracker now works for everyone, so I don't think we should even mention that you can fiddle with the added / removed activities. You could even argue for makingadd
andremove
private methods.