Issue 29345540: Issue 4030 - Move JNI bindings into separate library project (Closed)
|
Created:
June 3, 2016, 1:42 p.m. by anton Modified:
Sept. 8, 2017, 11:10 a.m. CC:
René Jeschke Visibility:
Public. |
DescriptionIssue 4030 - Move JNI bindings into separate library project
Patch Set 1 #
Total comments: 10
Patch Set 2 : Changeset in adblockplusandroid repo #
Total comments: 2
Patch Set 3 : Additional changes to libadblockplus-android with Diego's suggestions #
MessagesTotal messages: 6
since the things changed (created separate libadblockplus-android repo) i've modified the structure of directories keeping in mind: 1) it will be tracked as dependency 2) it will have tests app
Assuming patchset #1 is applied in fact (we discussed it with Rene and decided to add the files to libadblockplus-android repo and don't remove them from adblockplusandroid repo for now) patchset #2 is what is going to be done in adblockplus repo. In brief: 1) libadblockplus-adnroid files are removed 2) dependency to libadblockplus-android repo is added 3) Utils class is splitted into context-independent Utils class in the library and AppUtils class that depends on android Context. One small note: we're going to avoid using apache commons lang utils and rewrite used functions. To be done in separate task (leaving it in the library for now). https://codereview.adblockplus.org/29345540/diff/29348338/README.md File README.md (right): https://codereview.adblockplus.org/29345540/diff/29348338/README.md#newcode68 README.md:68: _Android Application_. i think it would be useful to add how to use this proxy app to get ads filtered out (the same intro just to what we have running the proxy app) https://codereview.adblockplus.org/29345540/diff/29348338/dependencies File dependencies (right): https://codereview.adblockplus.org/29345540/diff/29348338/dependencies#newcode6 dependencies:6: submodules/libadblockplus-android = libadblockplus-android hg:971a28756cde git:82a2f56974043a0b130a096ddaa877accbda0c23 now we get libadblockplus-android extracted automatically while ./ensure_dependencies.py. Is there any reason to reference to particular commit instead of just tip? For nor referencing the latest commit (both hg and git)
https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblo... File submodules/libadblockplus-android/libadblockplus-android/AndroidManifest.xml (right): https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblo... submodules/libadblockplus-android/libadblockplus-android/AndroidManifest.xml:5: android:versionName="1.3" > Since libadblockplus-android will be an independent project with its own releases, I think that we should do a fresh start to its versionCode/versionName (starting from 0.1 or 1.0) instead of using adblockplusandroid current version https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblo... File submodules/libadblockplus-android/libadblockplus-android/build.xml (right): https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblo... submodules/libadblockplus-android/libadblockplus-android/build.xml:30: Shouldn't we add the <fail> tag here in case of missing ndk.dir? https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblo... File submodules/libadblockplus-android/libadblockplus-android/src/org/adblockplus/android/Utils.java (right): https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblo... submodules/libadblockplus-android/libadblockplus-android/src/org/adblockplus/android/Utils.java:45: public static String capitalizeString(final String s) Why this method is kept here? It isn't called anywhere in the lib. This should be placed in the application AppUtils https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblo... submodules/libadblockplus-android/libadblockplus-android/src/org/adblockplus/android/Utils.java:57: public static void appendRawTextFile(final Context context, final StringBuilder text, final int id) This method isn't called by the lib and is redundant in AppUtils
https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblo... File submodules/libadblockplus-android/libadblockplus-android/AndroidManifest.xml (right): https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblo... submodules/libadblockplus-android/libadblockplus-android/AndroidManifest.xml:5: android:versionName="1.3" > On 2016/09/06 01:05:49, diegocarloslima wrote: > Since libadblockplus-android will be an independent project with its own > releases, I think that we should do a fresh start to its versionCode/versionName > (starting from 0.1 or 1.0) instead of using adblockplusandroid current version That makes sense, i'd prefer 1.0 since we have released product (proxy app) that is using the library. https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblo... File submodules/libadblockplus-android/libadblockplus-android/build.xml (right): https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblo... submodules/libadblockplus-android/libadblockplus-android/build.xml:30: On 2016/09/06 01:05:49, diegocarloslima wrote: > Shouldn't we add the <fail> tag here in case of missing ndk.dir? Yes, we should. And add the same rule to check sdk.dir https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblo... File submodules/libadblockplus-android/libadblockplus-android/src/org/adblockplus/android/Utils.java (right): https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblo... submodules/libadblockplus-android/libadblockplus-android/src/org/adblockplus/android/Utils.java:45: public static String capitalizeString(final String s) On 2016/09/06 01:05:49, diegocarloslima wrote: > Why this method is kept here? It isn't called anywhere in the lib. This should > be placed in the application AppUtils The original task was to split the sources and don't introduce any changes like remove (unused) methods. So this should be done in separate task like "clean-up". https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblo... submodules/libadblockplus-android/libadblockplus-android/src/org/adblockplus/android/Utils.java:57: public static void appendRawTextFile(final Context context, final StringBuilder text, final int id) On 2016/09/06 01:05:50, diegocarloslima wrote: > This method isn't called by the lib and is redundant in AppUtils Same reason: don't do any modifications for the source code in this task
https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblo... File submodules/libadblockplus-android/libadblockplus-android/src/org/adblockplus/android/Utils.java (right): https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblo... submodules/libadblockplus-android/libadblockplus-android/src/org/adblockplus/android/Utils.java:45: public static String capitalizeString(final String s) On 2016/09/06 05:56:23, anton wrote: > On 2016/09/06 01:05:49, diegocarloslima wrote: > > Why this method is kept here? It isn't called anywhere in the lib. This should > > be placed in the application AppUtils > > The original task was to split the sources and don't introduce any changes like > remove (unused) methods. So this should be done in separate task like > "clean-up". It doesn't make much sense to me, since some changes were already made to move utility methods that are only used by the application to AppUtils, e.g. showUpdateNotification() and updateSubscriptionStatus(). This method is only required by AdblockPlus.java and IMHO it makes no sense for an utility method only required by the application part to stay in the lib part
https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblo... File submodules/libadblockplus-android/libadblockplus-android/src/org/adblockplus/android/Utils.java (right): https://codereview.adblockplus.org/29345540/diff/29345541/submodules/libadblo... submodules/libadblockplus-android/libadblockplus-android/src/org/adblockplus/android/Utils.java:45: public static String capitalizeString(final String s) On 2016/09/06 14:08:58, diegocarloslima wrote: > On 2016/09/06 05:56:23, anton wrote: > > On 2016/09/06 01:05:49, diegocarloslima wrote: > > > Why this method is kept here? It isn't called anywhere in the lib. This > should > > > be placed in the application AppUtils > > > > The original task was to split the sources and don't introduce any changes > like > > remove (unused) methods. So this should be done in separate task like > > "clean-up". > > It doesn't make much sense to me, since some changes were already made to move > utility methods that are only used by the application to AppUtils, e.g. > showUpdateNotification() and updateSubscriptionStatus(). > > This method is only required by AdblockPlus.java and IMHO it makes no sense for > an utility method only required by the application part to stay in the lib part In general you are right. But this method is general-purpose and it's not related to app context. In contrast AppUtils class methods relate to context or app entities like activities, so i'd prefer to have this function here. Honestly StringUtils methods should be used instead of creating new method here. Keep it here for later tasks. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
