Add FlxGroup#killAll() and FlxGroup#reviveAll()

[IQAndreas]

See #30 (comment)

I also added a second commit which points out this new behavior of FlxGroup#kill() and just emphasizes the original behavior of FlxGroup#revive(). Since we are no longer overriding those methods, adding the relevant ASDoc inside FlxGroup would be possible, but seems more complicated than we need to make it. It's easier just to add the information to FlxBasic instead.

Should I keep the second commit, or remove it?

[Dovyski]

First a question: now that we have killAll() and reviveAll(), shouldn't FlxGroup#kill() just kill itself instead of all its members?

About the code, even though it looks great, I think we could implement it using already existing methods, such as callAll(). It makes sense to me since killAll() is some sort of alias for callAll("kill") after all. The same goes for reviveAll().

About the ASDoc, I think it is misplaced. The text should not be placed in FlxBasic, but in FlxGroup instead. If I want to know how FlxGroup#kill() works, I will check the docs for that method, not it's equivalent in the super class.

[IQAndreas]

Now that we have killAll() and eviveAll(), shouldn't FlxGroup#kill() just kill itself instead of all its members?

Yep, that's the way I set it up in the code (the diff looks a bit funny, but you can see the final result here).

The text should not be placed in FlxBasic, but in FlxGroup instead. If I want to know how FlxGroup#kill() works, I will check the docs for that method, not it's equivalent in the super class.

I agree, but the problem is, FlxGroup no longer has it's own kill or revive methods, since it is no longer overriding the FlxBasic methods, so adding the documentation may prove a hassle. After some thinking (though I haven't tested it yet), may be possible to do something like this:

/** Documentation here... */
override public function kill():void
{
    super.kill();
}

The reason I added it to FlxBasic: I figured since FlxBasic already mentions FlxGroup in a few cases, it's maybe not the end of the world if the FlxGroup behaviour is placed where it is.

[IQAndreas]

I think we could implement it using already existing methods, such as callAll(). It makes sense to me since killAll() is some sort of alias for callAll("kill") after all. The same goes for reviveAll().

To be honest, although handy, I hate methods like that; it reeks of too much "dynamicness". I figured every member is going to at least be a FlxBasic object which is guaranteed to have the kill() and revive() methods. When possible, I personally prefer being "stricter" about it by calling the methods directly.

It is also much faster, though in this scenario, I don't see performance as being an issue as killAll() and reviveAll() would be called so seldom.

Although it does use up more code than is necessary, so I am alright with changing it to callAll("kill") if you prefer.

[Dovyski]

I'm sorry about the kill() change, I probably overlooked while inspecting the code. As using callAll(), I agree with you about "dynamicness" and I don't like them too. Let's stick with the current implementation of killAll() and reviveAll().

About the docs, I'm not sure how to proceed. I think something like this...

/** Documentation here... */
override public function kill():void
{
    super.kill();
}

...is ugly, but necessary. IDEs showing docs during auto-complete, for instance, will help developers to understand what is happening. However, as you said, it's not the end of the world with the docs are in FlxBasic, I'm ok with that :)

And about pushing this change up to the next release, any thoughts on that?

[IQAndreas]

And about pushing this change up to the next release, any thoughts on that?

Yeah, that's probably a good idea. I went ahead and added a pull request which reverts the changes to revive() as well: #187

[Dovyski]

Great! This pull request will remain open and attached to #30, which has been pushed up to the next release. During the next cycle we check and merge this pull request.