search

commons-app / apps-android-commons

The Wikimedia Commons Android app allows users to upload pictures from their Android phone/tablet to Wikimedia Commons
https://commons-app.github.io/
Apache License 2.0
1.03k stars 1.23k forks source link

Go through all our existing dependencies to see if we should keep or remove them #3128

Closed misaochan closed 4 years ago

misaochan commented 5 years ago

Subtask of #1092

We have already implemented a stricter process for including new libraries in pull requests, and we will now go through all our existing dependencies to see if we should keep or remove them, and refactor the code accordingly.

RozaliiaRusskikh commented 5 years ago

Hello! As I understand we have to remove unused dependencies only?

nicolas-raoul commented 5 years ago

@RozaliyaRusskikh Yes, just comment each one by one and check whether it still works :-)

RozaliiaRusskikh commented 5 years ago

Thank you! Could I take this issue?

nicolas-raoul commented 5 years ago

@RozaliyaRusskikh OK! Please post your findings here.

This is a first step before a more in-depth look at each remaining dependency to see whether some could be dropped by changing our source code a bit.

RozaliiaRusskikh commented 5 years ago

I have looked through existing dependencies located in build. gradle (Module: app) file to see if they should be deleted or kept. I have commented each one by one and have checked whether it still works. The next dependencies are unused:

implementation 'com.google.code.gson:gson:2.8.5'
implementation 'io.reactivex.rxjava2:rxandroid:2.1.0'
implementation 'io.reactivex.rxjava2:rxjava:2.2.3'
implementation 'com.drewnoakes:metadata-extractor:2.11.0'
implementation 'com.mapbox.mapboxsdk:mapbox-android-sdk:6.8.0'
implementation 'com.jakewharton.timber:timber:4.7.1'
implementation "com.google.dagger:dagger:$DAGGER_VERSION"
nicolas-raoul commented 5 years ago

Nice! Can you please remove them and send a pull request? I forgot: Please also test on the lowest version of Android supported by the app (you can find in the manifest and create an AVD emulator for that Android version). Thanks! :-)

RozaliiaRusskikh commented 5 years ago

Could you please have a look at PR?

nicolas-raoul commented 5 years ago

@RozaliyaRusskikh Thanks for the PR! Review might take a bit of time since it is not a high-priority issue. By the way, here is a high-priority issue which is beginner-friendly, feel free to take it: #3126

misaochan commented 5 years ago

Hi @RozaliyaRusskikh , thanks for your interest! This issue is a little more complex than just removing unused dependencies, but that is a fantastic first step. I have merged your PR.

The next step would be deciding which USED dependencies are redundant and which ones are not, but that would be part of a larger discussion.

RozaliiaRusskikh commented 5 years ago

Thank you, @misaochan, @nicolas-raoul! I am very glad to hear it! It is very interesting to work on your team!

maskaravivek commented 5 years ago

I have reviewed all the dependencies that the app currently uses and apart from 2-3 dependencies all other libraries seem to be essential. Have attached links to the PRs for removing the non essential dependencies.

Thanks @RozaliyaRusskikh for removing the unused dependencies. Also just for documentation sake, we currently found just 2-3 dependencies which are non-essential as we carried out the excercise of removing lots of non essential dependencies in the last few months.

Links to some of those PRs

implementation 'com.github.nicolas-raoul:Quadtree:ac16ea8035bf07'

This library has a Java implementation of Quadtree and is being used for caching of categories. IMO its better to keep using this library.

Related: https://github.com/commons-app/apps-android-commons/pull/3163

implementation 'com.google.code.gson:gson:2.8.5'

Needed for JSON parsing.

implementation 'in.yuvi:http.fluent:1.3'

Should be removed. We no longer need this wrapper for Http client.

PR: https://github.com/commons-app/apps-android-commons/pull/3164

implementation 'com.squareup.okhttp3:okhttp:3.12.1' implementation 'com.squareup.okio:okio:1.15.0'

Required for OkHttp.

implementation 'io.reactivex.rxjava2:rxandroid:2.1.0' implementation 'io.reactivex.rxjava2:rxjava:2.2.3' implementation 'com.jakewharton.rxbinding2:rxbinding:2.1.1' implementation 'com.jakewharton.rxbinding2:rxbinding-support-v4:2.1.1' implementation 'com.jakewharton.rxbinding2:rxbinding-appcompat-v7:2.1.1' implementation 'com.jakewharton.rxbinding2:rxbinding-design:2.1.1'

Required for RxJava

implementation 'com.facebook.fresco:fresco:1.13.0'

Required for image loading

implementation 'com.drewnoakes:metadata-extractor:2.11.0'

Can be removed as suggested by dbrant. https://github.com/commons-app/apps-android-commons/pull/2947

implementation 'org.apache.commons:commons-lang3:3.8.1'

Yes, its being used for some utility functions.

implementation 'com.github.maskaravivek:wikimedia-android-data-client:v0.0.27'

Needed for media wiki api calls

// UI

implementation 'fr.avianey.com.viewpagerindicator:library:2.4.1.1@aar'

Used for page indicators. It okay to use it for now but maybe later we can do away with this dependency.

implementation 'com.github.chrisbanes:PhotoView:2.0.0'

Required for photo view

implementation 'com.github.pedrovgs:renderers:3.3.3'

Required for reducing boilerplate in recycler views. @neslihan can confirm if this is still required.

implementation 'com.mapbox.mapboxsdk:mapbox-android-sdk:6.8.0' implementation 'com.mapbox.mapboxsdk:mapbox-android-plugin-localization:0.6.0'

Mapbox related libraries

implementation 'com.github.deano2390:MaterialShowcaseView:1.2.0'

Required for our showcase views. Would be too much of an effort to implement it on our own

implementation 'com.dinuscxj:circleprogressbar:1.1.1'

Required for showing the progress bar in achivements activity

implementation 'com.karumi:dexter:5.0.0'

Required for runtime permissions

implementation "com.jakewharton:butterknife:$BUTTERKNIFE_VERSION" kapt "com.jakewharton:butterknife-compiler:$BUTTERKNIFE_VERSION"

Required for view bindings. Can't be removed

// Logging

implementation 'ch.acra:acra-dialog:5.3.0' implementation 'ch.acra:acra-mail:5.3.0'

ACRA related dependencies. We can't live without it. :)

implementation 'com.jakewharton.timber:timber:4.7.1'

Required for logging.

implementation 'org.slf4j:slf4j-api:1.7.25' api('com.github.tony19:logback-android-classic:1.1.1-6') { exclude group: 'com.google.android', module: 'android' }

These are required for our send logs feature. Can't be removed

// Dependency injector

implementation "com.google.dagger:dagger:$DAGGER_VERSION" implementation "com.google.dagger:dagger-android-support:$DAGGER_VERSION" kapt "com.google.dagger:dagger-android-processor:$DAGGER_VERSION" kapt "com.google.dagger:dagger-compiler:$DAGGER_VERSION"

Dagger dependency. We use dagger throughout the application.

// Debugging

releaseImplementation "com.squareup.leakcanary:leakcanary-android-no-op:$LEAK_CANARY_VERSION"

Leak canary is packaged only for debug builds and helps us catch memory leaks.

// Support libraries

implementation "com.google.android.material:material:1.1.0-alpha04" implementation "androidx.browser:browser:1.0.0" implementation "androidx.cardview:cardview:1.0.0" implementation 'androidx.constraintlayout:constraintlayout:1.1.3' implementation "androidx.exifinterface:exifinterface:1.0.0"

Again these dependencies are required to support features on older devices. Can't do away with it.

//swipe_layout

implementation 'com.daimajia.swipelayout:library:1.2.0@aar'

Required for swipe layout. Its good to have library otherwise manual implementation would be a bit difficult.

//metadata extractor

implementation 'com.drewnoakes:metadata-extractor:2.11.0'

Should be removed.

misaochan commented 5 years ago

@nicolas-raoul @neslihanturan @dbrant Would appreciate thoughts on https://github.com/commons-app/apps-android-commons/issues/3128#issuecomment-538644740 :)

ashishkumar468 commented 5 years ago

Cross verified @maskaravivek 's comment, LGTM :)

maskaravivek commented 4 years ago

@misaochan Can this issue be closed now?

misaochan commented 4 years ago

Sounds good to me!