search

NightscoutFoundation / xDrip

Nightscout version of xDrip+
https://jamorham.github.io/#xdrip-plus
GNU General Public License v3.0
1.42k stars 1.15k forks source link

Code Review: refactor code #1522

Closed tolot27 closed 9 months ago

tolot27 commented 4 years ago

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:

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

tzachi-dar commented 4 years ago

+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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

+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 commented 4 years ago

@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 commented 4 years ago

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

emp-00 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@tolot27 you can check https://github.com/NightscoutFoundation/xDrip/pull/1533#issue-530453200 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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

tolot27 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 :disappointed: 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 commented 2 years ago

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

suside commented 2 years ago

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.