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.

Sign up for GitHub

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

Jump to bottom

Prep for 1.0 release #156

Merged
merged 7 commits into from
Aug 30, 2017
Merged

Prep for 1.0 release #156

merged 7 commits into from
Aug 30, 2017

Conversation

SUPERCILEX
Copy link
Contributor

fixes #154

@SUPERCILEX
Copy link
Contributor Author

I really hate tests sometimes 😆

@samtstern
Anyway, this PR is now a little bigger than I originally intended:

  • Removes all deprecated APIs (which is just the AppSettingsDialog constructors and negative button listener method)
    • Also refactored how we handle clicks and contexts
  • Fixed tests failing because of AAPT2
    • I needed to do this because AAPT1 was failing to compile a string for some reason.

@SUPERCILEX
Copy link
Contributor Author

Also:

  • Removed BasicActivity: it says it's for tests, but I don't see it used in programmatic tests and it appears to be a subset of MainActivity anyway
  • Use 3.0 configuration

SUPERCILEX
SUPERCILEX commented Aug 30, 2017
View reviewed changes
easypermissions/build.gradle
compile "com.android.support:appcompat-v7:$support_library_version"
implementation "com.android.support:appcompat-v7:$support_library_version"
api "com.android.support:support-compat:$support_library_version"
api "com.android.support:support-fragment:$support_library_version"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I removed appcompat-v7 from the demo project, I couldn't get it to compile without these api declarations.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is working as intended. Some of the AppCompat classes are part of our public API, so we need an api dependency on them.

@SUPERCILEX
Copy link
Contributor Author

@samtstern Ok, this PR has grown too big so I've rebased all my commits such that they can be merged individually (instead of the usual squashing). I could also try to create a PR per commit, what do you think?

Since my last comment, I've fixed a bug where the AppSettingsDialog would use system default colors instead of picking the dev's app colors.

samtstern
samtstern reviewed Aug 30, 2017
View reviewed changes
Copy link
Contributor

@samtstern samtstern left a comment

Choose a reason for hiding this comment

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

This seems mostly good! Some comments left.

easypermissions/build.gradle
compile "com.android.support:appcompat-v7:$support_library_version"
implementation "com.android.support:appcompat-v7:$support_library_version"
api "com.android.support:support-compat:$support_library_version"
api "com.android.support:support-fragment:$support_library_version"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is working as intended. Some of the AppCompat classes are part of our public API, so we need an api dependency on them.

easypermissions/src/main/java/pub/devrel/easypermissions/AppSettingsDialog.java Outdated
}

void setNegativeListener(DialogInterface.OnClickListener negativeListener) {
mNegativeListener = negativeListener;
private static Context parseContext(Object activityOrFragment) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make this method something we call just once (at build() time for example?) and then save the Context in a variable rather than re-casting activityOrFragment every time we want to use it?

easypermissions/src/main/java/pub/devrel/easypermissions/AppSettingsDialog.java Outdated

void setContext(Context context) {
mContext = context;
static AppSettingsDialog parse(Intent intent, Activity activity) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would call this method either getInstance() or fromIntent()

@samtstern
Copy link
Contributor

@SUPERCILEX this PR is big but not problematically so, I think we can try to keep it together. Thank you for sending it!

@SUPERCILEX
Copy link
Contributor Author

@samtstern SGTM! I rebased everything because I thought you could compare two commits coming from the same HEAD, but apparently not so the diff is broken, sorry! 😊 I added back the mContext fields in the new 6aa514e and renamed that method to fromIntent.

While working on AppSettingsDialog, I noticed that we had a bunch of unnecessary RequiresApi annotations back from when our min sdk was 9 so I fixed that in e1c9301. And while working on that 😄, I noticed that we were using the framework Fragment even if the calling activity was a descendant of AppCompatActivity so I fixed that in a497251. Whew! I think I'm done now. 😆

@samtstern
Copy link
Contributor

@SUPERCILEX thanks for all the hard work here! This LGTM, I am going to merge it.

@samtstern samtstern changed the base branch from master to version-1.0 August 30, 2017 21:20
@samtstern samtstern merged commit a54c44c into googlesamples:version-1.0 Aug 30, 2017
@SUPERCILEX SUPERCILEX deleted the 1.0 branch August 30, 2017 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samtstern samtstern samtstern left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SUPERCILEX @samtstern