-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[interactive_media_ads] Adds support to set general SDK settings #9648
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant feature to configure the IMA SDK using a new ImaSettings
class. The changes are well-structured and span across the Dart API, platform interfaces, and native implementations for both Android and iOS. The addition of PigeonOverrides
for improved testability is a welcome enhancement. I've identified a couple of issues related to correctness and platform consistency, along with a suggestion to improve code maintainability. Overall, this is a solid contribution.
Future<void> setPlayerVersion(String? playerVersion) async { | ||
assert(playerVersion != null); | ||
final ImaSdkSettings settings = await nativeSettingsFuture; | ||
await settings.setPlayerVersion(playerVersion); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assertion will cause a runtime error if playerVersion
is null
. The method signature setPlayerVersion(String? playerVersion)
correctly indicates that null
is a valid value, and the underlying Pigeon-generated method also accepts a nullable String
.
Please remove the assertion to allow null
values to be passed to the native side, which aligns with the nullability of the parameter.
@override
Future<void> setPlayerVersion(String? playerVersion) async {
final ImaSdkSettings settings = await nativeSettingsFuture;
await settings.setPlayerVersion(playerVersion);
}
Future<void> setAutoPlayAdBreaks(bool autoPlayAdBreaks) async { | ||
final ImaSdkSettings settings = await nativeSettingsFuture; | ||
await settings.setAutoPlayAdBreaks(autoPlayAdBreaks); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many methods in this class follow the same pattern of awaiting nativeSettingsFuture
and then calling a single method on the resulting settings
object. This leads to some repetitive code.
To improve maintainability and reduce boilerplate, consider introducing a private helper method to encapsulate this pattern. For example:
Future<void> _withSettings(Future<void> Function(ImaSdkSettings) callback) async {
final ImaSdkSettings settings = await nativeSettingsFuture;
await callback(settings);
}
You could then refactor the methods like this:
@override
Future<void> setAutoPlayAdBreaks(bool autoPlayAdBreaks) {
return _withSettings(
(ImaSdkSettings settings) => settings.setAutoPlayAdBreaks(autoPlayAdBreaks),
);
}
|
||
@override | ||
Future<void> setSessionID(String? sessionID) { | ||
return nativeSettings.setSessionID(sessionID ?? ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When sessionID
is null
, it is being converted to an empty string (''
). However, the Android implementation passes null
through to the native side. The native iOS property sessionID
is also nullable (String?
), so it should be able to handle nil
.
To maintain consistency between platforms and align with the native SDK's nullability, it's better to pass sessionID
directly without the null-coalescing operator.
return nativeSettings.setSessionID(sessionID);
@@ -49,7 +49,7 @@ base class PlatformContentProgressProviderCreationParams { | |||
/// | |||
/// Provides updates required to enable triggering ads at configured cue points. | |||
abstract class PlatformContentProgressProvider { | |||
/// Creates a new [PlatformAdsManagerDelegate] | |||
/// Creates a new [PlatformAdsManagerDelegate]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
final AndroidImaSettings settings = switch (_androidParams.settings) { | ||
final AndroidImaSettings androidSettings => androidSettings, | ||
_ => AndroidImaSettings(_androidParams.settings.params), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
await settings.setSessionId(sessionID); | ||
} | ||
|
||
Future<ImaSdkSettings> _initSettings() async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it called initSettings
if it's creating a new settings object every time? The name implies initializing some shared state.
final ImaSdkSettings settings = | ||
await ImaSdkFactory.instance.createImaSdkSettings(); | ||
if (params.language case final String language) { | ||
unawaited(settings.setLanguage(language)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment about why this is unawaited?
return nativeSettings.setEnableBackgroundPlayback(enabled); | ||
} | ||
|
||
IMASettings _initSettings() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same naming question here.
Adds support for:
https://developers.google.com/interactive-media-ads/docs/sdks/android/client-side/api/reference/com/google/ads/interactivemedia/v3/api/ImaSdkSettings.html
https://developers.google.com/interactive-media-ads/docs/sdks/ios/client-side/reference/Classes/IMASettings.html
Pre-Review Checklist
[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assist
bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3