blitz-research / monkey

Blitz Research Monkey Source
225 stars 59 forks source link

Added an AppDelegate class, similar to the ActivityDelegate class found ... #56

Open sygem opened 10 years ago

sygem commented 10 years ago

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

blitz-research commented 10 years ago

Definitely like the general idea - a few thoughts:

sygem commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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/blitz-research/monkey/pull/56#issuecomment-43492462 .

sygem commented 9 years ago

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 commented 9 years ago

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