[interactive_media_ads] Adds initial Android implementation #6733

bparrishMines · 2024-05-15T00:14:41Z

Android implementation for flutter/flutter#134228

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

bparrishMines · 2024-05-23T16:02:43Z

packages/interactive_media_ads/android/build.gradle

+
+        // org.jetbrains.kotlin:kotlin-bom artifact purpose is to align kotlin stdlib and related code versions.
+        // See: https://youtrack.jetbrains.com/issue/KT-55297/kotlin-stdlib-should-declare-constraints-on-kotlin-stdlib-jdk8-and-kotlin-stdlib-jdk7
+        implementation(platform("org.jetbrains.kotlin:kotlin-bom:1.8.10"))


This is the same line added to camera_android_camerax: https://github.com/flutter/packages/blob/main/packages/camera/camera_android_camerax/android/build.gradle#L77

This fixed the build failure when adding the 'com.google.ads.interactivemedia.v3:interactivemedia:3.33.0' dependency.

bparrishMines · 2024-05-23T16:05:08Z

..._ads/android/src/main/kotlin/dev/flutter/packages/interactive_media_ads/ProxyApiRegistrar.kt

+  // of waiting for the main thread to run it.
+  internal open fun runOnMainThread(callback: Runnable) {
+    Handler(Looper.getMainLooper()).post { callback.run() }
+  }


This was added to the registrar to make it easier to handle and test callback methods on the correct thread.

bparrishMines · 2024-05-23T16:08:34Z

.../android/src/main/kotlin/dev/flutter/packages/interactive_media_ads/VideoAdPlayerProxyApi.kt

+  override fun setVolume(pigeon_instance: VideoAdPlayer, value: Long) {
+    (pigeon_instance as VideoAdPlayerImpl).savedVolume = value.toInt()
+  }
+
+  override fun setAdProgress(pigeon_instance: VideoAdPlayer, progress: VideoProgressUpdate) {
+    (pigeon_instance as VideoAdPlayerImpl).savedAdProgress = progress
+  }


Custom methods added since VideoAdPlayer.getVolume and VideoAdPlayer.getAdProgress require a synchronous callback method.

I think it's worth adding this as a code comment to these methods. It would be good to have it clarify why it's not calling api.foo in addition to saving the value.

packages/interactive_media_ads/lib/src/android/android_ad_display_container.dart

stuartmorgan

(I only briefly skimmed the tests.)

stuartmorgan · 2024-05-28T19:16:07Z

...ain/kotlin/dev/flutter/packages/interactive_media_ads/GeneratedInteractiveMediaAdsLibrary.kt

Can't we use InteractiveMediaAdsLibrary.g.kt? I thought it was only Java that prevented using the extra suffix.

Good catch. I forgot that we only did that because of the limitations of Java and Obj-c.

stuartmorgan · 2024-05-29T18:08:26Z

...ia_ads/android/src/main/kotlin/dev/flutter/packages/interactive_media_ads/AdErrorProxyApi.kt

+      AdError.AdErrorCode.INVALID_ARGUMENTS -> AdErrorCode.INVALID_ARGUMENTS
+      AdError.AdErrorCode.PLAYLIST_NO_CONTENT_TRACKING -> AdErrorCode.PLAYLIST_NO_CONTENT_TRACKING
+      AdError.AdErrorCode.UNEXPECTED_ADS_LOADED_EVENT -> AdErrorCode.UNEXPECTED_ADS_LOADED_EVENT
+    }


It looks like this doesn't handle the case where a new error code is added upstream. It seems like it would probably even fail to compile if that happened?

My intention was for this to break compilation when a new enum is added. Since this uses a pinned version of the IMA SDK, we can add a new enum to the code generation whenever we bump the version.

We add an unknown enum in the webview_flutter implementations, but I assumed we did that because it uses a system library and not a pinned SDK.

Since this uses a pinned version of the IMA SDK

It doesn't really though, because of optimistic upgrades. Because of the way Gradle works, unless we want to make the entire build fail on conflicts (which would be a large hammer) we don't actually have any control over whether we get the version listed in build.gradle.

...d/src/main/kotlin/dev/flutter/packages/interactive_media_ads/BaseDisplayContainerProxyApi.kt

stuartmorgan · 2024-05-29T18:17:15Z

.../android/src/main/kotlin/dev/flutter/packages/interactive_media_ads/VideoAdPlayerProxyApi.kt

+  override fun setVolume(pigeon_instance: VideoAdPlayer, value: Long) {
+    (pigeon_instance as VideoAdPlayerImpl).savedVolume = value.toInt()
+  }
+
+  override fun setAdProgress(pigeon_instance: VideoAdPlayer, progress: VideoProgressUpdate) {
+    (pigeon_instance as VideoAdPlayerImpl).savedAdProgress = progress
+  }


I think it's worth adding this as a code comment to these methods. It would be good to have it clarify why it's not calling api.foo in addition to saving the value.

packages/interactive_media_ads/lib/interactive_media_ads.dart

packages/interactive_media_ads/lib/src/android/android_ad_display_container.dart

stuartmorgan · 2024-05-29T18:42:51Z

packages/interactive_media_ads/lib/src/android/interactive_media_ads_proxy.dart

+    ) onError,
+    void Function(
+      VideoView,
+      MediaPlayer,


Wouldn't some of these be more readable without the trailing comma? Or would the inconsistency hurt readability?

Yea, it does make it more readable. I removed the trailing commas

stuartmorgan · 2024-05-29T18:45:00Z

packages/interactive_media_ads/pigeons/interactive_media_ads_android.dart

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// // ignore_for_file: avoid_unused_constructor_parameters


Why is everything in this file commented out?

This uses features from #6371, so it would break tests without it commented out. But I'll go ahead and uncomment it for now so it can be reviewed while I still wait for #6371 to land.

stuartmorgan

LGTM once the remaining comment about the enum fallback is resolved.

bparrishMines added 30 commits April 26, 2024 14:35

get libraries to generate

9f02730

add a bunch of classes

5f767fb

some more videview stuff

b3317a0

some display implementation for android

631c1e0

add the minimum required wrappings

35a4445

fix build error

8ac0311

implement other platform classes

bc4d770

start creating an implementation

1f74ead

move interfaces to bottom and format

4320f50

fix callback events

e85027d

remove prints

6739d7f

fix removeStrongReference error

e803962

Merge branch 'main' of github.com:flutter/packages into ima_android

0d59a8e

also send time updates

6b22ebc

combine implementation files

957e2ba

fix lint warnings

d0be873

fix Android dartPluginClass

875d6be

fix missing params call

0122f6c

make tracking ad progress in ad display container

d0971db

Merge branch 'main' of github.com:flutter/packages into ima_android

6b8ee3a

add missing error codes

b1ace40

discard ad break

cc728d2

licenses

f345071

copyright header

90965aa

first test file

93c239b

working tests

c1b00f1

more working tests

dce1723

more tests

76af339

media player test

c2d6f67

Merge branch 'main' of github.com:flutter/packages into ima_android

9dde9d6

bparrishMines added 3 commits May 20, 2024 13:21

separate class implementations

3893efe

Merge branch 'main' of github.com:flutter/packages into ima_android

1c5fac7

doc improvements

a30c9fc

bparrishMines changed the title ~~Ima android~~ [interactive_media_ads] Initial Android implementation May 20, 2024

bparrishMines changed the title ~~[interactive_media_ads] Initial Android implementation~~ [interactive_media_ads] Adds initial Android implementation May 20, 2024

bparrishMines added 7 commits May 20, 2024 13:51

lower meta version to 1.12.0

8fa430b

move meta to 1.10.0

220e2f0

only null ad duration on stop and error

f55805f

stop any ad tracking before starting a new one

284098a

Merge branch 'main' of github.com:flutter/packages into ima_android

69e088c

Merge branch 'main' of github.com:flutter/packages into ima_android

48828e3

stop ad progress tracking on video completion

891f4d4

bparrishMines commented May 23, 2024

View reviewed changes

packages/interactive_media_ads/lib/src/android/android_ad_display_container.dart Show resolved Hide resolved

bparrishMines commented May 23, 2024

View reviewed changes

packages/interactive_media_ads/lib/src/android/android_ad_display_container.dart Show resolved Hide resolved

bparrishMines requested a review from stuartmorgan May 23, 2024 19:12

bparrishMines marked this pull request as ready for review May 23, 2024 19:13

stuartmorgan reviewed May 29, 2024

View reviewed changes

bparrishMines added 7 commits May 30, 2024 12:37

change generated kotlin library name

9ada3ef

docs for kotlin classes

84d1ff5

Merge branch 'main' of github.com:flutter/packages into ima_android

1abcabc

variable docs

b4b2152

beef up docs and some formatting

519c68e

Merge branch 'main' of github.com:flutter/packages into ima_android

f7d25c6

uncomment pigeon file

8923d5f

bparrishMines requested a review from stuartmorgan June 2, 2024 23:32

stuartmorgan approved these changes Jun 3, 2024

View reviewed changes

stuartmorgan added the triage-android Should be looked at in Android triage label Jun 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[interactive_media_ads] Adds initial Android implementation #6733

[interactive_media_ads] Adds initial Android implementation #6733

bparrishMines commented May 15, 2024 •

edited

bparrishMines May 23, 2024 •

edited

bparrishMines May 23, 2024

bparrishMines May 23, 2024

stuartmorgan May 29, 2024

stuartmorgan left a comment •

edited

stuartmorgan May 28, 2024

bparrishMines Jun 1, 2024

stuartmorgan May 29, 2024

bparrishMines Jun 1, 2024

stuartmorgan Jun 3, 2024

stuartmorgan May 29, 2024

stuartmorgan May 29, 2024

bparrishMines Jun 1, 2024

stuartmorgan May 29, 2024

bparrishMines Jun 1, 2024

stuartmorgan left a comment

[interactive_media_ads] Adds initial Android implementation #6733

Are you sure you want to change the base?

[interactive_media_ads] Adds initial Android implementation #6733

Conversation

bparrishMines commented May 15, 2024 • edited

Pre-launch Checklist

bparrishMines May 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

bparrishMines commented May 15, 2024 •

edited

bparrishMines May 23, 2024 •

edited

stuartmorgan left a comment •

edited