-
Notifications
You must be signed in to change notification settings - Fork 160
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
Fix compatibility on Android platforms below Android API 28 #543
Fix compatibility on Android platforms below Android API 28 #543
Conversation
We need to discuss the minimum Android SDK version we should support before deciding which solution to use. |
@quietbits , @mollykarcher , do you have any insights or preferences on min version of android SDK? |
The minSDK of the current Lobstr client is 23. |
Android API Levels Cumulative usage: https://apilevels.com/ I tend to set the minimum supported version to 24, according to the information on the website above, which is enough to cover over 96% of devices. Additionally, many features of JDK 8 also require API 24+, on API 23, you can't even use update: By introducing Java 8+ API desugaring support, we can use |
Ok, thanks for recommendation on API 24, sounds good from a practical standpoint and 'in-the-field' insight. |
@overcat , it's great to see how your are adapting the sdk to work further in parallel on the android platform or native jdk. After review, one aspect stands out, this is making low level changes in the sdk which impact all usages but it is only relevant/needed for the android use case. wanted to suggest a different design option to consider for allowing the sdk to scale out separately for native java or android specifics with a standard jdk mechanism, SPI(service provider interface) pattern to enable downstream apps to plug in different runtime implementations such as for Base64. Here's a sketch of SPI classes to add in java sdk for Base64 provider:
So, in xdrgen, the code gen for static usage of And in the android project, can inject it's own SPI provider for different Base64 implementation:
create file in android project
|
@tamirms , @mollykarcher , @lijamie98 , @Ifropc would also appreciate any feedback related to this dual compatibility of native and android within single sdk - #543 (comment) |
I agree that using provider would be a preferred approach. In fact, we had something similar in the wallet SDK in the past |
|
Developers would need to manually change base64 implementation for old android devices and by default java native implementation is used. |
Using SPI can certainly meet the requirements. However, from the perspective of Android developers using this SDK, I think they may prefer the current implementation(?) Because they don't need to write their own provider. |
I added an SPI implementation in the new PR(overcat-deprecation#26). If everyone prefers it, I will merge it into this PR. |
Sounds convenient and more appropriate than some additional configurations. At least for now. |
For android SPI usage, we could take a small extra leap, making it more seamless for devs by creating a new
Android app imports |
Hi @sreuland, can you review this series of PRs? I have created a new repo, and once it is approved, I will transfer it to the gh stellar account. |
@@ -0,0 +1,24 @@ | |||
package org.stellar.javastellarsdkdemoapp |
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.
once https://github.com/overcat/java-stellar-sdk-android-spi is published to jitpack, this can go away correct, and change this example to have dep on that instead?
[edit] oh, actually, i see it's already published to jitpack as a snapshot!
https://jitpack.io/#overcat/java-stellar-sdk-android-spi/-SNAPSHOT
@overcat , https://github.com/overcat/java-stellar-sdk-android-spi/pull/1 looks good, I can't add myself as reviewer, but left approval comment. We'll get the setup going for that new repo on stellar in parallel, what's nice is that it's already published a snapshot to jitpack! https://jitpack.io/#overcat/java-stellar-sdk-android-spi/-SNAPSHOT so, can use that reference for dependencies in android test projects in the interim if that sounds ok? |
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.
great work, thanks for consideration of the spi suggestion, looks great!
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.
this can/should be deleted now correct? since the andoid spi impl deals with these specifics now.
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.
This includes the implementation of Base32, so it cannot be removed, but we can consider adding commons-codec
back and then moving this copy to java-stellar-sdk-android-spi
. Old versions of the Android platform include outdated commons-codec
, it does not contain the Base32 module, see https://stackoverflow.com/questions/9126567/method-not-found-using-digestutils-in-android/29833101#29833101
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 sounds like a worthwhile refactor, avoids the copy existing in the native jvm path, and that code copy then is really limited to just android apps at sdk level <= 25, which need to use the sdk provider impl override correct?
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.
Yes, but it will affect users with API level <= 27. They need to introduce Android SPI.
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.
ok, then should the docs on the sdk provider be consistent on that version, it looks version 26 is called out there as the compatibility line:
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 intend to do this because it will result in circular dependencies. The java-stellar-sdk-android-spi depends on java-stellar-sdk, and the java-stellar-sdk project should not depend on java-stellar-sdk-android-spi. Additionally, java-stellar-sdk-android-spi has independent tests in its repo. |
ok, yeah, that's fine, it was one other additional 'leap' we could have done as part of the SPI pattern by putting the |
# Conflicts: # CHANGELOG.md # src/main/java/org/stellar/sdk/StrKey.java
Fix #542
Before merging this PR, we need to merge stellar/xdrgen#170 first.
After merging this PR, if Java 8+ API desugaring support is not introduced, this SDK can run on platforms >= Android API 24, which is our minimum supported version.
If you need to run on lower versions of the platform, you need to introduce Java 8+ API desugaring support in your project. According to my testing, after introducing it, it can run on platforms >= Android API 21. I have not tested on lower versions of the platform; perhaps it can run.
Change: