Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## 0.2.5

* Fixes the management of `BillingClient` connection.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This should include how the management is being fixed.

* Introduces `BillingClientManager`.

## 0.2.4+2

* Updates links for the merge of flutter/plugins into flutter/packages.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

export 'src/billing_client_wrappers/billing_client_manager.dart';
export 'src/billing_client_wrappers/billing_client_wrapper.dart';
export 'src/billing_client_wrappers/purchase_wrapper.dart';
export 'src/billing_client_wrappers/sku_details_wrapper.dart';
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:async';

import 'package:flutter/widgets.dart';

import 'billing_client_wrapper.dart';
import 'purchase_wrapper.dart';

/// Abstraction of result of [BillingClient] operation that includes
/// a [BillingResponse].
abstract class HasBillingResponse {
/// The status of the operation.
abstract final BillingResponse responseCode;
}

/// Utility class that manages a [BillingClient] connection.
///
/// Connection is initialized on creation of [BillingClientManager].
/// If [BillingClient] sends `onBillingServiceDisconnected` event or any
/// operation returns [BillingResponse.serviceDisconnected], connection is
/// re-initialized.
///
/// [BillingClient] instance is not exposed directly. It can be accessed via
/// [run] and [runRaw] methods that handle the connection management.
///
/// Consider calling [dispose] after the [BillingClient] is no longer needed.
class BillingClientManager {
/// Creates the [BillingClientManager].
///
/// Immediately initializes connection to the underlying [BillingClient].
BillingClientManager() {
_connect();
}

/// Stream of `onPurchasesUpdated` events from the [BillingClient].
///
/// This is a broadcast stream, so it can be listened to multiple times.
/// A "done" event will be sent after [dispose] is called.
late final Stream<PurchasesResultWrapper> purchasesUpdatedStream =
_purchasesUpdatedController.stream;

/// [BillingClient] instance managed by this [BillingClientManager].
///
/// In order to access the [BillingClient], consider using [run] and [runRaw]
/// methods.
@visibleForTesting
late final BillingClient client = BillingClient(_onPurchasesUpdated);

final StreamController<PurchasesResultWrapper> _purchasesUpdatedController =
StreamController<PurchasesResultWrapper>.broadcast();

bool _isConnecting = false;
bool _isDisposed = false;

// Initialized immediately in the constructor, so it's always safe to access.
late Future<void> _readyFuture;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This is optional, but a Completer<void> could be used instead of a Future<void>. This would make it so you would not have to depend on a late initializer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case this wouldn't make much difference - new Completer would always have to be created in _connect function anyway.


/// Executes the given [block] with access to the underlying [BillingClient].
///
/// If necessary, waits for the underlying [BillingClient] to connect.
/// If given [block] returns [BillingResponse.serviceDisconnected], it will
/// be transparently retried after the connection is restored. Because
/// of this, [block] may be called multiple times.
///
/// A response with [BillingResponse.serviceDisconnected] may be returned
/// in case of [dispose] being called during the operation.
///
/// See [runRaw] for operations that do not return a subclass
/// of [HasBillingResponse].
Future<R> run<R extends HasBillingResponse>(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this method name could be more descriptive. As in it could be runWhenClientIsReady or runOnClientAvailable. I think this would make it easier to understand when this method is being used in the code. Same goes for the method runRaw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also agree that run/runRaw is ambiguous. I don't think that runWhenClientIsReady, runOnClientAvailable would be good though. Main idea of BillingClientManager is to handle BillingClient connection transparently, so its user doesn't have to worry about client readiness/availability/retries. I suggest withClient and withClientNonRetryable instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about runWithClient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done.

Future<R> Function(BillingClient client) block,
Copy link
Contributor

Choose a reason for hiding this comment

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

This parameter could use a more descriptive name. Callback methods in Dart are typically given a name such as on<Action>. My assumption is that this could be called onBillingClientAvailable or onBillingClientReady.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that'd be a good choice, since function that's passed here isn't a callback, it's called in place (across an optional asynchronous gap). This is more similar to Flutter's setState with argument named just fn. I agree that block is a bit ambiguous, what do you think about action instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point. action works for me!

) async {
assert(_debugAssertNotDisposed());
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 running an assert for a method that runs an assert. I think you could just have:

_debugAssertNotDisposed();

await _readyFuture;
final R result = await block(client);
if (result.responseCode == BillingResponse.serviceDisconnected &&
!_isDisposed) {
await _connect();
return run(block);
} else {
return result;
}
}

/// Executes the given [block] with access to the underlying [BillingClient].
///
/// If necessary, waits for the underlying [BillingClient] to connect.
/// Designed only for operations that do not return a subclass
/// of [HasBillingResponse] (e.g. [BillingClient.isReady],
/// [BillingClient.isFeatureSupported]).
///
/// See [runRaw] for operations that return a subclass
/// of [HasBillingResponse].
Future<R> runRaw<R>(Future<R> Function(BillingClient client) block) async {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with this block parameter.

assert(_debugAssertNotDisposed());
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for here for removing the assert.

await _readyFuture;
return block(client);
}

/// Ends connection to the [BillingClient].
///
/// Consider calling [dispose] after you no longer need the [BillingClient]
/// API to free up the resources.
///
/// After calling [dispose] :
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
/// After calling [dispose] :
/// After calling [dispose]:

/// - Further connection attempts will not be made;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: To be consistent

Suggested change
/// - Further connection attempts will not be made;
/// - Further connection attempts will not be made.

/// - [purchasesUpdatedStream] will be closed;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
/// - [purchasesUpdatedStream] will be closed;
/// - [purchasesUpdatedStream] will be closed.

/// - Calls to [run] and [runRaw] will throw.
void dispose() {
assert(_debugAssertNotDisposed());
_isDisposed = true;
client.endConnection();
_purchasesUpdatedController.close();
}

// If disposed, does nothing.
// If currently connecting, waits for it to complete.
// Otherwise, starts a new connection.
Future<void> _connect() {
if (_isDisposed) {
return Future<void>.value();
}
if (_isConnecting) {
return _readyFuture;
}
_isConnecting = true;
_readyFuture = Future<void>.sync(() async {
await client.startConnection(onBillingServiceDisconnected: _connect);
_isConnecting = false;
});
return _readyFuture;
}

void _onPurchasesUpdated(PurchasesResultWrapper event) {
if (_isDisposed) {
return;
}
_purchasesUpdatedController.add(event);
}

bool _debugAssertNotDisposed() {
assert(() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why one would throw a FlutterError inside of an assert. It defeats the point of having an assert. Would it not be better to make it this:

assert(!_isDisposed, 'my error message');

if (_isDisposed) {
throw FlutterError(
'A BillingClientManager was used after being disposed.\n'
'Once you have called dispose() on a BillingClientManager, it can no longer be used.',
);
}
return true;
}());
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@ typedef PurchasesUpdatedListener = void Function(
/// `com.android.billingclient.api.BillingClient` API as much as possible, with
/// some minor changes to account for language differences. Callbacks have been
/// converted to futures where appropriate.
///
/// Connection to [BillingClient] may be lost at any time (see
/// `onBillingServiceDisconnected` param of [startConnection] and
/// [BillingResponse.serviceDisconnected]).
/// Consider using [BillingClientManager] that handles these disconnections
/// transparently.
class BillingClient {
/// Creates a billing client.
BillingClient(PurchasesUpdatedListener onPurchasesUpdated) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'package:flutter/foundation.dart';
import 'package:in_app_purchase_platform_interface/in_app_purchase_platform_interface.dart';
import 'package:json_annotation/json_annotation.dart';

import 'billing_client_manager.dart';
import 'billing_client_wrapper.dart';
import 'sku_details_wrapper.dart';

Expand Down Expand Up @@ -265,7 +266,7 @@ class PurchaseHistoryRecordWrapper {
@JsonSerializable()
@BillingResponseConverter()
@immutable
class PurchasesResultWrapper {
class PurchasesResultWrapper implements HasBillingResponse {
/// Creates a [PurchasesResultWrapper] with the given purchase result details.
const PurchasesResultWrapper(
{required this.responseCode,
Expand Down Expand Up @@ -300,6 +301,7 @@ class PurchasesResultWrapper {
///
/// This can represent either the status of the "query purchase history" half
/// of the operation and the "user made purchases" transaction itself.
@override
final BillingResponse responseCode;

/// The list of successful purchases made in this transaction.
Expand All @@ -316,7 +318,7 @@ class PurchasesResultWrapper {
@JsonSerializable()
@BillingResponseConverter()
@immutable
class PurchasesHistoryResult {
class PurchasesHistoryResult implements HasBillingResponse {
/// Creates a [PurchasesHistoryResult] with the provided history.
const PurchasesHistoryResult(
{required this.billingResult, required this.purchaseHistoryRecordList});
Expand All @@ -325,6 +327,9 @@ class PurchasesHistoryResult {
factory PurchasesHistoryResult.fromJson(Map<String, dynamic> map) =>
_$PurchasesHistoryResultFromJson(map);

@override
BillingResponse get responseCode => billingResult.responseCode;

@override
bool operator ==(Object other) {
if (identical(other, this)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import 'package:flutter/foundation.dart';
import 'package:json_annotation/json_annotation.dart';

import 'billing_client_manager.dart';
import 'billing_client_wrapper.dart';

// WARNING: Changes to `@JsonSerializable` classes need to be reflected in the
Expand Down Expand Up @@ -182,7 +183,7 @@ class SkuDetailsWrapper {
/// Returned by [BillingClient.querySkuDetails].
@JsonSerializable()
@immutable
class SkuDetailsResponseWrapper {
class SkuDetailsResponseWrapper implements HasBillingResponse {
/// Creates a [SkuDetailsResponseWrapper] with the given purchase details.
@visibleForTesting
const SkuDetailsResponseWrapper(
Expand All @@ -202,6 +203,9 @@ class SkuDetailsResponseWrapper {
@JsonKey(defaultValue: <SkuDetailsWrapper>[])
final List<SkuDetailsWrapper> skuDetailsList;

@override
BillingResponse get responseCode => billingResult.responseCode;

@override
bool operator ==(Object other) {
if (other.runtimeType != runtimeType) {
Expand All @@ -221,7 +225,7 @@ class SkuDetailsResponseWrapper {
@JsonSerializable()
@BillingResponseConverter()
@immutable
class BillingResultWrapper {
class BillingResultWrapper implements HasBillingResponse {
/// Constructs the object with [responseCode] and [debugMessage].
const BillingResultWrapper({required this.responseCode, this.debugMessage});

Expand All @@ -239,6 +243,7 @@ class BillingResultWrapper {
}

/// Response code returned in the Play Billing API calls.
@override
final BillingResponse responseCode;

/// Debug message returned in the Play Billing API calls.
Expand Down
Loading