larsacus / LARSAdController

Lightweight ad mediation for iOS to properly manage multiple ad networks dynamically including iAd and Google ads.
http://theonlylars.com/blog/2013/01/10/stupid-easy-ads-with-larsadcontroller-3-dot-0/
MIT License
269 stars 60 forks source link

Racing condition -> black background instead of ad #60

Closed nickbit closed 10 years ago

nickbit commented 10 years ago

It seems that if an adapter view has not been added yet then the animation fails and a black background appears.

Adding the line

      [self.clippingContainer addSubview:adapter.bannerView];

in the code below, seems to fix the problem.

Lars, can you have a look?

- (void)animateBannerForAdapterVisible:(id <TOLAdAdapter>)adapter withCompletion:(void(^)(void))completion{
  NSLog(@"In animateBannerForAdapterVisible");

    [adapter layoutBannerForInterfaceOrientation:self.currentOrientation];

    if ([self.clippingContainer.subviews containsObject:adapter.bannerView] == NO) {
        //configure initial state for banner view off-screen
      // NB patch
      [self.clippingContainer addSubview:adapter.bannerView];
      // NB patch
        adapter.bannerView.frame = [self offScreenBannerFrameForAdapter:adapter
                                              presentationAnimationType:self.presentationType];
    }

   ...
larsacus commented 10 years ago

This sounds like some other problem than just calling addSubview: every time it gets animated. The only place that the subview gets added to the clipping container is when the ad banner is created, so it is definitely in the clipping container. The only way it would not be is if it were cleaned up. It is possible that the ad adapter is being animated on-screen before it actually has an ad. The black background would only exist on the ad banner animating on-screen. The ad container and clipping container both have transparent backgrounds.

nickbit commented 10 years ago

The addSubview is called only if it has not been added yet. I am speaking of a racing condition because some times the banner was added and the other time it was not. That's for the same app. I logged some messages to find out what was going wrong and compared the output. Since I am not the expert on this code, I would be grateful if you could provide with a solution to this. If there is any help I can provide please let me know.

larsacus commented 10 years ago

I'd love to see the logging output.

nickbit commented 10 years ago

Here it is. I removed my app's logging:

---------- R u n 1 --------

2014-01-10 20:01:01:769 EortesLite[24474:1803] -[LARSAdController observeValueForKeyPath:ofObject:change:context:] [Line 642] Ad loaded for Developer Ads!
2014-01-10 20:01:01:769 EortesLite[24474:1803] -[LARSAdController animateBannerForAdapterVisible:withCompletion:] [Line 373] In animateBannerForAdapterVisible
2014-01-10 20:01:01:770 EortesLite[24474:1803] -[LARSAdController onScreenBannerFrameForAdapter:withPinningLocation:] [Line 510] On screen - Final banner frame <Developer Ads>: {{0, 0}, {320, 50}}
2014-01-10 20:01:01:770 EortesLite[24474:1803] -[BannerViewController observeValueForKeyPath:ofObject:change:context:] [Line 138] In observeValue
2014-01-10 20:01:01:770 EortesLite[24474:1803] -[BannerViewController updateLayout] [Line 109] updateLayout
2014-01-10 20:01:01:770 EortesLite[24474:1803] -[BannerViewController viewDidLayoutSubviews] [Line 68] In viewDidLayoutSubviews
2014-01-10 20:01:01:770 EortesLite[24474:1803] -[BannerViewController viewDidLayoutSubviews] [Line 80] ------ Ad is visible
2014-01-10 20:01:01:770 EortesLite[24474:1803] -[BannerViewController viewDidLayoutSubviews] [Line 98] ------ Changing

---- R u n 2 ---

2014-01-10 20:01:45:392 EortesLite[24530:1803] -[LARSAdController observeValueForKeyPath:ofObject:change:context:] [Line 642] Ad loaded for Developer Ads!
2014-01-10 20:01:45:392 EortesLite[24530:1803] -[LARSAdController animateBannerForAdapterVisible:withCompletion:] [Line 373] In animateBannerForAdapterVisible
2014-01-10 20:01:45:393 EortesLite[24530:1803] -[LARSAdController offScreenBannerFrameForAdapter:presentationAnimationType:] [Line 485] Off screen - Initial banner frame <Developer Ads>: {{0, 50}, {320, 50}}
2014-01-10 20:01:45:393 EortesLite[24530:1803] -[LARSAdController onScreenBannerFrameForAdapter:withPinningLocation:] [Line 510] On screen - Final banner frame <Developer Ads>: {{0, 0}, {320, 50}}
2014-01-10 20:01:45:393 EortesLite[24530:1803] -[BannerViewController observeValueForKeyPath:ofObject:change:context:] [Line 138] In observeValue
2014-01-10 20:01:45:393 EortesLite[24530:1803] -[BannerViewController updateLayout] [Line 109] updateLayout
2014-01-10 20:01:45:394 EortesLite[24530:1803] -[BannerViewController viewDidLayoutSubviews] [Line 68] In viewDidLayoutSubviews
2014-01-10 20:01:45:394 EortesLite[24530:1803] -[BannerViewController viewDidLayoutSubviews] [Line 80] ------ Ad is visible
2014-01-10 20:01:45:394 EortesLite[24530:1803] -[BannerViewController viewDidLayoutSubviews] [Line 98] ------ Changing
larsacus commented 10 years ago

Are both runs running incorrectly? It looks like you are also using the developer ads -- if you remove the developer ads, does the issue persist?

nickbit commented 10 years ago

The first one is OK while the second one is not. I haven't tried without e developer ads, since this is what I need at this time. Probably if the ad load takes some time, the problem goes away.

larsacus commented 10 years ago

The developer ads project is currently in an unfinished state. If you notice any bugs in that project, I'd love to entertain a pull request. Let me know if the issue goes away when you remove the developer ads.

nickbit commented 10 years ago

Ok I found it.

It is indeed in the TOLDeveloperAds.m

- (UIView *)bannerView {
    if (_devBannerView == nil) {
        CGRect frame = [self frameForCurrentOrientation];
        self.devBannerView = [[TOLDeveloperBannerView alloc] initWithFrame:frame];

        UITapGestureRecognizer *tapGesture = [[UITapGestureRecognizer alloc] initWithTarget:self action:@selector(bannerTapped:)];
        [_devBannerView addGestureRecognizer:tapGesture];
      NSLog(@"adapter:%@  bannerView:%@", self, self.devBannerView);
    }
    return _devBannerView;
}

In TOLDeveloperAds.h declare

@property (strong, nonatomic) TOLDeveloperBannerView *devBannerView;

instead of

@property (strong, nonatomic) TOLDeveloperBannerView *bannerView;

otherwise it clashes with the TOLAdapter protocol method.

Of course replace all occurrences of bannerView with devBannerView in .m file

larsacus commented 10 years ago

If you look at the other two adapters, they follow the exact same pattern that the developers ads do and don't have any issues having the same name. All we are doing here is overriding the getter for the protocol name for the bannerView and creating it ourselves lazily if has not been created already. That is the only thing that method should be doing. There is no reason why simply renaming the ivar should solve this.

nickbit commented 10 years ago

Yes, I see it. It seems however that this change solves the problem. I would have gone on the safe side and avoid the name clash with the protocol however. There should be an explanation in any case, why this works and the other does not.

nickbit commented 10 years ago

Maybe this one helps: I noticed when it failed that the adapter.bannerView was creating a new one (_bannerView == nil in adapter) although a valid bannerView was already in the clipping subviews (containsObject check failed).

larsacus commented 10 years ago

The definition of a protocol is a template for properties or methods that should be implemented by some concrete implementation. There is no such thing as a "name clash" with a protocol. That being said, there is absolutely no difference between the original code and your new code except for the name of the ivar where the banner is actually being stored.