Doing subtractions and multiplications with a new Date() is awkward and the creation of the object is not needed if we only get the time.
Also globals should start with g_.
Doesn't change random map scripts so that D249 doesn't have to be rebased.
Details
Details
- Reviewers
causative - Commits
- rP19802: JS Date cleanup.
Notice that the terms return the same value or that if they wouldn't it wouldn't make a difference as we only use it to measure time diffs.
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Comment Actions
Build has FAILED
Link to build: http://jw:8080/job/phabricator/1363/
See console output for more information: http://jw:8080/job/phabricator/1363/console
Comment Actions
Executing section Default...
Executing section Source...
Executing section JS...
| | [NORMAL] ESLintBear (no-else-return):
| | Unnecessary 'else' after 'return'.
|----| | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/functions_utility.js
| |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/functions_utility.js
| 43| 43| return -1;
| 44| 44| else if (lowerX > lowerY)
| 45| 45| return 1;
| 46| |- else
| 47| |- return 0;
| | 46|+ return 0;
| 48| 47| }
| 49| 48|
| 50| 49| /**
binaries/data/mods/public/gui/common/functions_utility.js
| 212| » }·catch·(e)·{
| | [NORMAL] ESLintBear (no-empty):
| | Empty block statement.
binaries/data/mods/public/gui/common/functions_utility.js
| 176| » » if·(word.toLowerCase().indexOf(lastWord.toLowerCase())·!=·0)
| | [NORMAL] JSHintBear:
| | Use '!==' to compare with '0'.
| | [NORMAL] ESLintBear (no-multi-spaces):
| | Multiple spaces found before 'tries'.
|----| | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/maps/random/belgian_uplands.js
| |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/maps/random/belgian_uplands.js
| 266| 266| if (maxMinDist > minDistBetweenPlayers)
| 267| 267| {
| 268| 268| goodStartPositionsFound = true;
| 269| |- log("Exiting giant while loop after " + tries + " tries with a minimum player distance of " + maxMinDist);
| | 269|+ log("Exiting giant while loop after " + tries + " tries with a minimum player distance of " + maxMinDist);
| 270| 270| }
| 271| 271| else
| 272| 272| log("maxMinDist <= " + minDistBetweenPlayers + ", maxMinDist = " + maxMinDist);
binaries/data/mods/public/maps/random/belgian_uplands.js
| 302| » » » » if·(i·==·0)·//·...deep·water
| | [NORMAL] JSHintBear:
| | Use '===' to compare with '0'.
binaries/data/mods/public/maps/random/belgian_uplands.js
| 432| » » » if·(j·%·2·==·0)
| | [NORMAL] JSHintBear:
| | Use '===' to compare with '0'.
| | [NORMAL] ESLintBear (no-undef-init):
| | It's not necessary to initialize 'indexToAddTo' to undefined.
|----| | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/maps/random/caledonian_meadows.js
| |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/maps/random/caledonian_meadows.js
| 110| 110| let numPointsToAdd = pointsToAdd.length;
| 111| 111| for (let i = 0; i < numPointsToAdd; ++i)
| 112| 112| {
| 113| |- let indexToAddTo = undefined;
| | 113|+ let indexToAddTo;
| 114| 114| let minEnlengthen = Infinity;
| 115| 115| let minDist1 = 0;
| 116| 116| let minDist2 = 0;
| | [NORMAL] ESLintBear (no-undef-init):
| | It's not necessary to initialize 'actor' to undefined.
|----| | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/maps/random/caledonian_meadows.js
| |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/maps/random/caledonian_meadows.js
| 713| 713| {
| 714| 714| let x = areas[h][t].x;
| 715| 715| let y = areas[h][t].y;
| 716| |- let actor = undefined;
| | 716|+ let actor;
| 717| 717| let texture = pickRandom(myBiome[h].texture);
| 718| 718|
| 719| 719| if (slopeMap[x][y] < 0.4 * (minSlope[h] + maxSlope[h]))
binaries/data/mods/public/maps/random/caledonian_meadows.js
| 147| » while·(true)
| | [NORMAL] ESLintBear (no-constant-condition):
| | Unexpected constant condition.
binaries/data/mods/public/maps/random/caledonian_meadows.js
| 113| » » let·indexToAddTo·=·undefined;
| | [NORMAL] JSHintBear:
| | It's not necessary to initialize 'indexToAddTo' to 'undefined'.
binaries/data/mods/public/maps/random/caledonian_meadows.js
| 278| » » if·(placements.every(p·=>·getDistance(p.x,·p.y,·point.x,·point.y)·>·max(minDistance,·p.dist)))
| | [NORMAL] JSHintBear:
| | Don't make functions within a loop.
binaries/data/mods/public/maps/random/caledonian_meadows.js
| 283| » » if·(tries·!=·0·&&·tries·%·100·==·0)·//·Time·Check
| | [NORMAL] JSHintBear:
| | Use '!==' to compare with '0'.
binaries/data/mods/public/maps/random/caledonian_meadows.js
| 283| » » if·(tries·!=·0·&&·tries·%·100·==·0)·//·Time·Check
| | [NORMAL] JSHintBear:
| | Use '===' to compare with '0'.
binaries/data/mods/public/maps/random/caledonian_meadows.js
| 359| » » if·(i·%·3·==·0)
| | [NORMAL] JSHintBear:
| | Use '===' to compare with '0'.
binaries/data/mods/public/maps/random/caledonian_meadows.js
| 409| » » if·(i·%·2·==·0)
| | [NORMAL] JSHintBear:
| | Use '===' to compare with '0'.
binaries/data/mods/public/maps/random/caledonian_meadows.js
| 716| » » let·actor·=·undefined;
| | [NORMAL] JSHintBear:
| | It's not necessary to initialize 'actor' to 'undefined'.
binaries/data/mods/public/maps/random/caledonian_meadows.js
| 755| » if·(choice·==·0)
| | [NORMAL] JSHintBear:
| | Use '===' to compare with '0'.
| | [NORMAL] ESLintBear (no-multi-spaces):
| | Multiple spaces found before 'Engine'.
|----| | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
| |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
| 585| 585| */
| 586| 586| function updateTimeNotifications()
| 587| 587| {
| 588| |- let notifications = Engine.GuiInterfaceCall("GetTimeNotifications", g_ViewedPlayer);
| | 588|+ let notifications = Engine.GuiInterfaceCall("GetTimeNotifications", g_ViewedPlayer);
| 589| 589| let notificationText = "";
| 590| 590| for (let n of notifications)
| 591| 591| {
binaries/data/mods/public/gui/session/messages.js
| 615| » while·(true)
| | [NORMAL] ESLintBear (no-constant-condition):
| | Unexpected constant condition.
binaries/data/mods/public/gui/session/messages.js
| 496| » let·cheatCode·=·Object.keys(g_Cheats).find(cheatCode·=>·text.indexOf(cheatCode)·==·0);
| | [NORMAL] JSHintBear:
| | Use '===' to compare with '0'.
binaries/data/mods/public/gui/session/messages.js
| 874| » if·(chatAddressee.selected·>·0·&&·(text.indexOf("/")·!=·0·||·text.indexOf("/me·")·==·0))
| | [NORMAL] JSHintBear:
| | Use '!==' to compare with '0'.
binaries/data/mods/public/gui/session/messages.js
| 874| » if·(chatAddressee.selected·>·0·&&·(text.indexOf("/")·!=·0·||·text.indexOf("/me·")·==·0))
| | [NORMAL] JSHintBear:
| | Use '===' to compare with '0'.
binaries/data/mods/public/gui/session/messages.js
|1088| » let·isMe·=·msg.text.indexOf("/me·")·==·0;
| | [NORMAL] JSHintBear:
| | Use '===' to compare with '0'.
binaries/data/mods/public/gui/session/messages.js
|1092| » isMe·=·msg.text.indexOf("/me·")·==·0;
| | [NORMAL] JSHintBear:
| | Use '===' to compare with '0'.
binaries/data/mods/public/gui/session/messages.js
|1210| » » if·(text.indexOf(pName·+·"·")·==·0·&&·pName.length·>·match.length)
| | [NORMAL] JSHintBear:
| | Use '===' to compare with '0'.
| | [NORMAL] ESLintBear (no-multi-spaces):
| | Multiple spaces found before '+'.
|----| | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
| |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|1592|1592|
|1593|1593| playerStatistics.economyScore += total + ",";
|1594|1594| playerStatistics.militaryScore += Math.round((player.sequences.enemyUnitsKilledValue[maxIndex] +
|1595| |- player.sequences.enemyBuildingsDestroyedValue[maxIndex]) / 10) + ",";
| |1595|+ player.sequences.enemyBuildingsDestroyedValue[maxIndex]) / 10) + ",";
|1596|1596| playerStatistics.totalScore += (total + Math.round((player.sequences.enemyUnitsKilledValue[maxIndex] +
|1597|1597| player.sequences.enemyBuildinhttp://jw:8080/job/phabricator_lint/50/ for more details.
Comment Actions
mods/public/gui/common/functions_global_object.js has (new Date()).toLocaleTimeString() at line 72. So this useless braces could be removed too, if we're clearing all occurrences of new Date().
Comment Actions
period binds stronger than new by the looks of things:
https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Operators/Operator_Precedence
Comment Actions
Nope, it has the same priority, because it's new with arguments. Also you could notice, that it works for new Date().getTime(), which you've removed from functions_utility.js.
Comment Actions
If you want me to review this, I need to test them... so either remove me as a reviewer or add a test plan that at least covers each line of code touched. There are some things here I'm not 100% sure about. I'm not entirely certain how scoping works here with respect to g_LastTickTime, which you define in two files. Does that mean one overwrites the other? Or are they in different contexts? Other than that I have looked over the code and it looks OK to me, so if that's all you were looking for you can commit, but I'm not going to put a green checkmark on this without testing.
Comment Actions
That var was the case before already, so I'm innocent :p
Both your question can be answered with yes. If both files would be loaded, the most recently executed statement would overwrite.
But the vars are also defined in two different contexts:
- pregame is the mainmenu and that variable is used to animate the opening of submenus smoothly
- session is the ingame code and that variable is also used to animate the dropdown of the menu in the top right hand corner
Those Engine.SwitchGuiPage change the currently displayed page (and thus drop every currently loaded vars)
Engine.PushGuiPage stacks a page on top of it (still can't access the vars of the other pages).
The The XML files specified by the arguments of those 2 commands specify which XML and JS files are loaded then.
So this way one could prove that the mainmenu and the ingame page are never loaded at the same time, thus can never conflict.
add a test plan that at least covers each line of code touched
Seems a bit overkill. The important part is figuring out that the changed code returns exactly the same number as before.
Only g_LastTickTime has the type change (Date type to Number type) but the subtraction should still return the same value.
This code can be tested by checking that the two menus still dropdown correctly.
Comment Actions
I'm officially allowed to use the term 'eyeballed by causative' in the commit message which seems fair enough. I'm not going to write a test plan for every single line as this should be a quite simple patch, but I do appreciate that causative doesn't want to accept the patch officially if he isn't sure about every line changed.