Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Support Android 5 (COMMUNITY) #2955

Merged
merged 34 commits into from May 4, 2021

Conversation

fynngodau
Copy link
Contributor

@fynngodau fynngodau commented Apr 25, 2021

Implemets #1799.

Thanks to multiple compatibility PRs that were already merged (#2026, #2700, #2844), not a lot of changes were left.

Unfortunately, I had to disable the splash screen for Android 5 & 5.1, as the image is always stretched to fit the screen width and height without respecting its aspect ratio, looking very broken.

With this change, all visual bugs on Android 5 should be fixed, except that the ripple effect on the dispatcher cards is missing.

The PR also hides the Background Priority setting below Android 6, as such a system setting did not exist there.


Internal Tracking ID: EXPOSUREAPP-6360

Since these settings make no sense there, we are hiding them below Android
Marshmellow.
This makes the three-dot menu visible on Android 5
This is a very weird way of doing it, but since Android 5 doesn't
support the height attribute, it appears to be the best solution.
Via https://stackoverflow.com/a/19239478.
Since this backgroundTint was not applied, simply use the card_dark
drawable that already has the color we want.
Splash screens on Android 5 are very broken, as the images on the splash
screen are streched to fit the screen width. Therefore, the splash
screen can only be shown on API >= 23.
@fynngodau fynngodau requested a review from a team April 25, 2021 12:38
@dsarkar dsarkar added community Tag issues created by community members mirrored-to-jira This item is also tracked internally in JIRA labels Apr 25, 2021
@dsarkar
Copy link
Member

dsarkar commented Apr 25, 2021

@fynngodau Thanks. Internal Tracking ID: EXPOSUREAPP-6360

@harambasicluka
Copy link
Contributor

Hi @fynngodau this needs to be discussed with a lot of stakeholders. I'll keep you updated here 🚀
And as always thanks for all your PRs ❤️

@harambasicluka
Copy link
Contributor

Current idea is to merge this without the minSdkVersion. So we need some approval to change this but making our code work for older versions shouldn't be a problem so that it can be used in some forks... :)

@d4rken d4rken added this to the 2.2.0 milestone Apr 27, 2021
This partially reverts commit 2edced1.
@fynngodau
Copy link
Contributor Author

@harambasicluka I've pushed a change that undoes the change to minSdkVersion again. It is up to you which of the other changes you would like in your codebase – namely, 823356a is quite the hack.

@vaubaehn
Copy link
Contributor

vaubaehn commented Apr 29, 2021

@fynngodau
Maybe it could be helpful to add some comments to your changes that they're for Android 5 compatibility?
Not that after some months a new project member wonders about some "weird" code (e.g., 823356a) and reverts it as a clean up... Or is there no danger that something like this might happen?

@mtwalli mtwalli self-assigned this Apr 30, 2021
Copy link
Contributor

@mtwalli mtwalli left a comment

Choose a reason for hiding this comment

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

Thank you for the effort! , I would like to suggest the following changes ⬇️

@mtwalli
Copy link
Contributor

mtwalli commented Apr 30, 2021

In ErrorDialog file line 38 we need to change the textApperance too

       - setTextAppearance(R.style.body1)
       + TextViewCompat.setTextAppearance(this, R.style.body1)

Copy link
Member

@d4rken d4rken left a comment

Choose a reason for hiding this comment

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

Changes look good.

Would like a few more tests, and we need to guard against androidx.security issues.

val caps: NetworkCapabilities? = manager.getNetworkCapabilities(activeNetwork)
return caps?.hasCapability(NetworkCapabilities.NET_CAPABILITY_VALIDATED) ?: false
return when {
Build.VERSION.SDK_INT >= Build.VERSION_CODES.M -> {
Copy link
Member

Choose a reason for hiding this comment

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

Can we use BuildVersionWrap and unit test this?

Copy link
Contributor

@mtwalli mtwalli Apr 30, 2021

Choose a reason for hiding this comment

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

BuildVersionWrap does not remove the (warning/error) and we need to SuppessLint error, I prefer that way because it offers both

Copy link
Member

Choose a reason for hiding this comment

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

But can we test it without BuildVersionWrap?

Copy link
Contributor

@mtwalli mtwalli May 2, 2021

Choose a reason for hiding this comment

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

Yes, we can either:
1- Using the following Robolectric Util - which we already have -

 ReflectionHelpers.setStaticField(Build.VERSION::class.java, "SDK_INT", 23)

or
2- Creating our own helper to have less boilerplate code:

object SDKVersion {
    fun set(newValue: Int) {
        val field = Build.VERSION::class.java.getField("SDK_INT")
        field.isAccessible = true
        val modifiersField: Field = Field::class.java.getDeclaredField("modifiers")
        modifiersField.isAccessible = true
        modifiersField.setInt(field, field.modifiers and Modifier.FINAL.inv())
        field.set(null, newValue)
    }
}

and then in test cases set the required version SDKVersion.set(21)

I tried both ways and seem fine. If we agree about this approach, I can do it in another PR ,and totally remove the wrapper if no issue arises

Copy link
Member

@d4rken d4rken May 3, 2021

Choose a reason for hiding this comment

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

Isn't just using the wrapper object much easier? No slow Robolectric? No reflection? Works well with BuildConfigWrap already.

@@ -18,14 +19,13 @@ class PowerManagement @Inject constructor(
private val powerManager by lazy { context.getSystemService<PowerManager>()!! }

val isIgnoringBatteryOptimizations
get() = powerManager.isIgnoringBatteryOptimizations(context.packageName)
get() = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we use BuildVersionWrap and unit test this?

)
@SuppressLint("NewApi")
get() = when {
BuildVersionWrap.hasAPILevel(Build.VERSION_CODES.M) -> api23NetworkState()
Copy link
Member

Choose a reason for hiding this comment

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

Would like a unit test for this behavior.

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 might need some help with this. I wrote the following test:

@Test
fun `current state is correctly determined by method for API level 23`() = runBlockingTest2(ignoreActive = true) {
    val instance = createInstance(this)
    instance.networkState.first()

    verify { instance.api23NetworkState() }
}

and I am getting: java.lang.AssertionError: Verification failed: call 2 of 7: Any(child of context#3#10).getActiveNetwork()) was not called

Do you know why my test is not working?

Copy link
Member

Choose a reason for hiding this comment

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

Looks a bit like a nested verify call? i.e. verify on a mock of a mock or something like that.

The PR is in a good place currently, and we are going to merge it now.

@mtwalli or me will create a small follow up PR with additional unit tests. I'll ping you on the unit test when we get it working 😁

Corona-Warn-App/src/main/AndroidManifest.xml Show resolved Hide resolved
Copy link
Contributor

@mtwalli mtwalli left a comment

Choose a reason for hiding this comment

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

Thank you very much!, We will improve the test coverage and SDK version wrapper points in a follow up PR

@d4rken
Copy link
Member

d4rken commented May 4, 2021

Thanks a lot for all your effort on this @fynngodau. I'll leave the comments open and address them in a follow up PR on our end.

We'll keep the minAPI at 23 for now, prepare internal testing and discuss when we can lower the API requirement.

@d4rken d4rken merged commit 8296d1f into corona-warn-app:release/2.2.x May 4, 2021
@fynngodau fynngodau deleted the feature/lollipop branch May 4, 2021 09:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community Tag issues created by community members mirrored-to-jira This item is also tracked internally in JIRA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants