Page MenuHomeWildfire Games
Differential D753

Not so small input.js (originally been batch training) cleanup
ClosedPublic
Actions

Authored by Imarok on Jul 23 2017, 3:42 PM.

Details

Reviewers
bb
Commits
rP20447: Not so small input.js (originally been batch training) cleanup
Summary
  • Move selection_panels.js helper functions into a separate file
  • Rearrange some functions, to better represent the execution flow
  • Make batch training use g_NumberOfBatches instead of g_BatchTrainingCount so remember the number of batches instead of the number of units to train (needed for finishing P60)
  • Simplify the batch training code a bit
Test Plan

everything should work as before:

  • batch training
  • batch training with limits (e.g. wardogs)

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
bb added inline comments.Sep 20 2017, 9:54 PM
binaries/data/mods/public/gui/session/input.js
1117–1118 ↗(On Diff #3614)

Do notice this behaviour is different when both training and build restrictions are specified (not that this is anywhere the case now, but probably is nice for mods, a unit that can be build and trained.) but meh for now

1213 ↗(On Diff #3614)

what is this adding to getBuildingsWhichCanTrainEntity?

1220 ↗(On Diff #3614)

seems like used only once so inline

1239–1240 ↗(On Diff #3614)

tab => spaces

1244 ↗(On Diff #3614)

--g_NumberOfBatches?

1248–1249 ↗(On Diff #3614)

it looks worse to me to split 1 condition over 2 lines (a<\n b)

1249 ↗(On Diff #3614)

tab => spaces

1315 ↗(On Diff #3614)

indentation

1326 ↗(On Diff #3614)

previous indentation was better

1357 ↗(On Diff #3614)

whitespaces?

1423 ↗(On Diff #3614)

check looks indeed wrong

binaries/data/mods/public/gui/session/selection_panels_input.js
46 ↗(On Diff #3614)

not sure if moving this function is correct, since it is called from unit_actions.js as well when just giving a simple order, by clicking the session.

91–93 ↗(On Diff #3614)

"resource, dropsite, foundation, or enemy unit" don't think this is true anymore, and don't want to update the comment when a new action is added. I guess remove the first sentence and make the second more generic.

137 ↗(On Diff #3614)

not really selection_panels right?

174–175 ↗(On Diff #3614)

don't see why negating is better

186–187 ↗(On Diff #3614)

same

251 ↗(On Diff #3614)

why newline?, meh

Imarok retitled this revision from Small input.js (batch training) cleanup to Not so small input.js (originally been batch training) cleanup.Sep 30 2017, 6:40 PM
Imarok marked 19 inline comments as done.
In D753#35957, @bb wrote:

Perhaps before starting to move around functions randomly, we should have a clear plan which functions should be in which file or so.

I moved every function to that new file, that is not used inside input.js...

binaries/data/mods/public/gui/session/input.js
1248–1249 ↗(On Diff #3614)

True

1357 ↗(On Diff #3614)

where do you think it's wrong?

1119 ↗(On Diff #2938)

I guess those might be undefined.

binaries/data/mods/public/gui/session/selection_panels_input.js
46 ↗(On Diff #3614)

but isn't unit_actions.js also part of the selection_panels?

137 ↗(On Diff #3614)

not really selection_panels right?

It is only used there.
(see https://github.com/0ad/0ad/blob/709c78dbbd2c406d3f34717d3a314a3a06ee24d4/binaries/data/mods/public/gui/session/selection_panels.js#L927)

174–175 ↗(On Diff #3614)

because it removes one level of indentation

186–187 ↗(On Diff #3614)

same

326 ↗(On Diff #3614)

I know, but it's imho better to read this way

336 ↗(On Diff #2938)

I find it easier to read this way.

Imarok marked 3 inline comments as done.Sep 30 2017, 6:54 PM
Imarok updated this revision to Diff 3813.Sep 30 2017, 6:54 PM

Apply bb's style remarks

Vulcan added a comment.Sep 30 2017, 8:00 PM

Build is green

Updating workspaces...
Updating bundled third-party dependencies...

FCollada/FCollada.cpp
FCollada/FColladaPlugin.cpp
FCollada/FCDocument/FCDAnimated.cpp
FCollada/FCDocument/FCDAnimationChannel.cpp
FCollada/FCDocument/FCDAnimationClip.cpp
FCollada/FCDocument/FCDAnimationClipTools.cpp
FCollada/FCDocument/FCDAnimation.cpp
FCollada/FCDocument/FCDAnimationCurve.cpp
FCollada/FCDocument/FCDAnimationCurveTools.cpp
FCollada/FCDocument/FCDAnimationKey.cpp
FCollada/FCDocument/FCDAnimationMultiCurve.cpp
FCollada/FCDocument/FCDAsset.cpp
FCollada/FCDocument/FCDCamera.cpp
FCollada/FCDocument/FCDController.cpp
FCollada/FCDocument/FCDControllerInstance.cpp
FCollada/FCDocument/FCDControllerTools.cpp
FCollada/FCDocument/FCDEffectCode.cpp
FCollada/FCDocument/FCDEffect.cpp
FCollada/FCDocument/FCDEffectParameter.cpp
FCollada/FCDocument/FCDEffectParameterFactory.cpp
FCollada/FCDocument/FCDEffectParameterSampler.cpp
FCollada/FCDocument/FCDEffectParameterSurface.cpp
FCollada/FCDocument/FCDEffectPass.cpp
FCollada/FCDocument/FCDEffectPassShader.cpp
FCollada/FCDocument/FCDEffectPassState.cpp
FCollada/FCDocument/FCDEffectProfile.cpp
FCollada/FCDocument/FCDEffectProfileFX.cpp
FCollada/FCDocument/FCDEffectStandard.cpp
FCollada/FCDocument/FCDEffectTechnique.cpp
FCollada/FCDocument/FCDEffectTools.cpp
FCollada/FCDocument/FCDEmitter.cpp
FCollada/FCDocument/FCDEmitterInstance.cpp
FCollada/FCDocument/FCDEmitterObject.cpp
FCollada/FCDocument/FCDEmitterParticle.cpp
FCollada/FCDocument/FCDEntity.cpp
FCollada/FCDocument/FCDEntityInstance.cpp
FCollada/FCDocument/FCDEntityReference.cpp
FCollada/FCDocument/FCDExternalReferenceManager.cpp
FCollada/FCDocument/FCDExtra.cpp
FCollada/FCDocument/FCDForceDeflector.cpp
FCollada/FCDocument/FCDForceDrag.cpp
FCollada/FCDocument/FCDForceField.cpp
FCollada/FCDocument/FCDForceGravity.cpp
FCollada/FCDocument/FCDForcePBomb.cpp
FCollada/FCDocument/FCDForceWind.cpp
FCollada/FCDocument/FCDGeometry.cpp
FCollada/FCDocument/FCDGeometryInstance.cpp
FCollada/FCDocument/FCDGeometryMesh.cpp
FCollada/FCDocument/FCDGeometryNURBSSurface.cpp
FCollada/FCDocument/FCDGeometryPolygons.cpp
FCollada/FCDocument/FCDGeometryPolygonsInput.cpp
FCollada/FCDocument/FCDGeometryPolygonsTools.cpp
FCollada/FCDocument/FCDGeometrySource.cpp
FCollada/FCDocument/FCDGeometrySpline.cpp
FCollada/FCDocument/FCDImage.cpp
FCollada/FCDocument/FCDLibrary.cpp
FCollada/FCDocument/FCDLight.cpp
FCollada/FCDocument/FCDLightTools.cpp
FCollada/FCDocument/FCDMaterial.cpp
FCollada/FCDocument/FCDMaterialInstance.cpp
FCollada/FCDocument/FCDMorphController.cpp
FCollada/FCDocument/FCDObject.cpp
FCollada/FCDocument/FCDObjectWithId.cpp
FCollada/FCDocument/FCDocument.cpp
FCollada/FCDocument/FCDocumentTools.cpp
FCollada/FCDocument/FCDParameterAnimatable.cpp
FCollada/FCDocument/FCDParticleModifier.cpp
FCollada/FCDocument/FCDPhysicsAnalyticalGeometry.cpp
FCollada/FCDocument/FCDPhysicsForceFieldInstance.cpp
FCollada/FCDocument/FCDPhysicsMaterial.cpp
FCollada/FCDocument/FCDPhysicsModel.cpp
FCollada/FCDocument/FCDPhysicsModelInstance.cpp
FCollada/FCDocument/FCDPhysicsRigidBody.cpp
FCollada/FCDocument/FCDPhysicsRigidBodyInstance.cpp
FCollada/FCDocument/FCDPhysicsRigidBodyParameters.cpp
FCollada/FCDocument/FCDPhysicsRigidConstraint.cpp
FCollada/FCDocument/FCDPhysicsRigidConstraintInstance.cpp
FCollada/FCDocument/FCDPhysicsScene.cpp
FCollada/FCDocument/FCDPhysicsShape.cpp
FCollada/FCDocument/FCDPlaceHolder.cpp
FCollada/FCDocument/FCDSceneNode.cpp
FCollada/FCDocument/FCDSceneNodeIterator.cpp
FCollada/FCDocument/FCDSceneNodeTools.cpp
FCollada/FCDocument/FCDSkinController.cpp
FCollada/FCDocument/FCDTargetedEntity.cpp
FCollada/FCDocument/FCDTexture.cpp
FCollada/FCDocument/FCDTransform.cpp
FCollada/FCDocument/FCDVersion.cpp
FCollada/FMath/FMAllocator.cpp
FCollada/FMath/FMAngleAxis.cpp
FCollada/FMath/FMColor.cpp
FCollada/FMath/FMInterpolation.cpp
FCollada/FMath/FMLookAt.cpp
FCollada/FMath/FMMatrix33.cpp
FCollada/FMath/FMMatrix44.cpp
FCollada/FMath/FMQuaternion.cpp
FCollada/FMath/FMRandom.cpp
FCollada/FMath/FMSkew.cpp
FCollada/FMath/FMVector3.cpp
FCollada/FMath/FMVolume.cpp
FCollada/FUtils/FUAssert.cpp
FCollada/FUtils/FUBase64.cpp
FCollada/FUtils/FUBoundingBox.cpp
FCollada/FUtils/FUBoundingSphere.cpp
FCollada/FUtils/FUCrc32.cpp
FCollada/FUtils/FUCriticalSection.cpp
FCollada/FUtils/FUDaeEnum.cpp
FCollada/FUtils/FUDateTime.cpp
FCollada/FUtils/FUDebug.cpp
FCollada/FUtils/FUError.cpp
FCollada/FUtils/FUErrorLog.cpp
FCollada/FUtils/FUFile.cpp
FCollada/FUtils/FUFileManager.cpp
FCollada/FUtils/FULogFile.cpp
FCollada/FUtils/FUObject.cpp
FCollada/FUtils/FUObjectType.cpp
FCollada/FUtils/FUParameter.cpp
FCollada/FUtils/FUParameterizable.cpp
FCollada/FUtils/FUPluginManager.cpp
FCollada/FUtils/FUSemaphore.cpp
FCollada/FUtils/FUStringBuilder.cpp
FCollada/FUtils/FUStringConversion.cpp
FCollada/FUtils/FUSynchronizableObject.cpp
FCollada/FUtils/FUThread.cpp
FCollada/FUtils/FUTracker.cpp
FCollada/FUtils/FUUniqueStringMap.cpp
FCollada/FUtils/FUUri.cpp
FCollada/FUtils/FUXmlDocument.cpp
FCollada/FUtils/FUXmlParser.cpp
FCollada/FUtils/FUXmlWriter.cpp
FColladaPlugins/FArchiveXML/FArchiveXML.cpp
FColladaPlugins/FArchiveXML/FAXAnimationExport.cpp
FColladaPlugins/FArchiveXML/FAXAnimationImport.cpp
FColladaPlugins/FArchiveXML/FAXCameraExport.cpp
FColladaPlugins/FArchiveXML/FAXCameraImport.cpp
FColladaPlugins/FArchiveXML/FAXColladaParser.cpp
FColladaPlugins/FArchiveXML/FAXColladaWriter.cpp
FColladaPlugins/FArchiveXML/FAXControllerExport.cpp
FColladaPlugins/FArchiveXML/FAXControllerImport.cpp
FColladaPlugins/FArchiveXML/FAXEmitterExport.cpp
FColladaPlugins/FArchiveXML/FAXEmitterImport.cpp
FColladaPlugins/FArchiveXML/FAXEntityExport.cpp
FColladaPlugins/FArchiveXML/FAXEntityImport.cpp
FColladaPlugins/FArchiveXML/FAXForceFieldExport.cpp
FColladaPlugins/FArchiveXML/FAXForceFieldImport.cpp
FColladaPlugins/FArchiveXML/FAXGeometryExport.cpp
FColladaPlugins/FArchiveXML/FAXGeometryImport.cpp
FColladaPlugins/FArchiveXML/FAXImportLinking.cpp
FColladaPlugins/FArchiveXML/FAXInstanceExport.cpp
FColladaPlugins/FArchiveXML/FAXInstanceImport.cpp
FColladaPlugins/FArchiveXML/FAXLightExport.cpp
FColladaPlugins/FArchiveXML/FAXLightImport.cpp
FColladaPlugins/FArchiveXML/FAXMaterialExport.cpp
FColladaPlugins/FArchiveXML/FAXMaterialImport.cpp
FColladaPlugins/FArchiveXML/FAXPhysicsExport.cpp
FColladaPlugins/FArchiveXML/FAXPhysicsImport.cpp
FColladaPlugins/FArchiveXML/FAXSceneExport.cpp
FColladaPlugins/FArchiveXML/FAXSceneImport.cpp
FCollada/FCollada.cpp
FCollada/FCDocument/FCDAnimated.cpp
FCollada/FColladaPlugin.cpp
FCollada/FCDocument/FCDAnimationChannel.cpp
FCollada/FCDocument/FCDAnimationClip.cpp
FCollada/FCDocument/FCDAnimationClipTools.cpp
FCollada/FCDocument/FCDAnimation.cpp
FCollada/FCDocument/FCDAnimationCurve.cpp
FCollada/FCDocument/FCDAnimationCurveTools.cpp
FCollada/FCDocument/FCDAnimationKey.cpp
FCollada/FCDocument/FCDAnimationMultiCurve.cpp
FCollada/FCDocument/FCDAsset.cpp
FCollada/FCDocument/FCDCamera.cpp
FCollada/FCDocument/FCDController.cpp
FCollada/FCDocument/FCDControllerInstance.cpp
FCollada/FCDocument/FCDControllerTools.cpp
FCollada/FCDocument/FCDEffectCode.cpp
FCollada/FCDocument/FCDEffect.cpp
FCollada/FCDocument/FCDEffectParameter.cpp
FCollada/FCDocument/FCDEffectParameterFactory.cpp
FCollada/FCDocument/FCDEffectParameterSampler.cpp
FCollada/FCDocument/FCDEffectParameterSurface.cpp
FCollada/FCDocument/FCDEffectPass.cpp
FCollada/FCDocument/FCDEffectPassShader.cpp
FCollada/FCDocument/FCDEffectPassState.cpp
FCollada/FCDocument/FCDEffectProfile.cpp
FCollada/FCDocument/FCDEffectProfileFX.cpp
FCollada/FCDocument/FCDEffectStandard.cpp
FCollada/FCDocument/FCDEffectTechnique.cpp
FCollada/FCDocument/FCDEffectTools.cpp
FCollada/FCDocument/FCDEmitter.cpp
FCollada/FCDocument/FCDEmitterInstance.cpp
FCollada/FCDocument/FCDEmitterObject.cpp
FCollada/FCDocument/FCDEmitterParticle.cpp
FCollada/FCDocument/FCDEntity.cpp
FCollada/FCDocument/FCDEntityInstance.cpp
FCollada/FCDocument/FCDEntityReference.cpp
FCollada/FCDocument/FCDExternalReferenceManager.cpp
FCollada/FCDocument/FCDExtra.cpp
FCollada/FCDocument/FCDForceDeflector.cpp
FCollada/FCDocument/FCDForceDrag.cpp
FCollada/FCDocument/FCDForceField.cpp
FCollada/FCDocument/FCDForceGravity.cpp
FCollada/FCDocument/FCDForce

http://jenkins-master:8080/job/phabricator/2088/ for more details.

Vulcan added a comment.Sep 30 2017, 8:20 PM
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'target' to undefined.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/input.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/input.js
| 202| 202| 	if (!g_DevSettings.controlAll && !allOwnedByPlayer)
| 203| 203| 		return undefined;
| 204| 204| 
| 205|    |-	var target = undefined;
|    | 205|+	var target;
| 206| 206| 	if (!fromMinimap)
| 207| 207| 	{
| 208| 208| 		var ent = Engine.PickEntityAtPoint(x, y);
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'actionInfo' to undefined.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/input.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/input.js
| 216| 216| 	var actions = Object.keys(unitActions).slice();
| 217| 217| 	actions.sort((a, b) => unitActions[a].specificness - unitActions[b].specificness);
| 218| 218| 
| 219|    |-	var actionInfo = undefined;
|    | 219|+	var actionInfo;
| 220| 220| 	if (preSelectedAction != ACTION_NONE)
| 221| 221| 	{
| 222| 222| 		for (var action of actions)

binaries/data/mods/public/gui/session/input.js
| 198| »   »   var·entState·=·GetEntityState(ent);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'entState' is already declared in the upper scope.

binaries/data/mods/public/gui/session/input.js
| 426| »   switch·(ev.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 453| »   switch·(inputState)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 457| »   »   switch·(ev.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 512| »   »   switch·(ev.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 519| »   »   »   var·maxDragDelta·=·16;
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'maxDragDelta' is already declared in the upper scope.

binaries/data/mods/public/gui/session/input.js
| 562| »   »   switch·(ev.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 591| »   »   switch·(ev.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 660| »   »   switch·(ev.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 780| »   switch·(inputState)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 783| »   »   switch·(ev.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 885| »   »   switch·(ev.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 977| »   »   switch·(ev.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
|1046| »   »   »   switch·(ev.hotkey)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
|1368| »   switch·(action)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 205| »   var·target·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'target' to 'undefined'.

binaries/data/mods/public/gui/session/input.js
| 219| »   var·actionInfo·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'actionInfo' to 'undefined'.

binaries/data/mods/public/gui/session/input.js
| 233| »   for·(var·action·of·actions)
|    | [NORMAL] JSHintBear:
|    | 'action' is already defined.

binaries/data/mods/public/gui/session/input.js
| 236| »   »   »   var·r·=·unitActions[action].hotkeyActionCheck(target,·selection);
|    | [NORMAL] JSHintBear:
|    | 'r' is already defined.

binaries/data/mods/public/gui/session/input.js
| 241| »   for·(var·action·of·actions)
|    | [NORMAL] JSHintBear:
|    | 'action' is already defined.

binaries/data/mods/public/gui/session/input.js
| 244| »   »   »   var·r·=·unitActions[action].actionCheck(target,·selection);
|    | [NORMAL] JSHintBear:
|    | 'r' is already defined.

binaries/data/mods/public/gui/session/input.js
| 437| »   mouseIsOverObject·=·(hoveredObject·!=·null);
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'null'.

binaries/data/mods/public/gui/session/input.js
| 441| »   »   &&·(ev.button·==·SDL_BUTTON_LEFT·||·ev.button·==·SDL_BUTTON_RIGHT))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/gui/session/input.js
| 471| »   »   »   »   var·rect·=·updateBandbox(bandbox,·ev,·true);
|    | [NORMAL] JSHintBear:
|    | 'rect' is already defined.

binaries/data/mods/public/gui/session/input.js
| 474| »   »   »   »   var·ents·=·getPreferredEntities(Engine.PickPlayerEntitiesInRect(rect[0],·rect[1],·rect[2],·rect[3],·g_ViewedPlayer));
|    | [NORMAL] JSHintBear:
|    | 'ents' is already defined.

binaries/data/mods/public/gui/session/input.js
| 623| »   »   »   »   »   var·queued·=·Engine.HotkeyIsPressed("session.queue");
|    | [NORMAL] JSHintBear:
|    | 'queued' is already defined.

binaries/data/mods/public/gui/session/input.js
| 663| »   »   »   var·dragDeltaX·=·ev.x·-·dragStart[0];
|    | [NORMAL] JSHintBear:
|    | 'dragDeltaX' is already defined.

binaries/data/mods/public/gui/session/input.js
| 664| »   »   »   var·dragDeltaY·=·ev.y·-·dragStart[1];
|    | [NORMAL] JSHintBear:
|    | 'dragDeltaY' is already defined.

binaries/data/mods/public/gui/session/input.js
| 665| »   »   »   var·maxDragDelta·=·16;
|    | [NORMAL] JSHintBear:
|    | 'maxDragDelta' is already defined.

binaries/data/mods/public/gui/session/input.js
| 697| »   »   »   »   var·queued·=·Engine.HotkeyIsPressed("session.queue");
|    | [NORMAL] JSHintBear:
|    | 'queued' is already defined.

binaries/data/mods/public/gui/session/input.js
| 816| »   »   »   »   if·(ev.hotkey.indexOf("selection.group.")·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/input.js
| 821| »   »   »   »   »   »   if·(ev.hotkey.indexOf("selection.group.select.")·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/input.js
| 829| »   »   »   »   »   »   var·sptr·=·ev.hotkey.split(".");
|    | [NORMAL] JSHintBear:
|    | 'sptr' is already defined.

binaries/data/mods/public/gui/session/input.js
| 845| »   »   »   var·ent·=·Engine.PickEntityAtPoint(ev.x,·ev.y);
|    | [NORMAL] JSHintBear:
|    | 'ent' is already defined.

binaries/data/mods/public/gui/session/input.js
| 856| »   »   »   »   var·action·=·determineAction(ev.x,·ev.y);
|    | [NORMAL] JSHintBear:
|    | 'action' is already defined.

binaries/data/mods/public/gui/session/input.js
| 871| »   »   »   }
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'default'.

binaries/data/mods/public/gui/session/input.js
| 875| »   »   »   if·(g_Selection.toList().length·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/input.js
| 898| »   »   »   var·ent·=·Engine.PickEntityAtPoint(ev.x,·ev.y);
|    | [NORMAL] JSHintBear:
|    | 'ent' is already defined.

binari

http://jenkins-master:8080/job/phabricator_lint/561/ for more details.

Harbormaster completed remote builds in B3299: Diff 3813.Sep 30 2017, 8:21 PM
bb added inline comments.Oct 1 2017, 8:21 PM
binaries/data/mods/public/gui/session/input.js
1357 ↗(On Diff #3614)

between the "[" and "app" and the "Batch]" and "]"

binaries/data/mods/public/gui/session/selection_panels_input.js
46 ↗(On Diff #3614)

Nope unit_actions is part of session, and so it should be since about all user orders (not just the selection panels ones) run through that. Selection_panels is just the panels on the bottom including the garrison button and co there. F.e. a walk order given to a unit by clicking the session is clearly not selection_panels.

137 ↗(On Diff #3614)

Strange that is selection_panels but ok, does have some logical sense, but the comment in selection_panels could be adapted so it includes these buttons => out of scope

174–175 ↗(On Diff #3614)

meh leaves shorter code otherwise

bb added inline comments.Oct 1 2017, 8:24 PM
binaries/data/mods/public/gui/session/selection_panels_input.js
46 ↗(On Diff #3614)

maybe the function should be moved to unit_actions or so dunno

Imarok marked 4 inline comments as done.Oct 1 2017, 10:05 PM
Imarok added inline comments.
binaries/data/mods/public/gui/session/input.js
1357 ↗(On Diff #3614)

between the "[" and "app" and the "Batch]" and "]"

why that? afaik we don't use whitespaces when indexing
(e.g. someArray[1])

binaries/data/mods/public/gui/session/selection_panels_input.js
46 ↗(On Diff #3614)

true

174–175 ↗(On Diff #3614)

"Would make that an early to avoid the alignment and to make it consistent"
"don't see why negating is better"
Please come to an agreement with @elexis
(I don't care that much about that)

Imarok updated this revision to Diff 3835.Oct 1 2017, 10:05 PM
Imarok marked an inline comment as done.

Move getActionInfo to unit_actions

Vulcan added a comment.Oct 1 2017, 10:07 PM

Build is green

Updating workspaces...
Updating bundled third-party dependencies...


SpiderMonkey is already up to date


make: Entering directory '/mnt/data/jenkins-phabricator/workspace/phabricator/build/premake/premake4/build/gmake.unix'
==== Building Premake4 (release) ====
make: Leaving directory '/mnt/data/jenkins-phabricator/workspace/phabricator/build/premake/premake4/build/gmake.unix'

Premake args:  --with-system-nvtt --without-miniupnpc --collada --atlas
Building configurations...
Running action 'gmake'...
Generating ../workspaces/gcc/Makefile...
Generating ../workspaces/gcc/pyrogenesis.make...
Generating ../workspaces/gcc/network.make...
Generating ../workspaces/gcc/tinygettext.make...
Generating ../workspaces/gcc/lobby.make...
Generating ../workspaces/gcc/glooxwrapper.make...
Generating ../workspaces/gcc/simulation2.make...
Generating ../workspaces/gcc/scriptinterface.make...
Generating ../workspaces/gcc/engine.make...
Generating ../workspaces/gcc/graphics.make...
Generating ../workspaces/gcc/atlas.make...
Generating ../workspaces/gcc/gui.make...
Generating ../workspaces/gcc/lowlevel.make...
Generating ../workspaces/gcc/mongoose.make...
Generating ../workspaces/gcc/mocks_real.make...
Generating ../workspaces/gcc/mocks_test.make...
Generating ../workspaces/gcc/AtlasObject.make...
Generating ../workspaces/gcc/AtlasUI.make...
Generating ../workspaces/gcc/ActorEditor.make...
Generating ../workspaces/gcc/Collada.make...
Generating ../workspaces/gcc/test.make...
Done.
Building configurations...
Running action 'codeblocks'...
Generating ../workspaces/codeblocks/pyrogenesis.workspace...
Generating ../workspaces/codeblocks/pyrogenesis.cbp...
Generating ../workspaces/codeblocks/network.cbp...
Generating ../workspaces/codeblocks/tinygettext.cbp...
Generating ../workspaces/codeblocks/lobby.cbp...
Generating ../workspaces/codeblocks/glooxwrapper.cbp...
Generating ../workspaces/codeblocks/simulation2.cbp...
Generating ../workspaces/codeblocks/scriptinterface.cbp...
Generating ../workspaces/codeblocks/engine.cbp...
Generating ../workspaces/codeblocks/graphics.cbp...
Generating ../workspaces/codeblocks/atlas.cbp...
Generating ../workspaces/codeblocks/gui.cbp...
Generating ../workspaces/codeblocks/lowlevel.cbp...
Generating ../workspaces/codeblocks/mongoose.cbp...
Generating ../workspaces/codeblocks/mocks_real.cbp...
Generating ../workspaces/codeblocks/mocks_test.cbp...
Generating ../workspaces/codeblocks/AtlasObject.cbp...
Generating ../workspaces/codeblocks/AtlasUI.cbp...
Generating ../workspaces/codeblocks/ActorEditor.cbp...
Generating ../workspaces/codeblocks/Collada.cbp...
Generating ../workspaces/codeblocks/test.cbp...
Done.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2097/ for more details.

Vulcan added a comment.Oct 1 2017, 10:08 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/session/selection_panels.js
|  60| »   »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
|  74| »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
| 345| »   »   »   tooltip·+=·"\n"·+·"[color=\"red\"]"·+·translate(formationInfo.tooltip)·+·"[/color]";
|    | [NORMAL] ESLintBear (no-useless-concat):
|    | Unexpected string concatenation of literals.

binaries/data/mods/public/gui/session/selection_panels.js
| 782| »   »   »   »   »   »   switch·(entity.check)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
|1010| »   »   »   »   "[/font]"·+·"·"·+·getEntityNamesFormatted(template),
|    | [NORMAL] ESLintBear (no-useless-concat):
|    | Unexpected string concatenation of literals.

binaries/data/mods/public/gui/session/selection_panels.js
| 282| »   »   if·(!technologyEnabled·||·limits.canBeAddedCount·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/selection_panels.js
| 443| »   »   »   »   »   "callback":·function·(item)·{·lockGate(item.locked);·}
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 449| »   »   »   »   »   "callback":·function·(item)·{·lockGate(item.locked);·}
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 499| »   »   »   if·(state.pack.progress·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/selection_panels.js
| 643| »   »   if·(data.i·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/selection_panels.js
| 686| »   »   »   »   tech·=>·tech·!=·null·&&·!ret.some(
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'null'.

binaries/data/mods/public/gui/session/selection_panels.js
| 686| »   »   »   »   tech·=>·tech·!=·null·&&·!ret.some(
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 699| »   »   »   »   ret·=·ret.concat(filteredTechs.map(tech·=>·({
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 732| »   »   pair.hidden·=·data.item.tech.pair·==·null;
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with 'null'.

binaries/data/mods/public/gui/session/selection_panels.js
| 767| »   »   »   ].map(func·=>·func(template));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 813| »   »   »   button.onPress·=·function·()·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 822| »   »   »   »   button.onMouseEnter·=·function()·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
| 825| »   »   »   »   button.onMouseLeave·=·function()·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/selection_panels.js
|1042| »   »   if·(!technologyEnabled·||·limits.canBeAddedCount·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/selection_panels.js
|1141| »   »   »   if·(progress·||·!technologyEnabled·||·limits.canBeAddedCount·==·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '===' to compa

http://jenkins-master:8080/job/phabricator_lint/570/ for more details.

Harbormaster completed remote builds in B3320: Diff 3835.Oct 1 2017, 10:08 PM
bb added inline comments.Oct 1 2017, 10:58 PM
binaries/data/mods/public/gui/session/input.js
1357 ↗(On Diff #3614)

I mean there are too many whitespaces since we have an array not an object, it should be
"entities": [appropriateBuildings[buildingsCountToTrainFullBatch]],

Imarok marked an inline comment as done.Oct 2 2017, 10:14 AM
Imarok added inline comments.
binaries/data/mods/public/gui/session/input.js
1357 ↗(On Diff #3614)

Ah, I should have read your comment better :D

Imarok updated this revision to Diff 3836.Oct 2 2017, 10:15 AM
Imarok marked an inline comment as done.

Remove two whitespaces

Vulcan added a comment.Oct 2 2017, 10:15 AM
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'target' to undefined.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/input.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/input.js
| 202| 202| 	if (!g_DevSettings.controlAll && !allOwnedByPlayer)
| 203| 203| 		return undefined;
| 204| 204| 
| 205|    |-	var target = undefined;
|    | 205|+	var target;
| 206| 206| 	if (!fromMinimap)
| 207| 207| 	{
| 208| 208| 		var ent = Engine.PickEntityAtPoint(x, y);
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'actionInfo' to undefined.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/input.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/input.js
| 216| 216| 	var actions = Object.keys(unitActions).slice();
| 217| 217| 	actions.sort((a, b) => unitActions[a].specificness - unitActions[b].specificness);
| 218| 218| 
| 219|    |-	var actionInfo = undefined;
|    | 219|+	var actionInfo;
| 220| 220| 	if (preSelectedAction != ACTION_NONE)
| 221| 221| 	{
| 222| 222| 		for (var action of actions)

binaries/data/mods/public/gui/session/input.js
| 198| »   »   var·entState·=·GetEntityState(ent);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'entState' is already declared in the upper scope.

binaries/data/mods/public/gui/session/input.js
| 426| »   switch·(ev.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 453| »   switch·(inputState)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 457| »   »   switch·(ev.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 512| »   »   switch·(ev.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 519| »   »   »   var·maxDragDelta·=·16;
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'maxDragDelta' is already declared in the upper scope.

binaries/data/mods/public/gui/session/input.js
| 562| »   »   switch·(ev.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 591| »   »   switch·(ev.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 660| »   »   switch·(ev.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 780| »   switch·(inputState)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 783| »   »   switch·(ev.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 885| »   »   switch·(ev.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 977| »   »   switch·(ev.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
|1046| »   »   »   switch·(ev.hotkey)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
|1368| »   switch·(action)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/input.js
| 205| »   var·target·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'target' to 'undefined'.

binaries/data/mods/public/gui/session/input.js
| 219| »   var·actionInfo·=·undef

http://jenkins-master:8080/job/phabricator_lint/571/ for more details.

Vulcan added a comment.Oct 2 2017, 10:19 AM

Build is green

Updating workspaces...
Updating bundled third-party dependencies...


SpiderMonkey is already up to date


make: Entering directory '/mnt/data/jenkins-phabricator/workspace/phabricator/build/premake/premake4/build/gmake.unix'
==== Building Premake4 (release) ====
make: Leaving directory '/mnt/data/jenkins-phabricator/workspace/phabricator/build/premake/premake4/build/gmake.unix'

Premake args:  --with-system-nvtt --without-miniupnpc --collada --atlas
Building configurations...
Running action 'gmake'...
Generating ../workspaces/gcc/Makefile...
Generating ../workspaces/gcc/pyrogenesis.make...
Generating ../workspaces/gcc/network.make...
Generating ../workspaces/gcc/tinygettext.make...
Generating ../workspaces/gcc/lobby.make...
Generating ../workspaces/gcc/glooxwrapper.make...
Generating ../workspaces/gcc/simulation2.make...
Generating ../workspaces/gcc/scriptinterface.make...
Generating ../workspaces/gcc/engine.make...
Generating ../workspaces/gcc/graphics.make...
Generating ../workspaces/gcc/atlas.make...
Generating ../workspaces/gcc/gui.make...
Generating ../workspaces/gcc/lowlevel.make...
Generating ../workspaces/gcc/mongoose.make...
Generating ../workspaces/gcc/mocks_real.make...
Generating ../workspaces/gcc/mocks_test.make...
Generating ../workspaces/gcc/AtlasObject.make...
Generating ../workspaces/gcc/AtlasUI.make...
Generating ../workspaces/gcc/ActorEditor.make...
Generating ../workspaces/gcc/Collada.make...
Generating ../workspaces/gcc/test.make...
Done.
Building configurations...
Running action 'codeblocks'...
Generating ../workspaces/codeblocks/pyrogenesis.workspace...
Generating ../workspaces/codeblocks/pyrogenesis.cbp...
Generating ../workspaces/codeblocks/network.cbp...
Generating ../workspaces/codeblocks/tinygettext.cbp...
Generating ../workspaces/codeblocks/lobby.cbp...
Generating ../workspaces/codeblocks/glooxwrapper.cbp...
Generating ../workspaces/codeblocks/simulation2.cbp...
Generating ../workspaces/codeblocks/scriptinterface.cbp...
Generating ../workspaces/codeblocks/engine.cbp...
Generating ../workspaces/codeblocks/graphics.cbp...
Generating ../workspaces/codeblocks/atlas.cbp...
Generating ../workspaces/codeblocks/gui.cbp...
Generating ../workspaces/codeblocks/lowlevel.cbp...
Generating ../workspaces/codeblocks/mongoose.cbp...
Generating ../workspaces/codeblocks/mocks_real.cbp...
Generating ../workspaces/codeblocks/mocks_test.cbp...
Generating ../workspaces/codeblocks/AtlasObject.cbp...
Generating ../workspaces/codeblocks/AtlasUI.cbp...
Generating ../workspaces/codeblocks/ActorEditor.cbp...
Generating ../workspaces/codeblocks/Collada.cbp...
Generating ../workspaces/codeblocks/test.cbp...
Done.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2098/ for more details.

Harbormaster completed remote builds in B3322: Diff 3836.Oct 2 2017, 10:19 AM
bb added inline comments.Nov 10 2017, 1:54 PM
binaries/data/mods/public/gui/session/selection_panels_input.js
1 ↗(On Diff #3836)

Maybe a comment explaining what this file contains would be good.

32 ↗(On Diff #3836)

strictly a unit_action function too

Imarok updated this revision to Diff 4155.Nov 12 2017, 7:41 PM
Imarok marked 2 inline comments as done.

rebase upon rP20441 and fix bb's remarks.

binaries/data/mods/public/gui/session/selection_panels_input.js
32 ↗(On Diff #3836)

true

Vulcan added a comment.Nov 12 2017, 7:46 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Relax-NG validity error : Extra element props in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element variant failed to validate content
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element group has extra content: variant
Relax-NG validity error : Extra element group in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element actor failed to validate content
Vulcan added a comment.Nov 12 2017, 7:47 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/session/input.js
| 205| »   var·target·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'target' to 'undefined'.

binaries/data/mods/public/gui/session/input.js
| 219| »   var·actionInfo·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'actionInfo' to 'undefined'.

binaries/data/mods/public/gui/session/input.js
| 233| »   for·(var·action·of·actions)
|    | [NORMAL] JSHintBear:
|    | 'action' is already defined.

binaries/data/mods/public/gui/session/input.js
| 236| »   »   »   var·r·=·unitActions[action].hotkeyActionCheck(target,·selection);
|    | [NORMAL] JSHintBear:
|    | 'r' is already defined.

binaries/data/mods/public/gui/session/input.js
| 241| »   for·(var·action·of·actions)
|    | [NORMAL] JSHintBear:
|    | 'action' is already defined.

binaries/data/mods/public/gui/session/input.js
| 244| »   »   »   var·r·=·unitActions[action].actionCheck(target,·selection);
|    | [NORMAL] JSHintBear:
|    | 'r' is already defined.

binaries/data/mods/public/gui/session/input.js
| 441| »   »   &&·(ev.button·==·SDL_BUTTON_LEFT·||·ev.button·==·SDL_BUTTON_RIGHT))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/gui/session/input.js
| 471| »   »   »   »   var·rect·=·updateBandbox(bandbox,·ev,·true);
|    | [NORMAL] JSHintBear:
|    | 'rect' is already defined.

binaries/data/mods/public/gui/session/input.js
| 474| »   »   »   »   var·ents·=·getPreferredEntities(Engine.PickPlayerEntitiesInRect(rect[0],·rect[1],·rect[2],·rect[3],·g_ViewedPlayer));
|    | [NORMAL] JSHintBear:
|    | 'ents' is already defined.

binaries/data/mods/public/gui/session/input.js
| 623| »   »   »   »   »   var·queued·=·Engine.HotkeyIsPressed("session.queue");
|    | [NORMAL] JSHintBear:
|    | 'queued' is already defined.

binaries/data/mods/public/gui/session/input.js
| 663| »   »   »   var·dragDeltaX·=·ev.x·-·dragStart[0];
|    | [NORMAL] JSHintBear:
|    | 'dragDeltaX' is already defined.

binaries/data/mods/public/gui/session/input.js
| 664| »   »   »   var·dragDeltaY·=·ev.y·-·dragStart[1];
|    | [NORMAL] JSHintBear:
|    | 'dragDeltaY' is already defined.

binaries/data/mods/public/gui/session/input.js
| 665| »   »   »   var·maxDragDelta·=·16;
|    | [NORMAL] JSHintBear:
|    | 'maxDragDelta' is already defined.

binaries/data/mods/public/gui/session/input.js
| 697| »   »   »   »   var·queued·=·Engine.HotkeyIsPressed("session.queue");
|    | [NORMAL] JSHintBear:
|    | 'queued' is already defined.

binaries/data/mods/public/gui/session/input.js
| 829| »   »   »   »   »   »   var·sptr·=·ev.hotkey.split(".");
|    | [NORMAL] JSHintBear:
|    | 'sptr' is already defined.

binaries/data/mods/public/gui/session/input.js
| 845| »   »   »   var·ent·=·Engine.PickEntityAtPoint(ev.x,·ev.y);
|    | [NORMAL] JSHintBear:
|    | 'ent' is already defined.

binaries/data/mods/public/gui/session/input.js
| 856| »   »   »   »   var·action·=·determineAction(ev.x,·ev.y);
|    | [NORMAL] JSHintBear:
|    | 'action' is already defined.

binaries/data/mods/public/gui/session/input.js
| 871| »   »   »   }
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'default'.

binaries/data/mods/public/gui/session/input.js
| 898| »   »   »   var·ent·=·Engine.PickEntityAtPoint(ev.x,·ev.y);
|    | [NORMAL] JSHintBear:
|    | 'ent' is already defined.
Harbormaster completed remote builds in B3675: Diff 4155.Nov 12 2017, 7:47 PM
bb accepted this revision.Nov 12 2017, 9:45 PM

Seems like easy to fix style issues => done when committing.

binaries/data/mods/public/gui/session/input.js
1234–1236 ↗(On Diff #4155)

its the same as let template = decrement ? undefined : GetTemplateData(trainEntType);
(probably let template = decrement || GetTemplateData(trainEntType); too)

1357 ↗(On Diff #4155)

swap the * to a / and nuke braces

binaries/data/mods/public/gui/session/selection_panels_input.js
3 ↗(On Diff #4155)

sentence too long for 1 line, split

This revision is now accepted and ready to land.Nov 12 2017, 9:45 PM
Imarok updated this revision to Diff 4163.Nov 13 2017, 3:02 PM
Imarok marked 2 inline comments as done.

Apply 2/3 of bb's comment. Also introduce new local var batchedSize

Vulcan added a comment.Nov 13 2017, 3:52 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Relax-NG validity error : Extra element props in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element variant failed to validate content
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element group has extra content: variant
Relax-NG validity error : Extra element group in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element actor failed to validate content
Vulcan added a comment.Nov 13 2017, 3:54 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/session/input.js
| 205| »   var·target·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'target' to 'undefined'.

binaries/data/mods/public/gui/session/input.js
| 219| »   var·actionInfo·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'actionInfo' to 'undefined'.

binaries/data/mods/public/gui/session/input.js
| 233| »   for·(var·action·of·actions)
|    | [NORMAL] JSHintBear:
|    | 'action' is already defined.

binaries/data/mods/public/gui/session/input.js
| 236| »   »   »   var·r·=·unitActions[action].hotkeyActionCheck(target,·selection);
|    | [NORMAL] JSHintBear:
|    | 'r' is already defined.

binaries/data/mods/public/gui/session/input.js
| 241| »   for·(var·action·of·actions)
|    | [NORMAL] JSHintBear:
|    | 'action' is already defined.

binaries/data/mods/public/gui/session/input.js
| 244| »   »   »   var·r·=·unitActions[action].actionCheck(target,·selection);
|    | [NORMAL] JSHintBear:
|    | 'r' is already defined.

binaries/data/mods/public/gui/session/input.js
| 441| »   »   &&·(ev.button·==·SDL_BUTTON_LEFT·||·ev.button·==·SDL_BUTTON_RIGHT))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/gui/session/input.js
| 471| »   »   »   »   var·rect·=·updateBandbox(bandbox,·ev,·true);
|    | [NORMAL] JSHintBear:
|    | 'rect' is already defined.

binaries/data/mods/public/gui/session/input.js
| 474| »   »   »   »   var·ents·=·getPreferredEntities(Engine.PickPlayerEntitiesInRect(rect[0],·rect[1],·rect[2],·rect[3],·g_ViewedPlayer));
|    | [NORMAL] JSHintBear:
|    | 'ents' is already defined.

binaries/data/mods/public/gui/session/input.js
| 623| »   »   »   »   »   var·queued·=·Engine.HotkeyIsPressed("session.queue");
|    | [NORMAL] JSHintBear:
|    | 'queued' is already defined.

binaries/data/mods/public/gui/session/input.js
| 663| »   »   »   var·dragDeltaX·=·ev.x·-·dragStart[0];
|    | [NORMAL] JSHintBear:
|    | 'dragDeltaX' is already defined.

binaries/data/mods/public/gui/session/input.js
| 664| »   »   »   var·dragDeltaY·=·ev.y·-·dragStart[1];
|    | [NORMAL] JSHintBear:
|    | 'dragDeltaY' is already defined.

binaries/data/mods/public/gui/session/input.js
| 665| »   »   »   var·maxDragDelta·=·16;
|    | [NORMAL] JSHintBear:
|    | 'maxDragDelta' is already defined.

binaries/data/mods/public/gui/session/input.js
| 697| »   »   »   »   var·queued·=·Engine.HotkeyIsPressed("session.queue");
|    | [NORMAL] JSHintBear:
|    | 'queued' is already defined.

binaries/data/mods/public/gui/session/input.js
| 829| »   »   »   »   »   »   var·sptr·=·ev.hotkey.split(".");
|    | [NORMAL] JSHintBear:
|    | 'sptr' is already defined.

binaries/data/mods/public/gui/session/input.js
| 845| »   »   »   var·ent·=·Engine.PickEntityAtPoint(ev.x,·ev.y);
|    | [NORMAL] JSHintBear:
|    | 'ent' is already defined.

binaries/data/mods/public/gui/session/input.js
| 856| »   »   »   »   var·action·=·determineAction(ev.x,·ev.y);
|    | [NORMAL] JSHintBear:
|    | 'action' is already defined.

binaries/data/mods/public/gui/session/input.js
| 871| »   »   »   }
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'default'.

binaries/data/mods/public/gui/session/input.js
| 898| »   »   »   var·ent·=·Engine.PickEntityAtPoint(ev.x,·ev.y);
|    | [NORMAL] JSHintBear:
|    | 'ent' is already defined.
Harbormaster completed remote builds in B3679: Diff 4163.Nov 13 2017, 3:54 PM
elexis added a comment.Nov 13 2017, 5:18 PM

Thanks for cleaning this (the functions were definitely in the wrong file and a bit messy) and the reviews.
Having all changes in the commit message would be appreciated.

binaries/data/mods/public/gui/session/selection_panels_input.js
30 ↗(On Diff #4163)

(Sounds like Vector2D could be used above)

1 ↗(On Diff #3836)

@file if you want
Why not selection_panel_helpers?
(Don't forget the native lineending otherwise)

Imarok added inline comments.Nov 13 2017, 7:10 PM
binaries/data/mods/public/gui/session/selection_panels_input.js
30 ↗(On Diff #4163)

Don't think this makes sense without x/z vectors.

Imarok updated this revision to Diff 4171.Nov 13 2017, 7:15 PM
Imarok marked an inline comment as done.

Merge selection_panels_input.js into selection_panels_helpers.js. Also @file comment.

elexis added a comment.Nov 13 2017, 7:39 PM

("originally been batch training" perhaps you want to commit the behavior change separately if it's not too much work)
(no need to wait for my input with the commit)

binaries/data/mods/public/gui/session/selection_panels_helpers.js
3 ↗(On Diff #4171)

no comma I believe.
First two sentences could be merged.

176 ↗(On Diff #4171)

I think your reviewer doesn't like the singular they, could go for "When pressing...".
"another hotkey" should be "another camera jump hotkey"?
(Again not sure about the comma)

180 ↗(On Diff #4171)

(Does it have to be a global? If its used only once, then inlining means one less global to care about and it also means that if we had an entry in the options page, that this doesn't need reloading)

binaries/data/mods/public/gui/session/selection_panels_input.js
30 ↗(On Diff #4163)

So I got +1 on #4834?
The x/y property difference shouldn't stop us from adding good improvements.
Just the question whether it actually is one here. (Perhaps it would even be fun to return a Vector from the engine)
Actually that just looks like the squared distance, but isn't it, so meh

Vulcan added a comment.Nov 13 2017, 8:27 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Relax-NG validity error : Extra element props in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element variant failed to validate content
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element group has extra content: variant
Relax-NG validity error : Extra element group in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element actor failed to validate content
Vulcan added a comment.Nov 13 2017, 8:29 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/session/input.js
| 205| »   var·target·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'target' to 'undefined'.

binaries/data/mods/public/gui/session/input.js
| 219| »   var·actionInfo·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'actionInfo' to 'undefined'.

binaries/data/mods/public/gui/session/input.js
| 233| »   for·(var·action·of·actions)
|    | [NORMAL] JSHintBear:
|    | 'action' is already defined.

binaries/data/mods/public/gui/session/input.js
| 236| »   »   »   var·r·=·unitActions[action].hotkeyActionCheck(target,·selection);
|    | [NORMAL] JSHintBear:
|    | 'r' is already defined.

binaries/data/mods/public/gui/session/input.js
| 241| »   for·(var·action·of·actions)
|    | [NORMAL] JSHintBear:
|    | 'action' is already defined.

binaries/data/mods/public/gui/session/input.js
| 244| »   »   »   var·r·=·unitActions[action].actionCheck(target,·selection);
|    | [NORMAL] JSHintBear:
|    | 'r' is already defined.

binaries/data/mods/public/gui/session/input.js
| 441| »   »   &&·(ev.button·==·SDL_BUTTON_LEFT·||·ev.button·==·SDL_BUTTON_RIGHT))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/gui/session/input.js
| 471| »   »   »   »   var·rect·=·updateBandbox(bandbox,·ev,·true);
|    | [NORMAL] JSHintBear:
|    | 'rect' is already defined.

binaries/data/mods/public/gui/session/input.js
| 474| »   »   »   »   var·ents·=·getPreferredEntities(Engine.PickPlayerEntitiesInRect(rect[0],·rect[1],·rect[2],·rect[3],·g_ViewedPlayer));
|    | [NORMAL] JSHintBear:
|    | 'ents' is already defined.

binaries/data/mods/public/gui/session/input.js
| 623| »   »   »   »   »   var·queued·=·Engine.HotkeyIsPressed("session.queue");
|    | [NORMAL] JSHintBear:
|    | 'queued' is already defined.

binaries/data/mods/public/gui/session/input.js
| 663| »   »   »   var·dragDeltaX·=·ev.x·-·dragStart[0];
|    | [NORMAL] JSHintBear:
|    | 'dragDeltaX' is already defined.

binaries/data/mods/public/gui/session/input.js
| 664| »   »   »   var·dragDeltaY·=·ev.y·-·dragStart[1];
|    | [NORMAL] JSHintBear:
|    | 'dragDeltaY' is already defined.

binaries/data/mods/public/gui/session/input.js
| 665| »   »   »   var·maxDragDelta·=·16;
|    | [NORMAL] JSHintBear:
|    | 'maxDragDelta' is already defined.

binaries/data/mods/public/gui/session/input.js
| 697| »   »   »   »   var·queued·=·Engine.HotkeyIsPressed("session.queue");
|    | [NORMAL] JSHintBear:
|    | 'queued' is already defined.

binaries/data/mods/public/gui/session/input.js
| 829| »   »   »   »   »   »   var·sptr·=·ev.hotkey.split(".");
|    | [NORMAL] JSHintBear:
|    | 'sptr' is already defined.

binaries/data/mods/public/gui/session/input.js
| 845| »   »   »   var·ent·=·Engine.PickEntityAtPoint(ev.x,·ev.y);
|    | [NORMAL] JSHintBear:
|    | 'ent' is already defined.

binaries/data/mods/public/gui/session/input.js
| 856| »   »   »   »   var·action·=·determineAction(ev.x,·ev.y);
|    | [NORMAL] JSHintBear:
|    | 'action' is already defined.

binaries/data/mods/public/gui/session/input.js
| 871| »   »   »   }
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'default'.

binaries/data/mods/public/gui/session/input.js
| 898| »   »   »   var·ent·=·Engine.PickEntityAtPoint(ev.x,·ev.y);
|    | [NORMAL] JSHintBear:
|    | 'ent' is already defined.
Harbormaster completed remote builds in B3685: Diff 4171.Nov 13 2017, 8:29 PM
Imarok marked 3 inline comments as done.Nov 14 2017, 2:13 PM
Imarok added inline comments.
binaries/data/mods/public/gui/session/selection_panels_input.js
30 ↗(On Diff #4163)

So I got +1 on #4834?

Definitely

Closed by commit rP20447: Not so small input.js (originally been batch training) cleanup (authored by Imarok). · Explain WhyNov 14 2017, 2:30 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
public/
gui/
session/
575 lines
2 lines
325 lines
75 lines

Diff 4178

View Options

ps/trunk/binaries/data/mods/public/gui/session/input.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/session/selection_panels.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/session/selection_panels_helpers.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/session/unit_actions.js

Loading...
Wildfire Games · Phabricator