Open IQAndreas opened 10 years ago
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.
Now that we have
killAll()
andeviveAll()
, shouldn'tFlxGroup#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.
I think we could implement it using already existing methods, such as
callAll()
. It makes sense to me sincekillAll()
is some sort of alias forcallAll("kill")
after all. The same goes forreviveAll()
.
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.
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?
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: https://github.com/FlixelCommunity/flixel/pull/187
Great! This pull request will remain open and attached to https://github.com/FlixelCommunity/flixel/issues/30, which has been pushed up to the next release. During the next cycle we check and merge this pull request.
See https://github.com/FlixelCommunity/flixel/issues/30#issuecomment-26753623
I also added a second commit which points out this new behavior of
FlxGroup#kill()
and just emphasizes the original behavior ofFlxGroup#revive()
. Since we are no longer overriding those methods, adding the relevant ASDoc insideFlxGroup
would be possible, but seems more complicated than we need to make it. It's easier just to add the information toFlxBasic
instead.Should I keep the second commit, or remove it?