Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert startProductRequest(), finishTransaction(), restoreTransactions(), presentCodeRedemptionSheet() to pigeon #6032

Merged
merged 17 commits into from
Feb 13, 2024

Conversation

LouiseHsu
Copy link
Contributor

@LouiseHsu LouiseHsu commented Feb 2, 2024

Part 2 of flutter/flutter#117910

This PR converts startProductRequest(), finishTransaction(), restoreTransactions(), presentCodeRedemptionSheet() to pigeon, as well as add all remaining converts to and from pigeons for SK objects.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@LouiseHsu LouiseHsu changed the title all dart pigeon types and converters created for startProductRequest Converts startProductRequest(), finishTransaction(), restoreTransactions(), presentCodeRedemptionSheet() to pigeon Feb 8, 2024
@LouiseHsu LouiseHsu changed the title Converts startProductRequest(), finishTransaction(), restoreTransactions(), presentCodeRedemptionSheet() to pigeon Convert startProductRequest(), finishTransaction(), restoreTransactions(), presentCodeRedemptionSheet() to pigeon Feb 8, 2024
@LouiseHsu LouiseHsu marked this pull request as ready for review February 8, 2024 21:56
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, the only major thing is the missing short circuiting and potentially missing tests for all these extra converters.


NSArray<SKProductDiscount *> *skProductDiscounts = product.discounts;
NSMutableArray<SKProductDiscountMessage *> *pigeonProductDiscounts =
[[NSMutableArray alloc] init];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: you know the size here. It's more efficient to create the array of the proper size.

Suggested change
[[NSMutableArray alloc] init];
[NSMutableArray arrayWithCapacity: skProductDiscounts.count];

return nil;
}
NSArray<SKProduct *> *skProducts = productsResponse.products;
NSMutableArray<SKProductMessage *> *pigeonProducts = [[NSMutableArray alloc] init];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: same here

Suggested change
NSMutableArray<SKProductMessage *> *pigeonProducts = [[NSMutableArray alloc] init];
NSMutableArray<SKProductMessage *> *pigeonProducts = [NSMutableArray arrayWithCapacity: skProducts.count];

result([FlutterError errorWithCode:@"storekit_getproductrequest_platform_error"
message:error.localizedDescription
details:error.description]);
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This return got lost. You'll want to short circuit any of the following logic. In order to do that you'll have to call the completion handler then return.

message:@"Failed to get SKProductResponse in startRequest "
@"call. Error occured on iOS platform"
details:call.arguments]);
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

[self.paymentQueueHandler presentCodeRedemptionSheet];
result(nil);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just want to call out how these calls have disappears now. In the past, failing to call these created bugs, you've eliminated that risk now 👍

}

@visibleForTesting

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@@ -383,6 +550,53 @@ class SKProductWrapper {
subscriptionPeriod,
introductoryPrice,
discounts);

/// Convert from [SKProductMessage] to [SKProductWrapper]
static SKProductWrapper convertFromPigeon(SKProductMessage msg) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No action required: These are all a pain. I guess if pigeon had been used from day one, the generated classes could have been been used. We also talked about letting pigeon use existing classes too. That would have really helped here.


enum SKSubscriptionPeriodUnitMessage {
day,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove newlines please


@override
void presentCodeRedemptionSheet() {
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return;

}

/// Convert from [SKProductWrapper] to [SKProductMessage]
static SKProductMessage convertToPigeon(SKProductWrapper wrapper) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do all of these have tests?

@@ -470,7 +470,7 @@ + (nullable SKProductMessage *)convertProductToPigeon:(nullable SKProduct *)prod

NSArray<SKProductDiscount *> *skProductDiscounts = product.discounts;
NSMutableArray<SKProductDiscountMessage *> *pigeonProductDiscounts =
[[NSMutableArray alloc] init];
[[NSMutableArray arrayWithCapacity:skProductDiscounts.count] init];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't want to call init when using arrayWithCapacity:, that's already taken care for you. You just need that when you are using alloc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh oopsies

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Don't forget to manually test it with the example app before merging.

Copy link
Contributor

@hellohuanlin hellohuanlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just briefly skimmed through since Aaron already reviewed it

@@ -307,6 +307,10 @@ + (nullable SKPaymentTransactionMessage *)convertTransactionToPigeon:
}

+ (nullable SKErrorMessage *)convertSKErrorToPigeon:(NSError *)error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: mark error as nullable

[self presentCodeRedemptionSheet:call result:result];
#endif
} else if ([@"-[InAppPurchasePlugin retrieveReceiptData:result:]" isEqualToString:call.method]) {
if ([@"-[InAppPurchasePlugin retrieveReceiptData:result:]" isEqualToString:call.method]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is probably not related to your PR - is this format iOS only? It's very objc-specific. do we intend to change that after migrating to swift?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this entire code block/fcn will be removed after the pigeon migration so it should be fine

details:e.description]);
*error = [FlutterError errorWithCode:@"storekit_finish_transaction_exception"
message:e.name
details:e.description];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is this error used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all generated pigeon functions have like an extra "error" param, I think it gets passed back to the dart side automatically

@gaaclarke
Copy link
Member

I'm not sure why that CI step keeps failing, it's an infra failure so it's likely not caused by this PR. You can try updating your branch by merging in main or rebasing against main.

@LouiseHsu LouiseHsu merged commit 9385bbb into flutter:main Feb 13, 2024
78 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 13, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 13, 2024
flutter/packages@0a69259...9385bbb

2024-02-13 louisehsu@google.com Convert startProductRequest(), finishTransaction(), restoreTransactions(), presentCodeRedemptionSheet() to pigeon (flutter/packages#6032)
2024-02-13 49699333+dependabot[bot]@users.noreply.github.com [in_app_pur]: Bump org.json:json from 20231013 to 20240205 in /packages/in_app_purchase/in_app_purchase/example/android/app (flutter/packages#6096)
2024-02-12 stuartmorgan@google.com [local_auth] Rename iOS classes (flutter/packages#6108)
2024-02-12 jakubwalusiak@gmail.com [video_player_android] Handle BehindLiveWindowException (flutter/packages#5869)
2024-02-12 reidbaker@google.com [in_app_purchase] Add alternative billing apis for android (flutter/packages#6056)
2024-02-12 stuartmorgan@google.com [webview_flutter] Update compileSdk to 34 (flutter/packages#6106)
2024-02-12 37270954+foxtrotravi@users.noreply.github.com [cupertino_icons] Add example to cupertino icons (flutter/packages#5312)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
…ns(), presentCodeRedemptionSheet() to pigeon (flutter#6032)

Part 2 of flutter/flutter#117910

This PR converts startProductRequest(), finishTransaction(),
restoreTransactions(), presentCodeRedemptionSheet() to pigeon, as well
as add all remaining converts to and from pigeons for SK objects.

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] All existing and new tests are passing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants