billing proguard option

[Gerarddus]

adding billing proguard option through cordova plugin add cordova-plugin-proguard --variable HAS_BILLING=true

If you add

-keep class com.android.vending.billing.** { *; }

Directly to proguard-custom.txt, anyone who is not using the billing class will get (unnecessary) warnings. I figured there should be a way to customize the proguard-custom.txt file without having to fork the repo. So my proposal is to add a hook ‘androidBeforeInstall.js’ which adds the needed line based on the commandline.

cordova plugin add cordova-plugin-proguard --variable HAS_BILLING=true

will add the billing line, and if set to false (or anything else) it will not.

It has a couple of drawbacks though:

1) If you remove the plugin, you should add the variable name again, otherwise cordova will complain 2) If you omit the HAS_BILLING, cordova will complain.

You can add other options by using more variables like so:

cordova plugin add cordova-plugin-proguard --variable HAS_BILLING=true —-variable HAS_SOMETHING_ELSE=true

What do you think?

[greybax]

@Gerarddus thank you for your PR! Sounds good to me but I have couple questions before we will go with these changes.

2. If you omit the HAS_BILLING, cordova will complain.

If user will just run command cordova plugin add cordova-plugin-proguard as it is for now, will user get any errors or exceptions? If it is, so can we somehow to avoid it. I just want to make fo sure that we save backward compatibility for all our users.

Could you please also describe these changes and how it works in README.md file?

[greybax]

@Gerarddus any updates? Did you have a chance to take a look on my comment?

[Gerarddus]

@greybax, I've read them, and I agree, but haven't had the time yet to look into it just yet.

[Gerarddus]

@greybax, it looks like the removal of <preference name="HAS_BILLING" /> fixed the 2 mentioned problems of a required variable when adding/removing the plugin.

[greybax]

Nice! @Gerarddus could you please also add a description update into README.md file regarding how this plugin will work with new changes (how to add new parameters)?

[Gerarddus]

@greybax, would like to do so, but I have some second thoughts:

1. Right now you'd have to explicitly add the billing feature when installing the plugin, an alternative could be to explicitly not add the billing feature.
2. The proguard line -keep class com.android.vending.billing.** { *; } is now included in the javascript, perhaps its best to have it in the proguard textfile and just uncomment/comment it through javascript. I think that this would be a better implementation, it makes the proguard-custom.txt have all the possibilities instead of disperse them in a javascript like I did.
3. I'm not sure if this way of working with the commandline interface will work when other plugins (i'm thinking for instance of https://github.com/j3k0/cordova-plugin-purchase) would use it as a dependency using <dependency id="cordova-plugin-proguard" url="https://github.com/greybax/cordova-plugin-proguard" commit="master" /> since the spec doesn't seem to make it possible to add variables.

Further more, I still have to check (I keep forgetting that) what happens if you add the proguard line but don't use the billing library.

Do you have any thoughts on all of this?

[greybax]

@Gerarddus please let me share my thoughts on your questions:

Right now you'd have to explicitly add the billing feature when installing the plugin, an alternative could be to explicitly not add the billing feature.

Answer: We do not have to break current functionality for users if they will update for a new version. I think by default let's keep option without adding billing option as it is now.

The proguard line -keep class com.android.vending.billing.* { ; } is now included in the javascript, perhaps its best to have it in the proguard textfile and just uncomment/comment it through javascript. I think that this would be a better implementation, it makes the proguard-custom.txt have all the possibilities instead of disperse them in a javascript like I did.

Answer: Would be good to keep all settings in one place as it is now in proguard-custom.txt. So I think would be better to put this line into there and comment/uncomment it based on setting as you proposed.

I'm not sure if this way of working with the commandline interface will work when other plugins (i'm thinking for instance of https://github.com/j3k0/cordova-plugin-purchase) would use it as a dependency using since the spec doesn't seem to make it possible to add variables.

Answer: Yes, seems we have some issues here... I think we can describe this case into README.md file at least. So user, who will refer their extensions directly to master branch on github will not be able to get flexibility as he could get it through command line installation.

[Gerarddus]

@greybax Thanks for your thinking along. I think that I'm on the wrong track somehow, making things too complicated. So: What do you think if we don't use commandline installation and have the plugin search for another proguard-custom.txt in the app-directory and merge that with the one from the plugin when installing? That way you can have defaults set in the txt file that comes with the plugin, and users have all flexibility with using their own rules. I've made a pullrequest reflecting this setup.

[greybax]

@Gerarddus Sounds good to me! 👍

One question: do you know can we add some tests which will cover current functionality. I would like to have them because seems plugin getting bigger :)

[Gerarddus]

@greybax what exactly is the functionality that you would like to test? Jslint/hint or a cordova project setup-test with plugininstall and verify?

[greybax]

@Gerarddus would be nice to have them all, but for now, would be awesome to have a test for cordova project setup-test with plugininstall and verify

Regarding jslint/hint test not sure that we have a proper tool for validating proguard-custom.txt, but I think it is a great idea for a new project (lint/hint somehow proguard-custom.txt for correct proguard rules)

What do you think? :)

[Gerarddus]

@graybax I'm not really well versed in unittesting (how about you?), but I can give it a go based on examples found elsewhere. Shall I make it into a different pullrequest?

[Gerarddus]

@greybax Created a very basic test using examples from fovea inappbilling. make tests

[greybax]

@Gerarddus looks good to me, just one thing, could you please send test-related changes with new PR? So I can attach new PR with new issue.

[Gerarddus]

@greybax I've split the pullrequest in two parts.

[greybax]

PR has been separated on two parts: #20 and #21. Merged them separetely, so just closed this PR