googleads / googleads-mobile-flutter

A Flutter plugin for the Google Mobile Ads SDK
Apache License 2.0
338 stars 277 forks source link

[Feature Request] Provide a Dart Idiomatic Async API #890

Open caseycrogers opened 1 year ago

caseycrogers commented 1 year ago

[REQUIRED] Step 2: Describe the problem

The entire google_mobile_ads API is built with callback APIs. These are bug prone and very non-idiomatic for Flutter. Really non-idiomatic in any language post ~2005. This means each dev more or less needs to write a wrapper around the current API to convert it to Flutter idiomatic futures.

Here is an example of what the API would look like using Flutter/Dart idioms:

// Future allows the dev to use `FutureBuilder` instead of customized stateful logic.
Future<RewardedAd> ad = RewardedAd.load();

// Now we don't have to navigate several different callbacks.
// The stream will complete with either an earned or 
// dismissed event to indicate the outcome of showing the
// ad.
Stream<RewardedEvent> result = (await ad).show();

// Class that signals events while the ad is up.
// Subclasses could include things like:
//    RewardEarned(Reward reward);
//    AdDismissed(DismissDetails details);
abstract class RewardedAdEvent { ... }

However even this isn't very Flutter idiomatic. It really isn't even going far enough. The ideal would be a widget using the controller pattern roughly as follows (see ScrollController and Scrollable for full Flutter idiomatic examples of the controller pattern):

// Wrap a widget subtree in this to allow it to show ads.
class RewardedAdView extends StatefulWidget {
  static RewardedAdController of(BuildContext context) { ... }

  // Can be passed in the constructor or makes a default one.
  RewardedAdController controller;
  ...
}

class _RewardedAdView extends State<RewardedAdView> {
  Widget build(BuildContext context() {
    // Wraps child in a provider so that the subtree can call `.of`.
    return _RewardedAdControllerProvider(controller: widget.controller, ...);
  }
}

class _RewardedAdControllerProvider extends InheritedWidget {
  RewardedAdController controller;
  ...
}

// Manages current ad state.
class RewardedAdController  with ChangeNotifier {
  RewardedAd? ad;

  // Sets `ad`. Could possibly be private and just called
  // by the view class's `initState` method.
  Future<void> load() { ... }

  // Show the ad and get the results stream.
  Stream<RewardedAdEvent> show() async* { ... };
}

// Somewhere in a build function.
return MyButton(
  child: Text('Oh yeah! Show me an ad, I love ads!'),
  onPressed: () {
    final result = RewardedAdView.of(context).show();
    ...
  },
);
huycozy commented 1 year ago

I think this is a wide scope issue and may need a lot of codebase refactoring, not sure about the feasibility and amount of effort required for now. So, I'm labeling this with a single label for more information from others.

mulderpf commented 1 year ago

This would be a welcome change!!! We already experienced a huge amount of refactoring from the 0. versions of the plugin until today, so anything that is more like what we are used to using everyday, will likely cause a lot less issues. The way to use the plugin is a bit weird and doesn't feel like the rest of my code.

caseycrogers commented 1 year ago

Any news here? The existing API is really hard to use.

caseycrogers commented 1 year ago

I'm now adding Banner Ads to my app and am reminded how hilariously hard to use the AdMob APIs are: https://codelabs.developers.google.com/codelabs/admob-ads-in-flutter#6

@override
void initState() {
  ...

  // Why are we creating an instance if we're not going to hold a reference to it??
  BannerAd(
    adUnitId: AdHelper.bannerAdUnitId,
    request: AdRequest(),
    size: AdSize.banner,
    // Why is a simple callback it's own class??????
    listener: BannerAdListener(
      onAdLoaded: (ad) {
        setState(() {
          // WHHHHHYYYYYY do we have to do type casting here?????
          _bannerAd = ad as BannerAd;
        });
      },
      onAdFailedToLoad: (ad, err) {
        print('Failed to load a banner ad: ${err.message}');
        ad.dispose();
      },
    ),
  ).load();
}

This feels like 2010s "Enterprise grade" Java or something. This is really painful. I hope you guys fix it.

malandr2 commented 12 months ago

@caseycrogers this is something we are actively looking into but no immediate news to share. However, comments like these show the importance of this feature request. Thanks

rajabilal555 commented 5 months ago

I have done this using simple completers. If someone wants inspiration for using this api. Maybe these methods can be a part of the sdk one day...

Future<RewardedInterstitialAd> _populateRewardedInterstitialAd({
    required String adUnitId,
    required VoidCallback onAdDismissedFullScreenContent,
    required VoidCallback onAdShowedFullScreenContent,
  }) async {
    try {
      final adCompleter = Completer<Ad?>();
      await RewardedInterstitialAd.load(
        adUnitId: adUnitId,
        request: const AdRequest(),
        rewardedInterstitialAdLoadCallback: RewardedInterstitialAdLoadCallback(
          onAdLoaded: (ad) {
            ad.fullScreenContentCallback = FullScreenContentCallback(
              onAdShowedFullScreenContent: (ad) {
                onAdShowedFullScreenContent();
              },
              onAdDismissedFullScreenContent: (ad) {
                onAdDismissedFullScreenContent();
              },
            );

            adCompleter.complete(ad);
          },
          onAdFailedToLoad: adCompleter.completeError,
        ),
      );

      final rewardedInterstitial = await adCompleter.future;
      if (rewardedInterstitial == null) {
        throw const AdsClientException('Rewarded Interstitial Ad was null');
      }
      return rewardedInterstitial as RewardedInterstitialAd;
    } catch (error, stackTrace) {
      Error.throwWithStackTrace(AdsClientException(error), stackTrace);
    }
  }