In D1786#73229, @elexis wrote:

I'm pointing out these things mostly because I want to inform on our coding conventions and good practices.
But I think the entire file needs to be revisioned structually, adding some syntax sugar is not the essence but part of what needs to be done IMO.
And the more stuff is added to the patch, the harder it will be to review.
Fixing 6000 lines in many logical and syntactical ways is something that can be done in a github branch and typically consumes several weeks to discover all the defects and the ideal structure. Then one can refactor that into small disjoint, atomic, patches.
(I guess the code isn't really inviting to newcomers who want to add one feature and are confronted with a piling disarrangement.)

Turned the input events to a FSM from FSM.js (more o less)
Fixes #4848 and implements #2826

Successful build - Chance fights ever on the side of the prudent.

I really like the change, I just have some minor feedback for specific lines.

"Must be constructed in neutral territory. Has no territory radius effect. Trains all 'mercenary' units."

The military colony does have a territory radius and a different name.

Perhaps you could also include some related corrections for consistency and completeness?

• cavalry units → cavalry
• infantry units → infantry
• mercenary soldiers → mercenaries

Isn't that building the military coloney ?

Line 103 appears to be identical to line 66, so you might want to correct that one too.
However, because neither the civ bonus nor the structure are implemented, it might be better to just purge both.

Looks good to me.

Successful build - Chance fights ever on the side of the prudent.

In order to move things forward, I’ve simplified the diff to change only ‘cavalry soldiers → cavalry’, which was the scope of the original report.

Successful build - Chance fights ever on the side of the prudent.

I agree with the word change. However, since this change affects filenames beyond the specific word change suggested, I’ll let people more familiar with unit filename conventions give their approval.

@Itms any concerns with this patch ?

It is recomputed just if object changes owner, is created or some modification comes. Overlay for units is not updated white moving even when they get elevation bonus. Using let now = Date.now() and logging Date.now() - now at the end of GetElevationAdaptedRange returns 0 or 1 depending on range, bigger maximum range and elevationbonus longer it gets to compute. However with 1000 loop it freezes. But it is actually such problem if range overlay is not updated on movement ? We may restrict this computation just for buildings and make TODO to solve it for units ?

With the new message subscription, one could patrol on a hill with a unit that has the range displayed. Then the thing would be updated very often.

Successful build - Chance fights ever on the side of the prudent.

added missing check for change in elevation bonus

Right, elevationBonus is number based on which is added range corresponding to how high is current terrain.
It is based on GuiInterface.js lines 410 - 426 which result is used to display (+x) in toolip for range attack.

Didn't we have a ticket for it? I recall receiving a report about that once.
I also seem to recall having looked into the code and having gained the impression that range bonus was already accounted for.
Actually I get it again looking into the code and wondering why the elevation code in GetRange doesn't already provide the information.
I guess ElevationBonus != GetElevationAdaptedRange, the former must just have been height of the building.
Good then.

Successful build - Chance fights ever on the side of the prudent.

Before the patch the balkon position was wrong.