Nullability and javadocs #293

AllanWang · 2017-07-31T17:43:47Z

This is to add some annotations and update some old or inexisting javadocs.

Nullable and nonnull annotations help a lot for Kotlin users, where types allowing null values are distinguished.

The ones I've added are in public methods where I tracked down a possible null response. Other parts may need your input.

I also wanted to clarify some methods, so I added some javadocs and todos. I gave more comments directly on the respective lines.

A check is also added to sonatypeRepo so that devs who fork the project and compile it

AllanWang · 2017-07-31T17:48:04Z

library/build.gradle

@@ -69,9 +70,9 @@ uploadArchives {
    repositories.mavenDeployer {
        beforeDeployment { MavenDeployment deployment -> signing.signPom(deployment) }

-        repository(url: sonatypeRepo) {


sonatypeRepo is not found when building. If this is in a properties file that is gitignore, maybe add a check so other users can compile their forks

I ran into the same issue. I end up keeping this in git stash and undoing it when I push a PR. Would be great if the build file were updated to allow others to build without needing to do that.

Create a gradle.properties file and put a blank value there

@serggl if this is in your gradle property file, how about I just add a check to see if the value is defined? It'll be something along the lines of

if property file exists or variable exists in System.env (for travis), then execute the call

True. Can you add it?:)

@serggl done. It looks like you also added fake properties in the travis yml, so I removed those as it won't be necessary

AllanWang · 2017-07-31T17:48:23Z

library/src/main/java/com/anjlab/android/iab/v3/BillingProcessor.java

@@ -610,6 +615,7 @@ private boolean purchase(Activity activity, List<String> oldProductIds, String p
 							purchaseType,
 							purchasePayload);
 				}
+				//todo check if null check should be removed or if else should be removed


This will always be nonnull given the if statement above

yep. A remove candidate

I take it you mean that the null check if unnecessary. Resolved here 329c287

AllanWang · 2017-07-31T17:48:41Z

library/src/main/java/com/anjlab/android/iab/v3/BillingProcessor.java

+	 * Attempt to fetch purchases from the server and update our cache if successful
+	 * @return {@code true} if retrieval occurs, {@code false} otherwise
+	 * todo check if loading should return true if response code isn't successful
+	 */
 	public boolean loadOwnedPurchasesFromGoogle()


Currently, the load methods return true so long as a response is received, regardless of whether the response was successful or not. I also removed the first isInitialized call as it is called in both load methods. It therefore doesn't need to be verified a third time beforehand

AllanWang · 2017-07-31T17:49:49Z

library/src/main/java/com/anjlab/android/iab/v3/BillingProcessor.java

@@ -49,11 +50,11 @@
 	 */
 	public interface IBillingHandler
 	{
-		void onProductPurchased(String productId, TransactionDetails details);
+		void onProductPurchased(String productId, @Nullable TransactionDetails details);


Check if productId is nullable. As of now I don't think it is

autonomousapps

I'm in favor of this idea! What do you think, @serggl ?

autonomousapps · 2017-07-31T17:54:51Z

library/build.gradle

@@ -35,6 +35,7 @@ android {
 dependencies {
    androidTestCompile 'com.android.support.test:runner:0.5'
    androidTestCompile 'com.android.support.test:rules:0.5'
+    compile 'com.android.support:support-annotations:25.3.1'


I am personally very much in support of adding nullability annotations. Useful for us Java users, too!

autonomousapps · 2017-07-31T17:55:27Z

library/build.gradle

@@ -69,9 +70,9 @@ uploadArchives {
    repositories.mavenDeployer {
        beforeDeployment { MavenDeployment deployment -> signing.signPom(deployment) }

-        repository(url: sonatypeRepo) {


I ran into the same issue. I end up keeping this in git stash and undoing it when I push a PR. Would be great if the build file were updated to allow others to build without needing to do that.

AllanWang · 2017-07-31T18:57:06Z

library/src/main/java/com/anjlab/android/iab/v3/BillingProcessor.java

-	private static final Date DATE_MERCHANT_LIMIT_1 = new Date(2012, 12, 5); //5th December 2012
-	private static final Date DATE_MERCHANT_LIMIT_2 = new Date(2015, 7, 20); //21st July 2015
+	private static final Date DATE_MERCHANT_LIMIT_1 = new Date(1354694400000L); //5th December 2012
+	private static final Date DATE_MERCHANT_LIMIT_2 = new Date(1437375600000L); //21st July 2015


Replace with millis, since using the original notation may not produce expected results. See stack overflow. Based on PST

Are there any other options as a replacement? This is a bit less readable code

@serggl see latest commit. I'm 99% sure that that is how calendars are used, so it should be good. The date will likely vary based on timezones, but it shouldn't be off by more than a day.

serggl · 2017-08-01T06:41:52Z

library/src/main/java/com/anjlab/android/iab/v3/BillingProcessor.java

+	/**
+	 * Attempt to fetch purchases from the server and update our cache if successful
+	 * @return {@code true} if retrieval occurs, {@code false} otherwise
+	 * todo check if loading should return true if response code isn't successful


it does returns false when unsuccessful from here: https://github.com/anjlab/android-inapp-billing-v3/blob/master/library/src/main/java/com/anjlab/android/iab/v3/BillingProcessor.java#L296

@serggl I'm referring to this line where return true occurs after the if statement that checks if the response is ok. Is that expected behaviour?

oh, right. Should return false here if response was not successful

Resolved b281e51

serggl · 2017-08-01T06:56:08Z

👍

AllanWang · 2017-08-01T07:00:33Z

Currently, this is ready to be merged. However, there may be more things I'd like to clarify/annotate, so you can choose to wait for those or to merge this for now.

AllanWang · 2017-08-01T07:02:49Z

Oh one last thing. May not be suited for this PR, but you may consider updating the sdk to 26. It's out of beta now.

serggl · 2017-08-01T07:06:04Z

let me know when you decide it is ready to be merged. Thank you!

AllanWang · 2017-08-02T22:17:04Z

In the billing base class, commit is used, but I think it can be replaced with apply. You may want to look into that

AllanWang · 2017-08-02T22:07:56Z

library/src/main/java/com/anjlab/android/iab/v3/PurchaseInfo.java

@@ -108,4 +113,15 @@ public PurchaseInfo createFromParcel(Parcel source)
                    return new PurchaseInfo[size];
                }
            };
+
+    @Override
+    public boolean equals(Object o)


Check if this is good for equals or if other values should be checked. I replaced the test above to use equals rather than checking individual attributes.

AllanWang · 2017-08-02T22:53:47Z

After the comments above are addressed, this PR is ready for merging

serggl · 2017-08-04T05:48:10Z

only one small question on dates left

AllanWang · 2017-08-04T06:00:58Z

@serggl if any of you can, consider posting whatever formatting rules you use for Android Studio. Mine is different (braces are after if statements rather than on the next line etc). So I'm unable to format the code

serggl · 2017-08-04T06:15:43Z

@AllanWang I guess Im using default rules. However there are still classes which are formatted based on Eclipse rules

* Add annotations and some javadocs fixes * Add more nullables * Update loadOwnedPurchasesFromGoogle * Add check for sonatypeRepo var * Replace date * Remove extra import * Ensure load purchase only returns true on successful response * Remove unnecessary null check * Add more annotations * Remove uses of deprecated methods in library * Fix lint * Remove unused throw exception * Add this comparison * Optimize imports * Test locally and fix errors * Use calendar rather than date

AllanWang added 3 commits July 31, 2017 10:32

Add annotations and some javadocs fixes

8489dfa

Add more nullables

bfe53ac

Update loadOwnedPurchasesFromGoogle

346cf1f

AllanWang commented Jul 31, 2017

View reviewed changes

autonomousapps reviewed Jul 31, 2017

View reviewed changes

AllanWang added 2 commits July 31, 2017 11:22

Add check for sonatypeRepo var

7d3f8f8

Replace date

0ffcb9f

AllanWang commented Jul 31, 2017

View reviewed changes

Remove extra import

5a1e03b

serggl reviewed Aug 1, 2017

View reviewed changes

AllanWang added 2 commits July 31, 2017 23:48

Ensure load purchase only returns true on successful response

b281e51

Remove unnecessary null check

329c287

AllanWang added 2 commits August 2, 2017 14:56

Add more annotations

88354f0

Remove uses of deprecated methods in library

13b225e

AllanWang added 5 commits August 2, 2017 15:18

Fix lint

1bd767b

Remove unused throw exception

8472229

Add this comparison

49edd02

Optimize imports

abd3e16

Test locally and fix errors

0f57195

AllanWang commented Aug 2, 2017

View reviewed changes

Use calendar rather than date

4936ea3

serggl merged commit b04db8d into anjlab:master Aug 4, 2017

AllanWang mentioned this pull request Aug 4, 2017

Question: When does loadPurchase return false? #283

Closed

serggl added a commit that referenced this pull request Aug 7, 2017

bump build version to accomodate #289, #293, #295

be84fee

showdpro pushed a commit to showdpro/android-inapp-billing-v3 that referenced this pull request Jul 13, 2021

bump build version to accomodate anjlab#289, anjlab#293, anjlab#295

3ccec91

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nullability and javadocs #293

Nullability and javadocs #293

AllanWang commented Jul 31, 2017 •

edited

Loading

AllanWang Jul 31, 2017

autonomousapps Jul 31, 2017

serggl Jul 31, 2017

AllanWang Jul 31, 2017

serggl Jul 31, 2017

AllanWang Jul 31, 2017

AllanWang Jul 31, 2017

serggl Aug 1, 2017

AllanWang Aug 1, 2017

AllanWang Jul 31, 2017

AllanWang Jul 31, 2017

autonomousapps left a comment

autonomousapps Jul 31, 2017

autonomousapps Jul 31, 2017

AllanWang Jul 31, 2017 •

edited

Loading

serggl Aug 4, 2017

AllanWang Aug 4, 2017

serggl Aug 1, 2017

AllanWang Aug 1, 2017

serggl Aug 1, 2017 •

edited

Loading

AllanWang Aug 1, 2017 •

edited

Loading

serggl commented Aug 1, 2017

AllanWang commented Aug 1, 2017

AllanWang commented Aug 1, 2017

serggl commented Aug 1, 2017

AllanWang commented Aug 2, 2017

AllanWang Aug 2, 2017

AllanWang commented Aug 2, 2017

serggl commented Aug 4, 2017

AllanWang commented Aug 4, 2017

serggl commented Aug 4, 2017

Nullability and javadocs #293

Nullability and javadocs #293

Conversation

AllanWang commented Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autonomousapps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AllanWang Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serggl Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

AllanWang Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

serggl commented Aug 1, 2017

AllanWang commented Aug 1, 2017

AllanWang commented Aug 1, 2017

serggl commented Aug 1, 2017

AllanWang commented Aug 2, 2017

Choose a reason for hiding this comment

AllanWang commented Aug 2, 2017

serggl commented Aug 4, 2017

AllanWang commented Aug 4, 2017

serggl commented Aug 4, 2017

AllanWang commented Jul 31, 2017 •

edited

Loading

AllanWang Jul 31, 2017 •

edited

Loading

serggl Aug 1, 2017 •

edited

Loading

AllanWang Aug 1, 2017 •

edited

Loading