Closed ricardoalcantara closed 4 years ago
Thanks for pointing out! I'll fix that to allow null there. That particular error is a special one, as it doesn't originate from the Billing Library.
You need to call querySkuDetails
for an SKU before purchasing it.
Thanks for pointing out! I'll fix that to allow null there. That particular error is a special one, as it doesn't originate from the Billing Library.
You need to call
querySkuDetails
for an SKU before purchasing it.
Thanks for fixing it! I understand how this signal is emitting and I also know that I need to querySkuDetails
before, I mentioned it on my issue.
But I believe that sending null on an ErrorCode parameter is kind of strange and not very clear of what kind of error is that.
I also think that if you already have a method that return some result why use a signal that have two different purpose if null
is a internal error 'else' Google's error, and if later you need to signal two different internal errors how would I if
it (with string comparison)? That's why I suggested to use the result status = 1 because it makes more sense to me that status = Godot status and responseCode = Google status
That's why I made that whole explanation for, I just wanted to point that out too.
@ricardoalcantara I don't see a problem with null
. Remember that this is an error that can never happen in production (unless you publish an app without testing IAP once).
Never mind, I just wanted to help.
@ricardoalcantara you definitely helped (you pointed out this issue in the first place) :)
I just think introducing another special return format for one single error that can only occur in development makes error handling too complex. But I'd definitely implement something like your idea in case we add more errors that come from the plugin rather than the Billing Library.
@ricardoalcantara I'm starting to dislike making the error code nullable :sweat_smile: What do you think of printing a message and throwing an exception? This will crash the app but I think that's fine because it shouldn't happen in production and makes the developers aware that something is definitely wrong...
I'm starting to dislike making the error code nullable
I think the Engine doesn't support it.
What do you think of printing a message and throwing an exception? This will crash the app
I once saw Juan saying that the Engine should never crash and that they don't work with exceptions, I also think that crash the app for wrong flow not friendly.
I still think that change from emitting the signal to return the Dictionary a good solution.
public Dictionary purchase(String sku) {
if (!skuDetailsCache.containsKey(sku)) {
Dictionary returnValue = new Dictionary();
returnValue.put("status", 1); // FAILED = 1
returnValue.put("debug_message", "You must query the sku details and wait for the result before purchasing!");
return returnValue;
}
SkuDetails skuDetails = skuDetailsCache.get(sku);
BillingFlowParams purchaseParams = BillingFlowParams.newBuilder()
.setSkuDetails(skuDetails)
.build();
BillingResult result = billingClient.launchBillingFlow(getActivity(), purchaseParams);
Dictionary returnValue = new Dictionary();
if (result.getResponseCode() == BillingClient.BillingResponseCode.OK) {
returnValue.put("status", 0); // OK = 0
} else {
returnValue.put("status", 1); // FAILED = 1
returnValue.put("response_code", result.getResponseCode());
returnValue.put("debug_message", result.getDebugMessage());
}
return returnValue;
}
I think the Engine doesn't support it.
It doesn't support statically typed nullables. Variables can be of any type.
I once saw Juan saying that the Engine should never crash and that they don't work with exceptions, I also think that crash the app for wrong flow not friendly.
I agree with Juan that the Engine should never crash at runtime, but I think this is an exception, as this can possibly only happen when testing IAP. If this happens in production, the developer never tested IAP once, which, in my opinion, is not a case we should take care of. This particular error we are talking about ("You must query the sku details and wait for the result before purchasing!") is clearly a developer mistake. It's like running a GLES3 game on a GLES2-only device, it will also just crash. I'm also trying to avoid changing the current API for such a small problem so that people can upgrade seamlessly.
The absolutely best way would be an API which makes it impossible to produce that error, something like querySkuDetails
returns an SKU
object which has a purchase
method. But unfortunately, this is not possible with GDScript+Java at the moment (we cannot instantiate GDScript classes as far as I know).
@m4gr3d do you have any ideas?
@timoschwarzer
Okay.
@timoschwarzer
I'm also trying to avoid changing the current API for such a small problem so that people can upgrade seamlessly.
My suggestion won't break anything and won't change the API, actually people who are using the way it is may not be using this signal in that specific situation since it never worked!
@ricardoalcantara actually you're right. I don't know why but I thought purchase
was a void
all the time (which would have required an API change).
Would you like to make a PR for that?
@timoschwarzer Sure, I will make that change and create a PR for that. Thanks.
Godot version:
3.2.2.stable.mono.official
OS/device including version:
Android with GLES3
Issue description:
I am using this lib to implement IAP in my game, after a while testing I face this error:
C#
First I thought that it could be some C# issue, so I tested with GDScript and the same error happens:
GDScript
It happens when I try to purchase a SKU that I haven't queried before and it emitSignal with null parameter.
https://github.com/godotengine/godot-google-play-billing/blob/916385d3ca2dbbff493fb53272921b1ca8d71e65/godot-google-play-billing/src/main/java/org/godotengine/godot/plugin/googleplaybilling/GodotGooglePlayBilling.java#L178
it's registered to send Interger https://github.com/godotengine/godot-google-play-billing/blob/916385d3ca2dbbff493fb53272921b1ca8d71e65/godot-google-play-billing/src/main/java/org/godotengine/godot/plugin/googleplaybilling/GodotGooglePlayBilling.java#L230
This signal is also used when I try to purchase:
https://github.com/godotengine/godot-google-play-billing/blob/916385d3ca2dbbff493fb53272921b1ca8d71e65/godot-google-play-billing/src/main/java/org/godotengine/godot/plugin/googleplaybilling/GodotGooglePlayBilling.java#L205
the getResponseCode can be:
By google documentation: https://developer.android.com/reference/com/android/billingclient/api/BillingClient.BillingResponseCode
I can fix it and create a PR if it's confirmed a bug. But what could be a good solution?
1 - Create another signal for that purpose 2 - Use the signal but create a different errorCode from those in the google documentation (e.g -4 or 9) 3 -
public Dictionary purchase(String sku)
could return anotherstatus
withdebug_message
I prefer the 3 option: