[PATCH] Ungarrison all units from an ally building

[rogue-spectre]

Currently when one garrisons units in a building of an ally there is no option to ungarrison all units in one shoot like there is for one's building.

[maveric]

Better version.

[sanderd17]

Thanks for working on this. But I have some comments:

• Input.js: Instead of "if (test) return true / false", you can also "return test", which is more concise.
• unit_actions.js::942: The IsOwnedByAlly function is confusing. cmpPlayer.IsAlly() also returns true on itself (so a player is always considered an ally of itself). There shouldn't be a difference with the function you create now.
• GarrisonHolder.js and Commands.js: We indeed used for..each..in in the past, but we started the transition to the new ES standard for..of (​https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of)

Apart from that, I'm unsure if the building owner should be able to ungarrison allied units he doesn't own. EDIT: leper thinks it's fine if a building owner can ungarrison all units, so it's ok to leave it like that.

But overall, it looks like a good patch.

[maveric]

Replying to sanderd17:

• unit_actions.js::942: The IsOwnedByAlly? function is confusing. cmpPlayer.IsAlly?() also returns true on itself (so a player is always considered an ally of itself). There shouldn't be a difference with the function you create now.

This function will return false, if player is entity owner (we need different behavior for player (ungarrison all units) and allies (ungarrison only those that belong to player)).

[maveric]

Fixed all except IsOwnedByAlly function. We cant just call to cmpPlayer.isAlly because we need to distinguish only allies(not player).

[leper]

Replying to sanderd17:

• Input.js: Instead of "if (test) return true / false", you can also "return test", which is more concise.

You now do

if (state)
    return state.garrisonHolder;
return false;

while you should just do

return state && state.garrisonHolder;

.

Is the IsOwnedByAlly function just needed because we add the commands panel to the gui for allies? If yes adding some sort of filter to setupUnitPanel, or providing a specific implementation for allies (that might just copy most of the properties from the normal object over and replace a few) might be nicer.

Apart from that the code looks good.

[maveric]

Replying to leper:

Is the IsOwnedByAlly function just needed because we add the commands panel to the gui for allies?

It is also used when deciding either we send "unload-all" command or "unload-all-by-owner" to execute ungarrisoning (see 683 line of unit_actions.js).

[sanderd17]

If you make a separate panel for allies, you can add a different button with a different function asigned to it. Then you don't need to switch between the two cases internally.

You'd only need that check once: when deciding which panel to show (the owner panel, the allied panel. or none). So you won't have to put it in a function. And that will avoid the naming issue.

[maveric]

Well, it sounds like a good idea.

[maveric]

Added ally units command panel as new panel.

[sanderd17]

Nice work. I guess these should be the final remarks:

• Can you also add the repair button to the panel? You can probably just reuse the object of the other panel, so

  "repair": g_EntityCommands.repair

I think that's the only thing an ally should be able to do (next to garrisoning).

• There's an issue with the count display on the ungarrison button. it should only show the units you can ungarrison, so the units you own as allied garrisoner.

• You should also mind your whitespace (like keeping empty lines consistent, or the space at unit_actions.js::918).

[maveric]

Replying to sanderd17:

• Can you also add the repair button to the panel? You can probably just reuse the object of the other panel, so

  "repair": g_EntityCommands.repair

I think that's the only thing an ally should be able to do (next to garrisoning).

Maybe this issue is worth creating separate ticket? Because its not going to work this way and I am not sure about how this button should act (does it mean that player will be able to order ally workers to repair somewhat?).

[maveric]

The rest issues are fixed.

[leper]

You still have a space before var g_AllyEntityCommands =, you remove blank lines between functions in input.js, before the g_SelectionPanels.AllyCommand block you should add a blank line. And you reuse state in g_AllyEntityCommands in non-obvious ways.

[sanderd17]

Sorry, my bad, I thought the repair button was put on the structure to repair, not on the unit that does the reparation. So that comment was invalid.

[sanderd17]

In 16597:

Allow players to unload all their units when garrisoned in allied buildings. Patch by maveric. Fixes #2857