Skip to content
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

feat: add custom AdEventListener to Manilo #133

Open
wants to merge 3 commits into
base: dev-v1
Choose a base branch
from

Conversation

loicsiret
Copy link

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

@eneim
Copy link
Owner

eneim commented Jan 8, 2021

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

@eneim
Copy link
Owner

eneim commented Jan 10, 2021

[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.)

@loicsiret
Copy link
Author

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.

@eneim
Copy link
Owner

eneim commented Jan 11, 2021

@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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants