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

Elimination of Lint warnings #524

Closed
wants to merge 138 commits into from

Conversation

gerhardol
Copy link
Collaborator

Android Lint as well as Android Studio (IntelliJ) Inspect issues.
(Inspect and Lint partly use the same engines, but Inspect has some separate checks and configuration is partly separate).

The changes that has required more manual changes or should change something has been submitted as separate PRs. This PR is based on open PRs too (those were separated over time). Some files may give merge conflicts, like the build.gradle files that have been changed in many commits and branches.

The PR is big, but the alternative would be to submit 96 sequenced PR, as the changes to some extent are related. Most changes are relative easy to review, the changes are grouped per commit, review per commit can be done. (I recommend that viewing changes is done offline.) Certain commits can be reverted

The intention has been to have a clean Lint report, to be able to see new issues. This is almost the case, one error left. The GradleCompatible warning should be handled, but I have not found how this should be done. Froyo has a GradleDependency warning too, due to Google not supporting 2.2 and that the Lint check there is incorrect.

The out-of-the-box configuration for Lint has been used as much as possible, with minor adjustments (regardless of what I personally think of some of the issues). The strategy has been to fix as much as possible of the warnings, suppress in the code where reasonable and overriding in build.gradle, lint.xml or Project_Defaults.xml for general changes.

Related to the lint, the @TargetApi() override was removed. (See the commit message for explanations there)

The outcome of the review has been a few optimizations and improvements. This PR has few improvements in itself. However, it greatly simplifies finding new issues. Currently, you do not know if issues in code you are working with were there before or not. Making the changes once limits the "background noise" long term.

This has been tested in GenyMotion emulator on 7.0 and 2.3, as well as Sony Z3 6.0. However, I have separated commits and rebased several times, the final version is not separately tested much yet.

Will require manual checks or rewrite code

Adding GraphView twice is done to force two inclusions and 'upgrade' the version of
Android support library that GraphView was compiled with twice
(GraphView is not used in 2.2 (but 2.3 "Froyo" builds). A wrapper will maybe suppress for Froyo.)

TODO: Find how this should be handled without suppressions. The real benefit of a separate PR is to raise awareness.

Note: RunnerUp, MapBox, GraphView, Play Services all uses different versions of Support Lib. One solution may be to compile MapBox and GraphView separately and adapt to PlayServices version....

latestCompile - Classpath for compiling the latest sources.
+--- com.android.support:design:25.1.0
|    +--- com.android.support:support-v4:25.1.0
|    |    +--- com.android.support:support-compat:25.1.0
|    |    |    \--- com.android.support:support-annotations:25.1.0
|    |    +--- com.android.support:support-media-compat:25.1.0
|    |    |    +--- com.android.support:support-annotations:25.1.0
|    |    |    \--- com.android.support:support-compat:25.1.0 (*)
|    |    +--- com.android.support:support-core-utils:25.1.0
|    |    |    +--- com.android.support:support-annotations:25.1.0
|    |    |    \--- com.android.support:support-compat:25.1.0 (*)
|    |    +--- com.android.support:support-core-ui:25.1.0
|    |    |    +--- com.android.support:support-annotations:25.1.0
|    |    |    \--- com.android.support:support-compat:25.1.0 (*)
|    |    \--- com.android.support:support-fragment:25.1.0
|    |         +--- com.android.support:support-compat:25.1.0 (*)
|    |         +--- com.android.support:support-media-compat:25.1.0 (*)
|    |         +--- com.android.support:support-core-ui:25.1.0 (*)
|    |         \--- com.android.support:support-core-utils:25.1.0 (*)
|    +--- com.android.support:appcompat-v7:25.1.0
|    |    +--- com.android.support:support-annotations:25.1.0
|    |    +--- com.android.support:support-v4:25.1.0 (*)
|    |    +--- com.android.support:support-vector-drawable:25.1.0
|    |    |    +--- com.android.support:support-annotations:25.1.0
|    |    |    \--- com.android.support:support-compat:25.1.0 (*)
|    |    \--- com.android.support:animated-vector-drawable:25.1.0
|    |         \--- com.android.support:support-vector-drawable:25.1.0 (*)
|    +--- com.android.support:recyclerview-v7:25.1.0
|    |    +--- com.android.support:support-annotations:25.1.0
|    |    +--- com.android.support:support-compat:25.1.0 (*)
|    |    \--- com.android.support:support-core-ui:25.1.0 (*)
|    \--- com.android.support:transition:25.1.0
|         +--- com.android.support:support-annotations:25.1.0
|         \--- com.android.support:support-v4:25.1.0 (*)
+--- com.google.android.gms:play-services-wearable:10.0.1
|    +--- com.google.android.gms:play-services-base:10.0.1
|    |    +--- com.google.android.gms:play-services-basement:10.0.1
|    |    |    \--- com.android.support:support-v4:24.0.0 -> 25.1.0 (*)
|    |    \--- com.google.android.gms:play-services-tasks:10.0.1
|    |         \--- com.google.android.gms:play-services-basement:10.0.1 (*)
|    \--- com.google.android.gms:play-services-basement:10.0.1 (*)
+--- com.getpebble:pebblekit:4.0.1
+--- com.mapbox.mapboxsdk:mapbox-android-sdk:4.2.0
|    +--- com.android.support:support-annotations:23.4.0 -> 25.1.0
|    +--- com.android.support:support-v4:23.4.0 -> 25.1.0 (*)
|    +--- com.android.support:design:23.4.0 -> 25.1.0 (*)
|    +--- com.squareup.okhttp3:okhttp:3.4.1
|    |    \--- com.squareup.okio:okio:1.9.0
|    +--- com.mapzen.android:lost:1.1.1
|    |    +--- com.android.support:appcompat-v7:22.2.0 -> 25.1.0 (*)
|    |    +--- com.android.support:support-v4:22.2.0 -> 25.1.0 (*)
|    |    \--- com.google.guava:guava:18.0
|    \--- com.mapbox.mapboxsdk:mapbox-java-services:1.3.1
|         +--- com.squareup.retrofit2:retrofit:2.1.0
|         |    \--- com.squareup.okhttp3:okhttp:3.3.0 -> 3.4.1 (*)
|         +--- com.squareup.retrofit2:converter-gson:2.1.0
|         |    +--- com.squareup.retrofit2:retrofit:2.1.0 (*)
|         |    \--- com.google.code.gson:gson:2.7
|         \--- com.squareup.okhttp3:logging-interceptor:3.3.1
|              \--- com.squareup.okhttp3:okhttp:3.3.1 -> 3.4.1 (*)
\--- com.jjoe64:graphview:4.2.1
     \--- com.android.support:support-v4:22.1.1 -> 25.1.0 (*)
