#4161 closed enhancement (fixed)
[PATCH] cmpPlayer cleanup
Reported by: | leper | Owned by: | leper |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 21 |
Component: | UI & Simulation | Keywords: | patch |
Cc: | Patch: |
Description
Removes leakage of internal data, moves message registrations to the right place, cleans up a test, more leakage removal, adds utility function to clean up some things.
Attachments (5)
Change History (15)
comment:1 by , 8 years ago
comment:2 by , 8 years ago
I am permitting myself to review your patches.
0001:
maps/scripts/ConquestCommon.js
was forgotten
0002:
- i was also disturbed by that when updating the docs.
- Should we move those from
interfaces/Trigger.js
andinterfaces/EndGameManager.js
too ?
0003:
- ok
0004:
- ok
0005:
GuiInterface.js
L80 -> L90 can also use those new functions- (perhaps var -> let)
by , 8 years ago
Attachment: | 0001-Remove-cmpPlayerManager.GetAllPlayerEntities-which-l.patch added |
---|
by , 8 years ago
Attachment: | 0002-Move-message-registrations-to-the-right-file.patch added |
---|
by , 8 years ago
Attachment: | 0003-Remove-unneeded-includes.patch added |
---|
by , 8 years ago
Attachment: | 0004-Prevent-leaking-internal-data-in-cmpPlayer.patch added |
---|
comment:3 by , 8 years ago
3-5 weren't really updated apart from the last one, but the only change that remains is the removal of comments for the new functions that don't add anything more than what is in the name already. L80-90 needs a full array of booleans, so the new functions aren't a perfect fit for that and using them would very likely break lots of things in the gui.
My patches also aren't in the review queue to not get reviewed, so that first sentence leaves me puzzled.
EDIT: Now 005 also includes a bugfix.
by , 8 years ago
Attachment: | 0005-Add-GetPlayersByDiplomacy-to-cmpPlayer.patch added |
---|
comment:4 by , 8 years ago
(I looked too fast at those GuiInterface
array.)
0001:
- L88 of
maps/scripts/ConquestCommon.js
. If I'm not wrong you, the entities owned by gaia are also added, I don't know if it's intended.
0002:
- perhaps the
"PlayerDefeated"
and"PlayerWon"
messages from interfaces/EndGameManager.js can be moved too (as they are sent by Player component).
0003: ok
0004: ok
0005: ok
(by ok, I mean: I understood what is done in the patch, I checked the syntax, ran the tests, launched some games).
Shouldn't we use let instead of var?