nooralibutt / easy-ads

This repo has the integration of AdMob, AppLovin-Max, UnityAds, and Facebook ads.
https://pub.dev/packages/easy_ads_flutter
BSD 3-Clause "New" or "Revised" License
41 stars 20 forks source link

AdMob EasyBannerAd reloads ad on every rebuild #88

Open caseycrogers opened 6 months ago

caseycrogers commented 6 months ago
  1. Place an EasyBannerAd anywhere in the widget tree
  2. Trigger a rebuild (eg via hot reload)
  3. Note that the banner ad is reloaded

This is an especially big problem if you try to size your ad according to the current device size because keyboard open/close will trigger tons of rebuilds which will make the banner ad attempt to load ~10 times on every open/close. This was causing huge perf problems in prod and hurting my AdMob CPM before I figured out what was happening.

Here's a minimal repro to demonstrate the problem:

// This is just a test scenario to make it easy to test opening and closing the keyboard.
class AdTest extends StatefulWidget {
  const AdTest({super.key});

  @override
  State<AdTest> createState() => _AdTestState();
}

class _AdTestState extends State<AdTest> {
  final FocusNode focus = FocusNode();

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Material(
        child: Column(
          children: [
            Expanded(
              child: Center(
                child: Row(
                  children: [
                    Expanded(child: TextField(focusNode: focus)),
                    IconButton(
                      onPressed: () {
                        focus.unfocus();
                      },
                      icon: const Icon(Icons.close),
                    ),
                  ],
                ),
              ),
            ),
            // This widget creates a banner ad that will resize if the screen width changes.
            // Eg if the user changes phone orientation or is using a folding phone and fold/unfolds.
            const _SelfSizingBannerAd(),
          ],
        ),
      ),
    );
  }
}

class _SelfSizingBannerAd extends StatefulWidget {
  const _SelfSizingBannerAd();

  @override
  State<_SelfSizingBannerAd> createState() => _SelfSizingBannerAdState();
}

class _SelfSizingBannerAdState extends State<_SelfSizingBannerAd> {
  late Future<AdSize?> adSize = getAdSize();
  int? lastSeenWidth;

  @override
  void didChangeDependencies() {
    final int? newWidth = MediaQuery.maybeOf(context)?.size.width.truncate();
    if (newWidth != lastSeenWidth) {
      adSize = getAdSize();
    }
    super.didChangeDependencies();
  }

  @override
  Widget build(BuildContext context) {
    return FutureBuilder(
      future: adSize,
      builder: (context, adSizeSnap) {
        if (adSizeSnap.connectionState != ConnectionState.done) {
          return const CircularProgressIndicator();
        }
        print('Building with ad size: ${adSizeSnap.data}');
        return EasyBannerAd(adSize: adSizeSnap.data ?? AdSize.banner);
      },
    );
  }

  Future<AdSize?> getAdSize() async {
    print('Getting ad size from media query!');
    // This creates a dependency on media query which will trigger rebuilds
    // (but not reinitialization) on keyboard open/close.
    lastSeenWidth = MediaQuery.maybeOf(context)?.size.width.truncate();
    if (lastSeenWidth == null) {
      return null;
    }
    return AdSize.getCurrentOrientationAnchoredAdaptiveBannerAdSize(
      lastSeenWidth!,
    );
  }
}

Note that the banner ad disappears while the keyboard is moving and then reappears some time after-it attempts ~10 reloads during this time.

You can verify the problem by putting a breakpoint on this line and then tapping hot reload and noting that the load() function executes again: https://github.com/nooralibutt/easy-ads/blob/6a83bbc11895f65291338e8f3ae61e60d5462ad5/lib/src/easy_admob/easy_admob_banner_ad.dart#L70

The core problem here is that EasyAdMobBannerAd needs to be a stateful widget so that it is not reconstructed from scratch on every call to build. I can possibly take a stab at a PR in the next week or two if you do not have the time to tackle this, however I am also short time so I'm not sure if I actually can get to it. I also think the correct solution here is a fairly large rewrite. Here is what I would propose to properly solve this problem: Ads that are shown in the widget tree and stay there (ex: banner ads) vs ads that "pop up" (ex: interstitial) are fundamentally different and it doesn't really make sense to have them implement the same base class. Banner ads should be stateful widgets and load should be handled from initState and show should just be a build function. Note, perhaps the size-selection logic should even be built into the EasyBannerAd widget while we're at it?

Let me know what you think of this.

cihanst commented 6 months ago

yes, this is an important issue.