Skip to content
This repository was archived by the owner on Nov 4, 2023. It is now read-only.

Conversation

@sygem
Copy link

@sygem sygem commented May 11, 2014

...in the Android target. This makes it much easier for Module authors to hook into the application lifecycle, enabling deeper integration with the iOS platform.

A bit of history behind this change - I needed to hook into the "applicationDidBecomeActive" method for my Facebook plugin. Initially I added a category to BBMonkeyAppDelegate, overriding that method but keeping the call to "game->ResumeGame()". However, I am now writing a module for local notifications, and that also needs to put some code in "applicationDidBecomeActive". Using a category again would mean that I can't use both modules together, so a solution akin to that found in the Android target seems the most appropriate.

FWIW, I have already used this approach in my Chartboost module to start the session each time the user switches to the app, and that has been used in two submitted apps so far.

…nd in the Android target. This makes it much easier for Module authors to hook into the application lifecycle, enabling deeper integration with the iOS platform.
@blitz-research
Copy link
Owner

Definitely like the general idea - a few thoughts:

  • I don't think it should extend Monkey Object - it's for native use only and that just complicates things. For one thing, it'll be allocated via Monkey's gc (I think)! Unless you're 'marking' it, it'll probably cause problems eventually.
  • We've got a few 'AppDelegates' in there already now. How about calling it IosAppDelegate or something?
  • Can't seem to find the implementations for AppDelegate methods? Does it compile OK without them?!? If there's no implementation, virtual methods should be declared with '=0' at the end;.
  • Should we add a few more methods for future use? eg: didEnterBackground, willEnterForeground? or I guess people can add these as they need them.

- Added additional app lifecycle methods
- Fixed removal of app delegates using NSValue
@sygem
Copy link
Author

sygem commented May 15, 2014

Interestingly, when I took out the extending from Monkey Object it stopped compiling, because the "mark()" method goes missing. My usage goes something like "Monkey Class A extends Native Class B which extends IosAppDelegate".

I have also added some more methods to the IosAppDelegate, but decided against making them pure virtual - typically the applicationDidBecomeActive would be the most commonly used method, and pure virtuals would force people to write 4 more empty methods each time.

@blitz-research
Copy link
Owner

I'm still not really comfortable with IosAppDelegate being 'visible' to Monkey code - what are you doing on the Monkey side?

In the case of a chartboost module, I would have thought you'd write some native code that extended IosAppDelegate, and invoked the appropriate chartboost methods from there.

When I did the android ActivityDelegate, I also intended it to be for native use only. This eliminated a whole bunch of potential problems, such as 'app state' issues where the callbacks may occur at unusual or unexpected times - perhaps even in a different thread. In C++, there are also GC issues, such as 'marking' the delegates (which your code doesn't do, but probably works for you if you've got a 'delegate' global).

I'm not totally opposed to a Monkey-side IosAppDelegate, but the original intention was to provide a way for native developers to 'hook' into app objects at a low level with maximum flexibility.

@sygem
Copy link
Author

sygem commented May 19, 2014

I have been able to rewrite my code so that the IosAppDelegate doesn't extend from Object. To give you a concrete idea of how I am using this, I have uploaded two versions of my AdManager class:

http://www.sygem.com/monkey/advertmanager.ios.cpp
http://www.sygem.com/monkey/advertmanager.ios.copy.cpp

The first file shows how the IosAppDelegate would be used if it doesn't extend Object, and the second file does exactly the same stuff, this time extending from Object. On the Monkey side, there is a class that extends the Extern'd AdManager and calls methods like Initialize(), which registers the AppDelegate.

In conclusion then, both approaches are workable. I can see two advantages of my original approach - there is less native code to write, and the native iOS code follows a very similar pattern to the Android code (which extends from ActivityDelegate). I don't really understand the GC issues though, so if that is a deal breaker, so be it. I would be happy for you to make a final decision one way or another :-)

@blitz-research
Copy link
Owner

Ok, thanks for the examples, makes it much clearer what you are actually
doing!

My main concern was that you were implementing the IosAppDelegate methods
actually in Monkey, ie: had declared an Extern class etc. But you're not
doing this, so I'm a little more relaxed IosAppDelegate extending Object -
but not totally relaxed!

Another alternative is to use multiple inheritance, eg:

class AdManager : public Object, public IosAppDelegate{
public:

...etc...AdManager and IosAppDelegate methods go here...

virtual ~AdManager(){
//remove 'this' from global native iosAppDelegates list...
}
}

This effectively treats IosAppDelegate as an interface (but with
pre-implemented methods).

Note the destructor here - this automatically removes the IosAppDelegate
from the global native list of app delegates.

This or something like it is necessary in case the GC decides that the
AdManager object is 'unreachable'. For example, if you assign an AdManager
object to a Field, but later overwrite that field (either with Null, or a
different AdManager) the old AdManager becomes 'garbage' and will
eventually be deleted. If it's not also removed from the global
iosAppDelegates list, the next time applicationDidBecomeActive or whatever
happens, all hell is likely to break loose!

Either way, I still think I'm more comfortable with IosAppDelegate not
extending Object if you're OK with that.

On Mon, May 19, 2014 at 11:48 PM, sygem notifications@github.com wrote:

I have been able to rewrite my code so that the IosAppDelegate doesn't
extend from Object. To give you a concrete idea of how I am using this, I
have uploaded two versions of my AdManager class:

http://www.sygem.com/monkey/advertmanager.ios.cpp
http://www.sygem.com/monkey/advertmanager.ios.copy.cpp

The first file shows how the IosAppDelegate would be used if it doesn't
extend Object, and the second file does exactly the same stuff, this time
extending from Object. On the Monkey side, there is a class that extends
the Extern'd AdManager and calls methods like Initialize(), which registers
the AppDelegate.

In conclusion then, both approaches are workable. I can see two advantages
of my original approach - there is less native code to write, and the
native iOS code follows a very similar pattern to the Android code (which
extends from ActivityDelegate). I don't really understand the GC issues
though, so if that is a deal breaker, so be it. I would be happy for you to
make a final decision one way or another :-)


Reply to this email directly or view it on GitHubhttps://github.com//pull/56#issuecomment-43492462
.

@sygem
Copy link
Author

sygem commented Sep 13, 2014

I finally got around to looking at this again - I have rewritten all my code so that the IosAppDelegate doesn't extend Object. I am using the multiple inheritance as you suggested - it looks good and works well.

@sygem
Copy link
Author

sygem commented Feb 26, 2015

I have now added support for Local Notifications to this pull request, to accompany my notifications module.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants