kohii icon indicating copy to clipboard operation
kohii copied to clipboard

feat: add custom AdEventListener to Manilo

Open loicsiret opened this issue 4 years ago • 4 comments

This PR will allow user to register/unregister AdEventListener on a Manilo instance.

loicsiret avatar Jan 07 '21 15:01 loicsiret

Thanks for the PR, will review this soon this weekend.

eneim avatar Jan 08 '21 02:01 eneim

[Q] @loicsiret My current concern is the client implementing the AdEventListener to the Activity/Fragment or something that has a lifecycle, and register it as an AdEventListener to the Manilo instance (whose lifecycle is equal to the Application).

An easy way to address this is to put a comment to addListener telling that the client must call removeListener properly. A fairly complicated way but safer is to require a LifecycleOwner in the call to addListener, and maybe implement an automatic removal mechanism like how the LiveData is doing. But it sounds to be overkill. What do you think?

(Ask this because, from the library's point of view, one of the responsibility is to make people's lives easier by automating things :D, but on the other hand I don't want to put any tech debt on the maintainer's shoulder if it can be solved by well-communication.)

eneim avatar Jan 10 '21 13:01 eneim

I totally understand and I agree with it. If you add a listener as an user of the Lib it is your responsibility to remove it properly. An another way to do it, would be Manilo exposing a Flow and push all events in that flow. So no responsibility to the Lib to manage the listener. I confess add/remove listener was the simplest (laziest ?) Way to do it.

loicsiret avatar Jan 10 '21 16:01 loicsiret

@loicsiret Sorry for being late.

My current thinking progress:

  • As the AdEventListener is added to the ImaAdsLoader.Builder instance and this listener will then be added to the com.google.ads.interactivemedia.v3.api.AdsManager instance, if we reuse a Builder instance for multiple Bridge (which we should), one listener might be responded to multiple com.google.ads.interactivemedia.v3.api.AdsManager instances, which might cause the callback to be confusing (considering having multiple instances of the AdsManager is possible ...). Currently, the Manilo instance implements the Listener for the debugging purpose only.
  • I wanted to have a way to register the AdEventListener so I want to have this PR be completed. Let me hang this PR for a while and we can discuss a feasible way (as long as you still have the interest of course).

eneim avatar Jan 11 '21 11:01 eneim