gerhardol added a commit to gerhardol/runnerup that referenced this pull request Jul 31, 2017
This simplifies viewing newly introduced issues, a temporar measure until jonasoreland#524 is merged
@davidedelvento
Copy link

So how about merging this PR after the next release and in the current beta?

@gerhardol
Copy link
Collaborator Author

@davidedelvento
That is my proposal too, make a new beta quickly after.

I could try to break out some specific changes, but it will not give so much.

@gerhardol gerhardol added this to the Classic_Next milestone Aug 10, 2017
@gerhardol
Copy link
Collaborator Author

@jonasoreland @davidedelvento
I will merge master and check for newly added issues before merging

Should this be squashed or not?
There are many commits, they have information why a change was made though.

Should this be merged to classic as well as ng?
If only ng, it will be hard to apply patches for the branches.

@cmmata
Copy link

cmmata commented Aug 11, 2017

My personal opinion: it's better to add all commits rather than squash to only one, because:

  • In one squashed commit, you normally loose commit messages. Or it's more difficult to see which file is affected for every commit message
  • If one change makes problems, you can see exactly what is changed and revert that files. If it's a big squashed commit, it's more difficult to find problematic files and revert them.

And the second question, I think it will be better to merge in Classic and NG. If someone needs to make a change in classic branch, it will be nice to have that warnings solved.

But it's only my opinion :)

…tions

# Conflicts:
#	.gitignore
#	.travis.yml
#	app/AndroidManifest.xml
#	app/latest/java/org/runnerup/feedwidget/FeedWidgetService.java
#	app/lint-baseline.xml
#	app/src/org/runnerup/db/DBHelper.java
#	app/src/org/runnerup/export/DefaultSynchronizer.java
#	app/src/org/runnerup/export/DigifitSynchronizer.java
#	app/src/org/runnerup/export/EndomondoSynchronizer.java
#	app/src/org/runnerup/export/FileSynchronizer.java
#	app/src/org/runnerup/export/FunBeatSynchronizer.java
#	app/src/org/runnerup/export/GarminSynchronizer.java
#	app/src/org/runnerup/export/GoogleFitSynchronizer.java
#	app/src/org/runnerup/export/RunKeeperSynchronizer.java
#	app/src/org/runnerup/export/RuntasticSynchronizer.java
#	app/src/org/runnerup/view/AccountListActivity.java
#	build.gradle
#	common/src/main/java/org/runnerup/common/util/Constants.java
#	hrdevice/src/org/runnerup/hr/HRManager.java
#	wear/lint-baseline.xml
unused import
unused parameters and methods
variable can be final
access can be private
array type init
constantconditions

A few suppressions
@gerhardol
Copy link
Collaborator Author

Sync merged with master, reviewed changes. Will test the updates a little more before merging to master.
Meanwhile, the branch is merged to 'master_ng' branch, that also raises the minimal SDK version to 21. For @cmmata to start with.

@davidedelvento
Copy link

I agree with @cmmata regarding squashing. In fact, in my git merge I always use --no-ff.

Regarding merging this in classic too, if you could merge in classic without raising the minimal Android version required, I'd say it makes sense to have it there. Otherwise, it's mixed bag, I see the usefulness of having this, but do we want to cut people out? But in the end, I am neutral. I am not planning to do much with classic, so I'm fine either way.

@gerhardol
Copy link
Collaborator Author

@davidedelvento
Minimal SDK is not raised (there are lints suppressed due to minimal in latest is 15 and minimal in froyo is 8).
It is much easier to cherry pick corrections from one branch to the other so I definitely want this. For instance for Synchronizers, sensors. A little more testing first.
Maybe even the AppCompat changes should be merged to classic for that reason (but the "views" will likely need to be rewritten anyway).

@gerhardol
Copy link
Collaborator Author

Superseded by #671

@gerhardol gerhardol closed this Jul 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants