@wraitii I guess you have overseen @vladislavbelov's inline comments. ;)

I listed myself, because I thought not listing myself leaves the impression I didn't review the patch. But you are right, committing implies a review by the committer, so listing myself is redundant.

In D1377#73333, @Angen wrote:

just still dont get this javascript features somehow, there is return text && sprintf("..."), and aside from that the result of this expresion should be true, but it is replaced string

what part don't you get?
the sprintf?
the &&?

Will commit soon.

Use D844. Fix minor style issues. Use correct style.

Game works for me. (Intel Xeon with Ubuntu Gnome)

My concern was fixed. Thanks.

In general that looks like a good patch. The idea to group variables and functions that touch the same topic into globals sounds ok, but have you thought about using files for grouping?
Regarding the state machine (inputState) I would strongly recommend looking into using a proper fsm implementation with entry and leave functions. (I think we have a fsm.js somewhere) That could also help fixing some bugs like #4848
Also related to #2826

In D1377#72778, @elexis wrote:

It would only be changing this:

Engine.GetGUIObjectByName("tipText").caption = tipText.join("\n");

to

Engine.GetGUIObjectByName("tipText").caption = tipText.map(text => sprintf("• %(tiptext)s", { "text": text })).join("\n");

that's not a lot of work at all (in particular in comparison to changing every line in every file and making every translator copy&paste that bullet symbol for every translation).

In D1377#72771, @elexis wrote:

Doesn't sound like a good alternative, as some language might want to e.g. add the symbol at the end of the line...

sprintf(translate("%(symbol)s %(text)s"))

In D1377#72768, @elexis wrote:

Is it really better to modify every line to use that `•? Translators will have to edit it again. They might do so once, but what if we like to chose a different styling, for example an actual texture?
Alternatives would be keeping the "-" and replacing it if the line begins with that character (sounds like keeping ballast);
or deleting the - and prepending an arbitrary symbol to every line but the first.

Doesn't sound like a good alternative, as some language might want to e.g. add the symbol at the end of the line...

XML would be an alternative but had the disadvantage that one can't use the same GUI object structure for every tooltip.
JSON would be an alternative, but I don't see where the txt format is worse than JSON for the loading tips.

So yes I think (and it seems like you do too) it is best to modify each line.

Taking my two comments aside that looks pretty good. (Also it was accepted by others already...)

In rP22105#32480, @Stan wrote:

That's fair. I'll add it then.

In rP22105#32478, @Stan wrote:

Well only if it is being used in the main game building roster. Else it would confuse players.

Shouldn't you also add the building to the map "Sandbox - The Gauls"?

I guess you also want to add tests for that new feature :)

In rP22103#32457, @elexis wrote:

The default review questions are:

• What are the properties and implications of the existing implementation? Which of them are desirable and which would be preferably avoidable?
• What are the properties and implications of the proposed implementation? Comparing to the previous implementation, are the stakeholders (players, devs, modders, players hardware components like memory and cpu) better off?
• Which alternative implementation can we imagine?
• Which implementation of all these will be the best?
• Synthesis: Can we imagine a new implementation that keeps the advantages of all proposed models while leaving as few disadvantages as possible?

In rP22103#32453, @elexis wrote:

This.isInfinite is no longer serialized

Is this is the only behavior change in this commit? Behavior changes should be mentioned in the commit message (and before cosmetics).

@Imarok, do you have an opinion on the implementation of the serialization change?

Nice patch

Rest looks good.

In D1752#72563, @elexis wrote:

I wasn't proposing a hotkey that triggers the "middle button action", but a hotkey that triggers the specific minimap action you want for the minimap patch.

I know

The question is whether it's a good idea to assign three actions to buttons in general and thus commit the patch, or wether whether alternate patterns, such as one action per button are more intuitive.
Perhaps there are some fringe examples, I guess the minimap button already is one of the fringiest. So perhaps it's okay to add that support in the engine but advise people to better use a separate button instead for regular buttons, unless they have a really strong use case where it's also a bad idea to use global hotkeys.

Considering the Minimap use case, it's also strange that this event doesn't send the coordinates to the minimap. I guess it works that way: user clicks, code is triggered, code gets the coordinates from the minimap object? I have a revision proposal somewhere to allow sending a JS::Object to the GUI event.
It should really be interchangeable for people without the third mousebutton. Don't OSX hipsters have only one mouse button?

Isn't the ScriptEvent("worldclick", coords); + IsHotkeyPressed("sendping_minimap") enabling that?

In D1517#72556, @elexis wrote:

How about this? https://stackoverflow.com/questions/201593/is-there-a-simple-way-to-convert-c-enum-to-string/238157#238157

That's more or less the same as my first bullet point.

(There also are D492 / D1578 doing things to the network FSM code)

As far as I can see they don't change anything with the enums, except that the MessageTypes are moved into a separate file.

What are you waiting for? ;)

In D1752#70523, @elexis wrote:

Actually, does this have to be hardlinked to a mouse tripleclick? Why not use hotkeys so people can put it on arbitrary input? There will certainly be people with only 2 buttons (Wasn't that about every mac user?) Which doesn't mean that the GUI hardcoding support can't be completed. I guess the use case for these hardcoded GUI events is to keep some things unchangeable, like the leftclick on a button, the leftclick on a list item. These are conventions that are the same throughout platforms, websites, decades. But there are not many of such things that are so common.

GUI events are for actions executed onto a specific GUI element. Hotkeys are global events or apply to the focused GUI element.
Polling or only reacting if minimap is focused is not suitable, so we have to use GUI events. For those without a middle mouse button, we could implement a gui button. (Analogous to the unit commands which can be triggered either by key or by buttons)

It's already supported, see default.cfg:

pan = MouseMiddle ; Enable scrolling by moving mouse

There it is used as global Hotkey and checked via polling.

Adding const

In D1517#63044, @elexis wrote:

The macro in NetMessages.h apparently does something similar to what we want here, resulting in the C++ variable names in the mainlog.html file:
<p>Net client: Received message CAuthenticateResultMessage { m_Code: 0, m_HostID: 12, m_Message: Logged in } of size 29 from server</p>

Implement suggested changes

Fix the addded comments ^^

Add comments to the Schema

Sorry to ask, but does this need petra support?

It seems like you made the diff dependencies wrong around.
I think this revision should be the parent of GridBrowser GUI addon.
(Everyone gets confused by that at least once ;))

Thank you for the patch.
And sorry, that it took so much time :/

In D932#71723, @Polakrity wrote:

In D932#71681, @elexis wrote:

Wondering whether these two options shouldn't be located in the Gamesetup UI and Lobby UI part of default.cfg and options.json.

I think it's more instinctive to go to the "Sound" tab.

I tend to the same opinion.

But maybe u want duplicate these options in each tab?

Definitely not a good idea ;)

Add a option to disable the experience status bar

Fix license header year

Good patch.
The only question that remains: Should it be enabled by default?
It could be quite annoying, so I'd lean to disable it by default.
What do you think?

In D1769#71333, @elexis wrote:

Was there a reason for the == case?

Was there since leper implemented Healing in rP11536

Adding an option for this is a good idea.
Will do so as soon as i have time for it.

In D1473#70819, @wraitii wrote:

I think this should differentiate packed and unpacked siege engines. (Correct me if it already does)

It doesn't as-is but I actually disagree here, so :p . Is the triple-clicking behaviour described above in-game already?

In D1473#70814, @wraitii wrote:

This seems obviously good to me?
Unless I'm misunderstanding things this just affects the double-clicking behaviour?

Support for multiple pings. Use player color for pings. Ping is shown as increasing square. (hope someone will code something more beautiful) Limit each player to 1 ping per second.

In D1751#70444, @elexis wrote:

In CCmpMinimap.cpp:

	// Not serialized (based on renderer timing):
	// TODO: eventually ping state should be serialized and tied into simulation time, but currently lag causes too many problems
	double m_PingEndTime;

		case MT_MinimapPing:
			// This depends on the viewing player, so don't alter the synchronized simulation state
			m_IsPinging = true;

Can find some more information with blame then.

That is the minimap attack notification. (white blinking unit)

Use sim command instead of chat. Play sound. (but the wrong) Create ping entity on real map. (but the wrong)

In D1751#70415, @elexis wrote:

(I think a simulation command would be the wrong place for that.)

That's an important decision. Why is a simulation command wrong and chat right?

Whether it goes through the simulation or not, it sounds like it should end up as a GUI notification, near the other events that the GUI processes that it receives from the gamestate. So that one could also play a funky sound or display something in the 2D menus.
That would be g_NotificationsTypes instead of g_IsChatAddressee and parseChatAddressee (Maybe the others have an opinion too, but IMO parsing player command events that create local simulation entities seems really out of place from a function that is supposed to be about chat messages)
These events usually come from the GUIInterface, i.e. cmpGuiInterface.AddTimeNotification or cmpGuiInterface.PushNotification.
If you do it via chat, it won't be in the replay. If it was in the simulation, one could even do things in the simulation if that event is received (extensibility for future creativity in 0ad and mods. for example units automatically reacting towards that ping, perhaps an actual simulation entity that may influence the simulation other than the VisualActor component and Position, marking that position instead of every player creating the same local one). One could store additional data about the ping.
For example if you want a ping to be displayed for 30 seconds and people rejoin or load a savegame 1 second after such a map ping was sent, some of the players will see it for the next 29 seconds and possibly refer to that point in the chat, while the other player doesn't see that thing.

Ok, you convinced me. It'll be a sim command. (Was my first approach, but I thought such only visible thing should be better handled like chat)

Finishing to read the code, I see there is no local entity created yet.

As already stated in the TODOs in the diff description.

IMO that would be very relevant, we have a bunch of flag actors, and perhaps we can find something shiny to make it more visible without trying to find the differently colored pixel on that small thing in the bottom left corner.

The plan is to make more than just a tiny pixel in the minimap. I thought about expanding concentric circles or such.

Cosmetic changes (Year numbers, semi)

If someone had read the linting results of the patch....
(Yeah, I know input.js throws a bunch of warnings)

If nobody objects, I'll commit that soon.

In D1749#70366, @wraitii wrote:

Not to be annoying but wouldn't it be easier to say "from -X to +X, and "Normal" has no bonuses"?

In D1749#70337, @Nescio wrote:

How about the following then?

The AI's resource gathering rate and trade gain depends on the difficulty level (Sandbox −58%, Very Easy −44%, Easy −25%, Medium normal, Hard +25%, Very Hard +56%); the easiest levels also have a slower production and building rate.