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

Bumps compile API to 29 (Q) and removes deprecated calls and unnecessary casts #7864

Merged
merged 1 commit into from
Nov 22, 2019

Conversation

rafaeltoledo
Copy link
Contributor

This will help the Android client to evolve with the latest libraries (as the legacy support libs will not be shipped anymore with the com.android.support package).

This PR also makes the app compliant with the new API requirements to start later this year:
https://android-developers.googleblog.com/2019/02/expanding-target-api-level-requirements.html

@rafaeltoledo
Copy link
Contributor Author

Seems that the CI needs the target 28 to succeed the build. It's possible to auto-install it if all the licenses are accepted.

sdkmanager --licences

I'll fix the lint errors today later

@Simonx22
Copy link
Member

Hi, we bumped the target API to 28 in a different PR recently. However, AndroidX migration is still needed. Do you want to rebase this PR? I can also do AndroidX migration in a new PR if you want.

@rafaeltoledo
Copy link
Contributor Author

@Simonx22 sure, I'll rebase it and update this PR :)

@JosJuice
Copy link
Member

We currently have both this PR and PR #8471 fixing the same problem in the same way, which is kind of an odd situation... I guess I'll merge the other one?

Since there are some small changes that this PR makes but not the other PR (like updating the file picker version and removing some typecasts), I'll leave this open in case you want to rebase the PR to only include those changes. Otherwise you can close the PR.

@rafaeltoledo
Copy link
Contributor Author

@JosJuice no problem, I'll rebase it and check if this still makes sense

@@ -1,7 +1,7 @@
apply plugin: 'com.android.application'

android {
compileSdkVersion 28
compileSdkVersion 29
Copy link
Member

Choose a reason for hiding this comment

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

Did you test this change on Android 10? I previously also changed the SdkVersion to 29 but forgot that scoped storage is a thing, which might cause issues so I just went with 28 for now.

Copy link
Member

Choose a reason for hiding this comment

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

targetSdkVersion 29 is what gives us scoped storage, right? I'm not aware of compileSdkVersion 29 doing the same.

Copy link
Member

Choose a reason for hiding this comment

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

Ohh I think you're right. Should be good then!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe makes sense to explicitly set targetSdkVersion 28 until we stress the scoped storage scenario better in Android Q devices?

@@ -89,20 +89,6 @@ public void onAttach(Context context)
mPresenter.onAttach();
}

/**
* This version of onAttach is needed for versions below Marshmallow.
Copy link
Member

Choose a reason for hiding this comment

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

We still have minSdkVersion set to 21. Will this fragment continue to work on old versions of Android despite what this comment says?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is now deprecated and replaced by void onAttach(@NonNull Context context)

Copy link
Member

Choose a reason for hiding this comment

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

And that means that it will work on older versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@Simonx22
Copy link
Member

I think the commit message should be updated because it's still saying that it's bumping to v28 and moving dependencies to AndroidX.

@rafaeltoledo rafaeltoledo changed the title Bumps target to API 28 and moves dependencies to AndroidX Bumps target to API 29 and removes deprecated calls and unnecessary casts Nov 21, 2019
@rafaeltoledo rafaeltoledo changed the title Bumps target to API 29 and removes deprecated calls and unnecessary casts Bumps compile API to 29 (Q) and removes deprecated calls and unnecessary casts Nov 21, 2019
@Simonx22
Copy link
Member

The commit message is still saying "Bumps target API to 29 and moves dependencies to AndroidX".
I think it should maybe be changed to something like the title because we don't target v29 yet (since only the compileSDK version was updated), and this PR is removing deprecated calls and unnecessary cats now instead of moving dependencies to AndroidX.
Other than that: LGTM

@JosJuice JosJuice merged commit abc1453 into dolphin-emu:master Nov 22, 2019
@JosJuice
Copy link
Member

I'm getting some odd behavior with this merged. The tab bar where you switch between GC/Wii/other now has a while background instead of a color background, and the actions on dialogs (e.g. yes/no) now have a color background instead of white. I'm assuming this has to do with the changes to styles.xml, but I'm not sure exactly what happened or what the proper fix is. Do either of you two know?

@mbc07
Copy link
Member

mbc07 commented Nov 22, 2019

Since Pie, Android is pushing white backgrounds with just small color accents everywhere, supposedly to make it easier to implement dark mode. That might be the case here...

@JosJuice
Copy link
Member

I'm on 8.1.0, though... But maybe it affects me anyway because that behavior is part of AndroidX?

@mbc07
Copy link
Member

mbc07 commented Nov 22, 2019

Very likely...

@JosJuice
Copy link
Member

Addressed in PR #8481.

@rafaeltoledo
Copy link
Contributor Author

@JosJuice I can open another PR with the fixes if sounds good to you.
For the tabs, basically, we have individual attributes for TabLayout now
For the dialogs imports should be from AndroidX instead of system default.

@JosJuice
Copy link
Member

If it's a different fix than the simple revert in PR #8481, then yes, please open a PR.

@rafaeltoledo
Copy link
Contributor Author

@JosJuice Submitted #8482

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants