Assuming you the renames are correct and the proposed changes are made.

Accept if proposed changes are followed.

In D406#66521, @vladislavbelov wrote:

the double > JS::Number conversion loses precision.

I think that is why we have FixedPoint number type in C++. (I guess it's mainly use in simulation)

In D1649#66370, @elexis wrote:

In D1649#66186, @Imarok wrote:

Besides the comments: Looks good.
Could reproduce and fixes. 👍

Anything more that needs review?

Nice 👍

Besides the comments: Looks good.
Could reproduce and fixes. 👍

Fix the inconsistency noticed by elexis for the Enable/Disable button, the website button and the arrows.

In D1623#64723, @elexis wrote:

While proofreading the modmod files for completeness, I found that the default caption of this button is inconsistent. It is shown when the button is disabled by default, but the caption remains Enabled if there is no mod selected. It would be more consistent to always have the same state if there is no mod selected.

Good catch!

Why did you abandon?
I think nothing speaks against allowing right and left click.

The proposed renames in the patch description sound sane. 👍
The added descriptions are also good. (Besides the comments I gave)
(Needs rebase.)

Else looks good

In D1627#65048, @smiley wrote:

In D1627#65047, @Imarok wrote:

Good job. 👍
Is it intended, that the Enable Feedback button is first greyed out and the user has to click on technical details and then on connect to enable the user report?

Pretty sure it's intended. They are supposed to read and accept the terms before doing it I guess.

Sure but wouldn't it be better to just open the terms window when the user clicks the enable feedback button without reading the terms instead of disabling the button?
The current way seems to be a bit counterintuitive.

Good job. 👍
Is it intended, that the Enable Feedback button is first greyed out and the user has to click on technical details and then on connect to enable the user report?

I didn't forget this diff, but as I understand our current state, only things that are needed for the rerelease or that are game-breaking are committed.

Ok, works

In rP20064#30938, @elexis wrote:

rP20064

I think Itms has the best overview about our CI and such things.

se>>! In rP21441#31054, @elexis wrote:

So when I commit a bug and call it "commit a bug", it's not broken because it does what it claims to?
Why do we have the concern feature if we don't use it to keep a list of things that should be fixed that are related to a known commit?

Ubuntu 16.04.4 LTS 64-Bit
gcc version 5.4.0 20160609
using premake5 ✔

In D1581#63463, @Stan wrote:

Might want to make sure as well that the territory does not have any influence, so we can have the classical mode @wowgetoffyourcellphone talked about. Because switching territory visibility off is nice, but it might confuse people. Also it will have to be made an option, and that option shouldn't be available for rated games.

Thanks for the patch.

Cause of the "registered too many accounts within one hour" message not shown anymore.

In D1557#62851, @Imarok wrote:

Looks good code-wise.
Played a game and also worked.
Couldn't reproduce the OOS, so couldn't test that.

Looks good code-wise.
Played a game and also worked.
Couldn't reproduce the OOS, so couldn't test that.

So do I get a "yes" for the approach?

Looks good, tested.
(Couldn't test a malformed report, as that would have broken the game for all A23 players)

In D1518#62053, @elexis wrote:

In D1518#62052, @Imarok wrote:

wanna add that minimal lobby.js diff, so that we don't call Engine.GetEngineInfo() number_of_lobby_games times per guiTick?

Not for 20 microseconds * numGames.
I was rather becoming interested to go the other way and remove the cache of that call in the replaymenu.

wanna add that minimal lobby.js diff, so that we don't call Engine.GetEngineInfo() number_of_lobby_games times per guiTick?
(else looks good)

Maybe nicer approach fro creating the map, by using a macro

In D1517#61919, @elexis wrote:

Benefits of the patch:

• less error-prone debugging: It is easy to mix up things when debugging as you pointed out in this trac ticket today.
• less time-consuming: One also has to either use an IDE that displays the enum values or compute them manually. There is a number of state variables with similar names which can mislead debugging developers.
• simple code: Since the map is directly below the enum, it seems to be hard to forget the // Keep this in sync with foo part.
  
  Disadvantages of the patch:
• more error-prone code: It's feasible to mismatch string with enums when maintaining.
• slightly more maintenance work, more hunks in patches, more code to learn before comprehending it for beginners
  
  So it seems like a tradeoff of limited advantages and disadvantages. Given the time that I already spent on matching the numbers with enum constants, chosing the code strategy where one should optimize the code for progressive revisioning, maybe this could be considered doable if you want to spend the time on this rather than other things. But do we really benefit from paying the cost or is it sufficient if we debug correctly when we debug?

Just quickly scanned the diff and tried it out. Seems to be nice.
Shouldn't we also do cycling for the other autocomplete words?

Tested it in the lobby today.
Only I as host had the patch applied.
At start while loading 3 people lost connection and I got Net server: Error running FSM update (type=25 state=4) (That is NMT_END_COMMAND_BATCH and NSS_PREGAME).
But after the the remaining players finished loading, all could at least rejoin successfully.

In P121#920, @wraitii wrote:

I think the lobby change is still OK, but I'm fine with not adding it for slight readability reasons.

In P121#917, @elexis wrote:

IMO Globals are something to be avoided where possible. We have so many of them already and the ideal number of total globals is somewhere near 2.
After implementing the C++ cache, it consumes 10-30ms instead of 170ms.
30 games * 20ms per call = 0.6 seconds every few seconds or more often.
So I'd be ok with paying the cost of 1 added line of code in the updateGameList function of lobby.js as you had proposed once, while leaving the rest in JS as is?
Reportedly the gamesetup is slow too when sending stanzas, but that must be a problem of a factor 10 less than in the lobby page (as the problem is multiplied with the number of games there, while gamesetup stanzas are only sent all 3 seconds or upon client join / leave.

In D1512#61939, @wraitii wrote:

Let's do it your way then, I'm not dying on this hill. Feel free to commandeer with your patch, otherwise I'll take it as a basis and carry on myself.

In German we say: "der klügere gibt nach" ;)
(No offense in direction of elexis intended)

In D1512#61875, @elexis wrote:

meh.patch4 KBDownload

something like that although it doesn't work yet at all, needs a different scriptinterface when started in nonvisual mode, missing comments and motivation, unneeded include and moved something unrelated which gave me pain while reading.

That could work 👍

In P121#915, @elexis wrote:

What speaks against this approach for the release?

See irclogs yesterday. Recaping:
My main reason for prefering a C++ solution is that it is done once, rather than N times and that it is implementable with the same amount of code in a different place. So why pick the worse alternative if the alternative has presumably the same cost. Prefering quantity over quality doesn't pay off in the long term.
A second weaker argument is that const g_EngineInfo = Engine.GetEngineInfo(); is called each time a new GUI page is opened, every message box, so you get 170ms delay (on my machine) each time even if not used.

Hmm, that's a point..

A third weaker argument is that new pages don't have it cached, we need to go through all GUI pages to check it for completeness.

In D1512#61933, @wraitii wrote:

I think we have a pretty good handle on the problem, and we realise that properly fixing it is hard. I suggest doing the hotfix (JS caching), and then raising a concern about it until we fix things properly. Otherwise it'll take ages imo.

I agree. I proposed a patch in P121.

What speaks against this approach for the release?
It is global.
It needs no maintenance.
It changes only simple things, so probability of breaking is quite low.
Of course this does not cache C++ invocations of Engine.GetEngineInfo(), but those aren't called that often.

In D1512#61930, @elexis wrote:

In D1512#61928, @Imarok wrote:

Could we just do a small and quick fix (common js only cache) for A23 and think about the approptiate fix for A24 after the rerelease?

Committing something without knowing what investigating the situation prior to the commit is not happening with my agreement and the matter is not hard to understand.

Could we just do a small and quick fix (common js only cache) for A23 and think about the approptiate fix for A24 after the rerelease?

This breaks the version check.

Looks good.
The use of SetLongTimeout is correct and complete. (i.e. all functions leading to a loading screen set the long timeout and all functions leading out of the loading into the game revert the timeout)
Wondered about the 0s in enet_peer_timeout(peer, 0, 0, 0); but after reading the documentation (http://enet.bespin.org/group__peer.html#gac48f35cdd39a89318a7b4fc19920b21b) it seems correct.

Looks nice, will give a closer look tomorrow.

Why not caching it on init?

I cannot reproduce any lobbylag on windows. All above was doen using Ubuntu.

In rP21301#30684, @ffffffff wrote:

lead to immersive lags in the lobby (Engine.GetEngineInfo().mods command) (only when public mod is zipped to public.zip, unzipped all fine)

to test replace Engine.GetEngineInfo().mods in lobby.js and gamesetup.js with [] so it wont be executed

will be much faster then
but i dont know why atm

(i hear when they take my mod fgodmod and the lobby files and some other files are in unpacked version then on files system is less lag users tell.)

In rP21242#30675, @ffffffff wrote:

lead to immersive lags in the lobby (Engine.GetEngineInfo().mods command) (only when public mod is zipped to public.zip, unzipped all fine)

to test replace Engine.GetEngineInfo().mods in lobby.js and gamesetup.js with [] so it wont be executed

will be much faster then
but i dont know why atm

What about that: enemy units give some resistance against pushing, but if they get pushed hard enough they also go away. (Not sure if that is easy to implement)

Also add the location if entity has vanished. Still containing a button texture and not using the correct textWidth. Both will be fixed as soon as D844 is done.

In D1461#60834, @elexis wrote:

We can equally convert the lagwarnings to buttons that ask you to kick the guy.

oops, that was the wrong tab xD

In D1461#60588, @elexis wrote:

As mentioned line breaks won't occur here as far as I see, but if we ever want to support them, D844 would be a place to visit.

Fix works and is complete.
Was broken since implementation in rP13944

Really using the hotkey was forgotten...

In D1461#60571, @elexis wrote:

afaik

Complaint and Engine.GetTextWidth

In rP21076#30299, @ffffffff wrote:

can we get experience bar small white below healthbar maybe on unit? helpsome for watching development of units in fight.

In rP20886#27707, @ffffffff wrote:

In rP20886#27706, @elexis wrote:

(Notice he means the golden line, not the white one)

oh there must be wrong something no gap ther

In D1461#59849, @elexis wrote:

Only "You have been attacked" should be the button, maybe that timestamp prefix too. The rest has no reason to not remain clickable.