Code Review: refactor code

[tolot27]

Over the last couple of days I spent some time to understand the xDrip code. After some reviewing I found the following issues which I like to address soon:

• cleanup Gradle files (reorganization of gradle files and libraries #1538)
• remove local Android Archive (AAR) files and use the libraries from public repositories like maven, instead
  • update ColorPicker
  • remove hellocharts
  • search-preference.aar v1.0.4 requires casts to AppCompatActivity
  • remove usb-serial-for-android-v010 (implementation 'org.clojars.nakkaya:usb-serial-for-android-v010:010') because it is imported into ImportedLibraries\usbserial; it looks like it is a much older version 0.2.0-pre
• ImportedLibraries.dexcom is imported from https://github.com/nightscout/android-uploader and requires ImportedLibraries\usbserial. Remove unused code.
• upgrade all dependencies to the newest one which still supports the minSdkVersion we require
  • setup Dependabot at the main repository; did this on my fork and it works so far
• avoid calling new methods on older versions: minSdkVersion is set to 18/19 but methods from level 21 or 26 are called (a473048)
• migrate to AndroidX (mandatory as of Android SDK 28.0), previous versions support com.android.support:support-annotations which is currently in use (74a4f02)
• migrate rxjava (1.3.3) to rxjava2
• enable incremental compilation (enable incremental debug builds and apply changes #1598)
• enable parallel and incremental annotation (android.enableSeparateAnnotationProcessing=true)
• upgrade Android Gradle Plugin from 3.4.1 to at least 3.5.x (https://developer.android.com/jetpack/androidx/releases/databinding) (fix package names and add support for incremental compilation #1539)
  • requires package names written in all lowercase (see Naming a Package) because old data binding (android.databinding.enableV2=true) is removed) (fix package names and add support for incremental compilation #1539)
• upgrade Android Gradle Plugin to 3.6.x or higher requires migration to AndroidX or android.useAndroidX=true and android.enableJetifier=true in gradle.properties (74a4f02)
• migrate from Fabric Crashlytics (io.fabric) to Firebase Crashlytics it was replaced by New Relic
• TargetSdkVersion < 26 are no longer supported. As of the second half of 2018, Google Play requires that new apps and app updates target API level 26 or higher. Only required if we want to deploy the app using Google Play Store.
• refactor common code of phone and wear app into a library module to avoid code duplication (e.g. classes Constants and Dex_Constants) and improve maintenance (see Initiative: Better Support for Android Wear  #1733). Good example is provided at https://stackoverflow.com/a/62966117/10373188.
  • document library code
• address warnings/errors reported by Android Studio Code Inspection
• consistent code formatting and lint checks
  • GitHub bot which formats PRs automatically or warn at least
• provide a pull request template
• configure continuous integration (CI)
• Code Coverage (create code coverage reports #2102)
• review ProGuard usage and rules: https://developer.android.com/studio/build/shrink-code

Some new security-related tasks arose during the code review:

• remove Google Maps key from repository (securing Google Maps key #1596 adds fundamental support)
• protect google-services.json either by using Travis Encrypting Files or creating a new build type containing a dummy google-services.json file.

[tzachi-dar]

+1000 to enable incremental compilation

As for:
TargetSdkVersion < 26 are no longer supported. As of the second half of 2018, Google Play requires that new apps and app updates target API level 26 or higher.
I'm not sure it is possible. I think that there are things that we are doing and are not allowed by new code. (IIRC this has to do with broadcasting, but probably @jamorham remembers more).

[nickb24]

Regarding "migrate rxjava (1.3.3) to rxjava2", I've already done that in my fork if you want to check out that code. I've also changed the targetSdk to version 26.

I'm testing on a Pixel 2 running Android 11 and everything seems to be working fine so far.

[tolot27]

@nickb24

Regarding "migrate rxjava (1.3.3) to rxjava2", I've already done that in my fork if you want to check out that code. I've also changed the targetSdk to version 26.

Thx for the offer, but we need clean commits/PRs which addresses certain problems "atomically". I took a look at your fork and found that commit 1a6d6a3 mixes rxJava and Bt related stuff. If would be great I you can provide a clean PR.

[tolot27]

+1000 to enable incremental compilation

It already works for me if I go straight to Android Gradle Plugin 4.1.1 and use the jetifier (android.useAndroidX=true and android.enableJetifier=true in gradle.properties). That was just to test it. We should go step by step.

As for:
TargetSdkVersion < 26 are no longer supported. As of the second half of 2018, Google Play requires that new apps and app updates target API level 26 or higher.
I'm not sure it is possible. I think that there are things that we are doing and are not allowed by new code. (IIRC this has to do with broadcasting, but probably @jamorham remembers more).

Yea, that's a problem that it is not documented. Anyway, API level 26+ is only required if we like to provide an app in the Google Play Store.

[nickb24]

@nickb24

Regarding "migrate rxjava (1.3.3) to rxjava2", I've already done that in my fork if you want to check out that code. I've also changed the targetSdk to version 26.

Thx for the offer, but we need clean commits/PRs which addresses certain problems "atomically". I took a look at your fork and found that commit 1a6d6a3 mixes rxJava and Bt related stuff. If would be great I you can provide a clean PR.

No problem I understand. However only the following files are needed for rxjava2 migration:
app/src/main/java/com/eveningoutpost/dexdrip/ShareTest.java
app/src/main/java/com/eveningoutpost/dexdrip/Services/DexShareCollectionService.java
app/src/main/java/com/eveningoutpost/dexdrip/ImportedLibraries/dexcom/ReadDataShare.java

Those files have no bluetooth code modified, only rxjava.

[tolot27]

@nickb24 Okay, thanks. I just scammed the commits. Will continue tomorrow.

[emp-00]

Improving the code allowing xDrip+ to be provided by the official PlayStore would be a great benefit to all ppl with company phones. These are blocking sideloading of unknown apps ("blocked by our IT admin") -> only PlayStore apps are allowed for "security reasons". With the latest Android security updates (Samsung company phone) all workarounds to enable sideloading xDrip+ are now being blocked as well, so it's a rather urgent issue.

[tim162]

@tolot27 Migrating to AndroidX (probably done by an experienced xDrip dev) also gives the opportunity for new features. I implemented Android Auto compatibility where your car can read your alarms loud for you but the car connectivity required migrating to AndroidX. I've done it in my branch but as many many files have automatically been changed while migrating and I'm quite new to xDrip developing, I have no idea if all of the other features are still working after Android Studios auto-migration.

As soon as the migration to AndroidX is done, I can implement the Android Auto connectivity with ease.

[tolot27]

@tim162 Please can you create a new branch in your repro based on master and just set android.useAndroidX=true and android.enableJetifier=true in gradle.properties and test your Android Auto related code? With that options enabled you can also upgrade the Android Studio Gradle Plugin. There is no need to change lots of classes if the jetifier is enabled.

[tim162]

@tolot27 you can check #1533 (comment)
I implemented the feature in another way (imo way better) and explained everything in the PR description. I can do the migration to AndroidX in a seperate PR but I wanted to seperate it as there are many many small file changes.

[jamorham]

This is a perfectly reasonable set of tasks but there are also reasons why many of these things have not been done before. The most significant is that it is very hard to test the subtle interaction changes that can occur with library and other changes.

I'll just comment on the things I think are significant or disagree with:

I don't believe the assumption that newer versions of google libraries are always better is correct. I've been bitten by minor changes to even the build tool revision numbers causing behavior changes before so it needs to be handled with lots of care. Older libraries get tested on newer devices but rarely does that happen in reverse. For example some LG Android 5 phones don't support vector drawables which is why xDrip works around that.

Remote repositories can stop hosting the libraries we might be using which might cause problems in the future. I don't agree with moving locally hosted ones to reference the remote repository without a good reason. Google famously removed one of the wear libraries we were using.

I very much doubt that xDrip will get listed on the play store. Just one example we would have to remove SMS functionality. Their policies are very developer unfriendly and unfriendly to apps which want to do things like always maintaining bluetooth connections etc.

Targeting newer SDK versions is probably worth avoiding unless we have to. I think we'd lose access to some of the mechanisms we're using to keep the app always running smoothly and also the hidden api calls that we use to work around bugs in the framework.

Question regarding incremental compilation, does this still work while maintaining the minsdkversion at 18? don't you exceed the 65k public method limit and therefore need to use the proguard shrinking which is incompatible with it?

[tolot27]

Some new security-related tasks arose during the code review:

- [ ] remove Google Maps key from repository (#1596 adds fundamental support)
- [ ] protect `google-services.json` either by using [Travis Encrypting Files](https://docs.travis-ci.com/user/encrypting-files) or creating a new build type containing a dummy `google-services.json` file.

I had to add it to the initial comment to get the tasks recognized.

[tolot27]

This is a perfectly reasonable set of tasks but there are also reasons why many of these things have not been done before. The most significant is that it is very hard to test the subtle interaction changes that can occur with library and other changes.

I agree with you that we need comprehensive tests. Most of the bugs reported here are related to the devices xDrip supports (mostly related to Bluetooth). If we would use the different build channels better, we could get better feedback from the users willing to test newer builds. Up to now, most of the users have to install the nightly build. That is the biggest risk I can see so far.

I don't believe the assumption that newer versions of google libraries are always better is correct. I've been bitten by minor changes to even the build tool revision numbers causing behavior changes before so it needs to be handled with lots of care. Older libraries get tested on newer devices but rarely does that happen in reverse.

I totally agree with you that newer libraries are not always better. But often, they are better and much safer. The AndroidX libraries for instance are increasing the compatibility - not in every case, that is not possible, but for most of the apps.

For example some LG Android 5 phones don't support vector drawables which is why xDrip works around that.

How many users are still using LG Android 5 phones?

Remote repositories can stop hosting the libraries we might be using which might cause problems in the future. I don't agree with moving locally hosted ones to reference the remote repository without a good reason. Google famously removed one of the wear libraries we were using.

Indeed that happened and can happen in the future. But when it happens, we can integrate the latest available/most suitable library into xDrip at that time. Currently, I do not see any reason why we should keep unused libraries in the xDrip repository and integrating them into the apk.

Targeting newer SDK versions is probably worth avoiding unless we have to. I think we'd lose access to some of the mechanisms we're using to keep the app always running smoothly and also the hidden api calls that we use to work around bugs in the framework.

We can target new SDK versions to test it with newer builds (on the nightly channel) while not increasing minSdkVersion. For some features (i. e. the Crowdin SDK) it is necessary to increment targetSdkVersion to 25. But this is just required for a special build.
But I think there is a good reason to target newer SDK versions: Forward Compatibility. The majority of devices running a much newer Android version were a lot of security risks/holes are fixed and likely also many framework bugs (BTW: The November Android patch also fixed a Bluetooth security issue and did not introduce any new framework bug). As long as we document the workarounds that "keep the app always running smoothly and also the hidden api calls" and writing tests for it, we can try newer versions safely. I know, not everything can be tested, but most of the things. We can even setup travis to create builds for many different devices with test integration.

Question regarding incremental compilation, does this still work while maintaining the minsdkversion at 18? don't you exceed the 65k public method limit and therefore need to use the proguard shrinking which is incompatible with it?

I wonder why we should still support Android 4.3 (= API version 18). I cannot believe that there is a significant number of users using such old Android devices and requiring the most recent version of xDrip. Anyway, we can create additional build types for lower API versions any time. Before we do that, we could ask the users analyse log statistics.
Regarding to the 65k methods limit: multiDexEnabled is enabled in build.gradle and automatically enabled if API level >= 21. Hence, there should be no problem because it is working for a long time.

The incremental compilation depends just on the Gradle version and Android Gradle Plugin (AGP) version used and not on minSdkVersion. I've tried it already and it works.

My hope is that it is getting more and more attractive for developers to contribute to xDrip. Once that's the case, we can improve testing and integrate new features faster.

[tolot27]

I did a rework of the incremental compilation and pushed a new PR #1598.

[tolot27]

One more advantage of upgrading AGP to 4.x is the availability of the Build Analyzer. But once again, this needs the fix of the package names.

[androckz]

Hi all.
I'd like to build the project, but I can't find build instructions anywhere. I noticed that issue #1012 has been closed, so I try to ask here.
In a windows 10 (and/or Linux if possible) environment, what are the requirements to build the project (main branch) with no errors and no need to modify sources or other (hidden) options? I mean... installed Android SDKs, JDK version, gradle version, android studio version and so on. And also...is Android Studio always necessary or can we build from command line?

If someone will briefly explain me how to build, I'm available to write some documentation about it.

Thanks a lot

[nickb24]

Hi all.
I'd like to build the project, but I can't find build instructions anywhere. I noticed that issue #1012 has been closed, so I try to ask here.
In a windows 10 (and/or Linux if possible) environment, what are the requirements to build the project (main branch) with no errors and no need to modify sources or other (hidden) options? I mean... installed Android SDKs, JDK version, gradle version, android studio version and so on. And also...is Android Studio always necessary or can we build from command line?

If someone will briefly explain me how to build, I'm available to write some documentation about it.

Thanks a lot

I don't think there are any special build instructions. I installed latest Android Studio in Ubuntu. Open Android studio and follow the prompts to import a project from Github. The project should build just fine.

If you can post where you are stuck in this process maybe I can take a look and see if I can help.

[tolot27]

And also...is Android Studio always necessary or can we build from command line?

Indeed, you can build from command line using gradle. @afcady provided a makefile with #903.

If someone will briefly explain me how to build, I'm available to write some documentation about it.

That would be great. Maybe you can extend https://xdrip.github.io?

[nightscout-ricsi]

This is a very straightforward work. Is there any work going on on this project?
I suppose the regrouping of the settings would be a very handy change, too. And I think alerts should be inside Settings, but on the same level. And reminders should be in the same panel as alerts (Alert and reminders).

[tolot27]

Thx for the offer, but we need clean commits/PRs which addresses certain problems "atomically". I took a look at your fork and found that commit 1a6d6a3 mixes rxJava and Bt related stuff. If would be great I you can provide a clean PR.

No problem I understand. However only the following files are needed for rxjava2 migration: app/src/main/java/com/eveningoutpost/dexdrip/ShareTest.java app/src/main/java/com/eveningoutpost/dexdrip/Services/DexShareCollectionService.java app/src/main/java/com/eveningoutpost/dexdrip/ImportedLibraries/dexcom/ReadDataShare.java

@nickb24 Why did you add the throws Exception to every accept(Long s) method definition? It is already defined at the Consumer interface. Did you see any side effect not declaring the exception in the overridden functions?

[nickb24]

Thx for the offer, but we need clean commits/PRs which addresses certain problems "atomically". I took a look at your fork and found that commit 1a6d6a3 mixes rxJava and Bt related stuff. If would be great I you can provide a clean PR.

No problem I understand. However only the following files are needed for rxjava2 migration: app/src/main/java/com/eveningoutpost/dexdrip/ShareTest.java app/src/main/java/com/eveningoutpost/dexdrip/Services/DexShareCollectionService.java app/src/main/java/com/eveningoutpost/dexdrip/ImportedLibraries/dexcom/ReadDataShare.java

@nickb24 Why did you add the throws Exception to every accept(Long s) method definition? It is already defined at the Consumer interface. Did you see any side effect not declaring the exception in the overridden functions?

That's a good question. I'm honestly unsure at this point, it's been over 1 year that I touched that code. Looking at it now I think you are right, it can be removed. I believe at the time I was following a guide from Github on how to migrate to RxJava2 and it might have come from there. I believe I was trying to catch null values, since RxJava2 doesn't accept null values Docs.

[tolot27]

Regarding code formatting @jwoglom suggested the Github Action Checkstyle for Java and @suside suggested spotless.

According to @suside spotless has the following advantages over actions/checkstyle-for-java:

1. can be run on dev machine with ./gradlew spotlessCheck
2. can format code with ./gradlew spotlessApply
3. can be extended to pretty much all file types
4. it can lint only differences between PR and master branch (perhaps actions/checkstyle-for-java has that too?)

It could be something like this

GitHub Actions can be tested on developer machines as well using act.

A feature what I really like to have is partial code formatting. For the moment, just the code a commit provides/touches should be reformatted to not distract the review process too much. Is this possible with one of these tools?

[tolot27]

It looks like checkstyle-for-java supports partial reformatting using filter-mode. Since it uses reviewdog, many other languages are supported as well. Here, Kotlin, JSON and XML are of interest.

Spotless seem to support gradual formatting but the setup looks trickier.

[suside]

I did some testing and checkstyle+reviewdog wins when it comes to checking only changed lines.
Spotless gradual formatting works per file, so even if I change only one line the whole commited file is reformatted 😞
Checkstyle for Java works per added line or diff_context, but the output is distracting given I changed only two lines example.

IMO linter should fail during build while suggestions should be in the log output and not as review comments. Such scenario is possible with CLI run like so checkstyle -f xml -c /usr/share/checkstyle/google_checks.xml $(git diff --name-only origin/master) | reviewdog -f=checkstyle -diff="git diff origin/master" -fail-on-error

[tolot27]

Thanks for testing this. It looks like sometimes an incorrect line is reported (i. e. suside#5 (comment)).
Is it possible to stripe the package name from the classes (com.puppycrawl.tools.checkstyle.checks.sizes.LineLengthCheck -> LineLengthCheck)

[suside]

Is it possible to stripe the package name from the classes...

I don't think it's possible without external tools to format checkstyle output before it is fed to reviewdog.