Implement ad caching and decouple the b lo c #89
Conversation
- Create a new StatelessWidget class AdPlaceholder - Implement EquatableMixin for state comparison - Add documentation explaining the purpose and usage of AdPlaceholder - Define a unique ID for each ad placeholder instance - Include this model in the ads library for feed item ad placeholders
- Create a singleton AdCacheService to cache native ads - Add methods to get, set, and clear cached ads - Implement logging for ad cache operations - Add a method to print the current state of the ad cache - Ensure proper disposal of AdMob native ad resources when clearing the cache
- Create self-contained AdLoaderWidget for loading and displaying native ads - Integrate AdCacheService for efficient ad caching and retrieval - Handle loading states, errors, and successful ad loads within the widget - Decouple ad loading from BLoC to improve scrolling performance in lists - Use hardcoded imageStyle for now, allowing for future configurability
- AdmobNativeAdWidget is now a StatefulWidget to properly manage the lifecycle of the native ad object. - Added logic to dispose of the native ad when the widget is removed from the tree or when the underlying ad object changes. - Implemented error handling for incorrect ad object types. - Updated documentation to reflect the new implementation and responsibilities of the widget.
- Removed redundant documentation above AdMobNativeAdWidget class - The documentation for this widget was duplicated, which is unnecessary and can lead to confusion - This change ensures the codebase adheres to better documentation practices
… FeedDecoratorService - Remove public `injectAds` method to prevent injecting loaded AdFeedItems - FeedDecoratorService now only injects stateless AdPlaceholder markers - Actual ad loading and management is handled by new AdLoaderWidget in UI layer - This change simplifies FeedDecoratorService, improves performance, and prevents crashes related to stateful native ad objects
- Replace ad injection with stateless ad placeholder injection - Remove public `injectAds` method and related ad loading responsibilities - Update class documentation
- Clear ad cache on full refresh, filter application, and filter clearance - Update feed pagination to use ad placeholders instead of ads - Refactor ad injection method name for clarity
- Replace AdFeedItem with AdPlaceholder to decouple ad loading from feed item generation - Introduce AdLoaderWidget to handle ad loading and display - Update imports to use AdService and AdPlaceholder instead of direct ad models - Modify HeadlinesFeedFetchRequested event to not include adThemeStyle
- Replace ad injection with ad placeholder injection for entity details and pagination - Clear cached ads when loading new entity details to ensure fresh ad content - Update method names in FeedDecoratorService to reflect ad placeholder focus
- Add support for displaying ads in the entity details page - Implement AdLoaderWidget to load and show ads - Update imports to include necessary ad-related classes and services
…h results and pagination - Replace `injectAds` with `injectAdPlaceholders` in search pagination and results - Clear previous ad cache for new searches - Update comments to reflect change from ads to ad placeholders
- Remove custom ad rendering logic - Integrate AdLoaderWidget for consistent ad display - Add AdService dependency - Update imports and handle deprecation of some packages
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @fulleni, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly refactors the application's ad display mechanism by introducing an ad caching service and decoupling ad loading logic from the BLoC layer. The changes aim to improve performance, prevent crashes related to native ad object lifecycle management, and streamline the process of displaying ads within various feeds.
Highlights
- Ad Caching Implementation: A new AdCacheService is introduced as a singleton to efficiently store and retrieve loaded native ad objects, reducing redundant network requests and improving scrolling fluidity.
- Decoupled Ad Loading: The BLoC layers (e.g., HeadlinesFeedBloc, EntityDetailsBloc, HeadlinesSearchBloc) no longer directly load or manage native ad objects. Instead, they now inject lightweight AdPlaceholder models into the feed.
- Dedicated Ad Loader Widget: A new AdLoaderWidget is responsible for handling the entire lifecycle of an ad slot in the UI. It checks the AdCacheService first and then loads a new ad if necessary, ensuring proper display and disposal.
- Improved Native Ad Disposal: The AdmobNativeAdWidget is converted to a StatefulWidget to correctly manage the dispose() method of native AdMob ad objects, preventing memory leaks and ensuring resources are released when ads are no longer needed.
- Strategic Ad Cache Clearing: The AdCacheService is strategically cleared in various BLoC events (e.g., feed refreshes, filter changes, new entity loads) to ensure that users always see fresh and relevant ads when the content context changes.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and positive architectural change by decoupling ad loading from the BLoC layer and implementing a caching mechanism. Using AdPlaceholder in the BLoC state and handling the ad lifecycle in a dedicated AdLoaderWidget is a great approach that improves maintainability, performance, and stability. The code is well-structured and the changes are applied consistently across different parts of the app.
My review includes a few suggestions to further enhance the implementation:
- Improving type safety in the
AdCacheService. - Preventing potential crashes in
AdLoaderWidgetby checking if the widget is mounted before updating its state. - Cleaning up unused parameters in
FeedDecoratorServiceto simplify the code.
Overall, this is a high-quality contribution.
| setState(() { | ||
| _loadedAd = adFeedItem.nativeAd; | ||
| _isLoading = false; | ||
| }); |
There was a problem hiding this comment.
The setState call here follows an await. If the widget is disposed while the ad is being fetched, this call will throw an exception. You should wrap setState calls that follow an await with a mounted check to prevent this. This advice also applies to the setState calls in the else and catch blocks of this try-catch.
if (mounted) {
setState(() {
_loadedAd = adFeedItem.nativeAd;
_isLoading = false;
});
}
Signed-off-by: fulleni <fulleni@hotmail.com>
Description
Type of Change