Merge pull request #119 from flutter-news-app-full-source-code/Robust-Ad-Lifecycle-Management-and-Intelligent-Placement

Robust ad lifecycle management and intelligent placement

Author: fulleni

lib/ads/widgets/admob_inline_ad_widget.dart

@@ -55,9 +55,10 @@ class _AdmobInlineAdWidgetState extends State<AdmobInlineAdWidget> {
   @override
   void didUpdateWidget(covariant AdmobInlineAdWidget oldWidget) {
     super.didUpdateWidget(oldWidget);
-    // If the inlineAd object itself has changed (e.g., a new ad was loaded
-    // for the same placeholder ID), dispose the old ad and set the new one.
-    if (widget.inlineAd.id != oldWidget.inlineAd.id) {
+    // If the inlineAd object reference itself has changed, dispose the old ad
+    // and set the new one. This is a more robust check than just comparing IDs,
+    // as a new InlineAd instance might be created for the same logical ad slot.
+    if (widget.inlineAd != oldWidget.inlineAd) {
       _disposeCurrentAd(); // Dispose the old ad object
       _setAd();
     }
@@ -146,7 +147,13 @@ class _AdmobInlineAdWidgetState extends State<AdmobInlineAdWidget> {
     // lists to prevent "unbounded height" errors.
     return SizedBox(
       height: adHeight,
-      child: admob.AdWidget(ad: _ad! as admob.AdWithView), // Cast to AdWithView
+      // Use a ValueKey derived from the adObject's hashCode to force Flutter
+      // to create a new AdWidget instance if the underlying ad object changes.
+      // This prevents the "AdWidget is already in the Widget tree" error.
+      child: admob.AdWidget(
+        key: ValueKey(_ad!.hashCode),
+        ad: _ad! as admob.AdWithView,
+      ),
     );
   }
 }

lib/ads/widgets/feed_ad_loader_widget.dart

@@ -62,8 +62,7 @@ class _FeedAdLoaderWidgetState extends State<FeedAdLoaderWidget> {
   bool _hasError = false;
   final Logger _logger = Logger('FeedAdLoaderWidget');
   late final InlineAdCacheService _adCacheService;
-  late final AdService
-  _adService; // AdService will be accessed via _adCacheService
+  late final AdService _adService;
 
   /// Completer to manage the lifecycle of the ad loading future.
   /// This helps in cancelling pending operations if the widget is disposed
@@ -106,11 +105,13 @@ class _FeedAdLoaderWidgetState extends State<FeedAdLoaderWidget> {
       // Immediately set the widget to a loading state to prevent UI flicker.
       // This ensures a smooth transition from the old ad (or no ad) to the
       // loading indicator for the new ad.
-      setState(() {
-        _loadedAd = null;
-        _isLoading = true;
-        _hasError = false;
-      });
+      if (mounted) {
+        setState(() {
+          _loadedAd = null;
+          _isLoading = true;
+          _hasError = false;
+        });
+      }
       _loadAd();
     }
   }
@@ -136,15 +137,24 @@ class _FeedAdLoaderWidgetState extends State<FeedAdLoaderWidget> {
   /// If found, it uses the cached ad. Otherwise, it requests a new inline ad
   /// from the [AdService] using `getFeedAd` and stores it in the cache
   /// upon success.
+  ///
+  /// It also includes defensive checks (`mounted`) to prevent `setState` calls
+  /// on disposed widgets and ensures the `_loadAdCompleter` is always completed
+  /// to prevent `StateError`s.
   Future<void> _loadAd() async {
     // Initialize a new completer for this loading operation.
     _loadAdCompleter = Completer<void>();
 
-    // Ensure the widget is still mounted before calling setState.
+    // Ensure the widget is still mounted before proceeding.
     // This prevents the "setState() called after dispose()" error
     // if the widget is removed from the tree while the async operation
     // is still in progress.
-    if (!mounted) return;
+    if (!mounted) {
+      if (_loadAdCompleter?.isCompleted == false) {
+        _loadAdCompleter!.complete();
+      }
+      return;
+    }
 
     // Attempt to retrieve the ad from the cache first.
     final cachedAd = _adCacheService.getAd(widget.adPlaceholder.id);
@@ -154,11 +164,12 @@ class _FeedAdLoaderWidgetState extends State<FeedAdLoaderWidget> {
         'Using cached ad for feed placeholder ID: ${widget.adPlaceholder.id}',
       );
       // Ensure the widget is still mounted before calling setState.
-      if (!mounted) return;
-      setState(() {
-        _loadedAd = cachedAd;
-        _isLoading = false;
-      });
+      if (mounted) {
+        setState(() {
+          _loadedAd = cachedAd;
+          _isLoading = false;
+        });
+      }
       // Complete the completer only if it hasn't been completed already.
       if (_loadAdCompleter?.isCompleted == false) {
         _loadAdCompleter!.complete();
@@ -178,11 +189,12 @@ class _FeedAdLoaderWidgetState extends State<FeedAdLoaderWidget> {
           'Ad placeholder ID ${widget.adPlaceholder.id} has no adIdentifier. '
           'Cannot load ad.',
         );
-        if (!mounted) return;
-        setState(() {
-          _hasError = true;
-          _isLoading = false;
-        });
+        if (mounted) {
+          setState(() {
+            _hasError = true;
+            _isLoading = false;
+          });
+        }
         // Complete the completer normally, indicating that loading finished
         // but no ad was available. This prevents crashes.
         if (_loadAdCompleter?.isCompleted == false) {
@@ -214,11 +226,12 @@ class _FeedAdLoaderWidgetState extends State<FeedAdLoaderWidget> {
         // Store the newly loaded ad in the cache.
         _adCacheService.setAd(widget.adPlaceholder.id, loadedAd);
         // Ensure the widget is still mounted before calling setState.
-        if (!mounted) return;
-        setState(() {
-          _loadedAd = loadedAd;
-          _isLoading = false;
-        });
+        if (mounted) {
+          setState(() {
+            _loadedAd = loadedAd;
+            _isLoading = false;
+          });
+        }
         // Complete the completer only if it hasn't been completed already.
         if (_loadAdCompleter?.isCompleted == false) {
           _loadAdCompleter!.complete();
@@ -229,11 +242,12 @@ class _FeedAdLoaderWidgetState extends State<FeedAdLoaderWidget> {
           'No ad returned.',
         );
         // Ensure the widget is still mounted before calling setState.
-        if (!mounted) return;
-        setState(() {
-          _hasError = true;
-          _isLoading = false;
-        });
+        if (mounted) {
+          setState(() {
+            _hasError = true;
+            _isLoading = false;
+          });
+        }
         // Complete the completer normally, indicating that loading finished
         // but no ad was available. This prevents crashes.
         if (_loadAdCompleter?.isCompleted == false) {
@@ -247,11 +261,12 @@ class _FeedAdLoaderWidgetState extends State<FeedAdLoaderWidget> {
         s,
       );
       // Ensure the widget is still mounted before calling setState.
-      if (!mounted) return;
-      setState(() {
-        _hasError = true;
-        _isLoading = false;
-      });
+      if (mounted) {
+        setState(() {
+          _hasError = true;
+          _isLoading = false;
+        });
+      }
       // Complete the completer normally, indicating that loading finished
       // but an error occurred. This prevents crashes.
       if (_loadAdCompleter?.isCompleted == false) {

lib/headlines-feed/view/headlines_feed_page.dart

@@ -342,6 +342,7 @@ class _HeadlinesFeedPageState extends State<HeadlinesFeedPage> {
                     }
 
                     return FeedAdLoaderWidget(
+                      key: ValueKey(item.id), // Add a unique key for AdPlaceholder
                       adPlaceholder: item,
                       adThemeStyle: AdThemeStyle.fromTheme(Theme.of(context)),
                       adConfig: adConfig,

lib/shared/services/feed_decorator_service.dart

@@ -392,8 +392,8 @@ class FeedDecoratorService {
   /// This method ensures that ad placeholders for *inline* ads (native and banner)
   /// are placed according to the `adPlacementInterval` (initial buffer before
   /// the first ad) and `adFrequency` (subsequent ad spacing). It correctly
-  /// accounts for content items only, ignoring previously injected decorators
-  /// when calculating placement. Interstitial ads are not handled here.
+  /// accounts for content items and decorators, ignoring previously injected
+  /// ad placeholders when calculating placement.
   ///
   /// [feedItems]: The list of feed items (headlines, other decorators)
   ///   to inject ad placeholders into.
@@ -452,8 +452,8 @@ class FeedDecoratorService {
 
     final result = <FeedItem>[];
     // This counter tracks the absolute number of *content items* (headlines,
-    // topics, sources, countries) processed so far, including those from
-    // previous pages. This is key for accurate ad placement.
+    // topics, sources, countries, and decorators) processed so far, including
+    // those from previous pages. This is key for accurate ad placement.
     var currentContentItemCount = processedContentItemCount;
 
     // Get the primary ad platform and its identifiers
@@ -475,12 +475,9 @@ class FeedDecoratorService {
       result.add(item);
 
       // Only increment the content item counter if the current item is
-      // a primary content type (not an ad or a decorator).
-      // This ensures ad placement is based purely on content density.
-      if (item is Headline ||
-          item is Topic ||
-          item is Source ||
-          item is Country) {
+      // a primary content type or a decorator (not an ad placeholder).
+      // This ensures ad placement is based purely on content/decorator density.
+      if (item is! AdPlaceholder) {
         currentContentItemCount++;
       }