Details

Reviewers

Imarok
elexis

Commits

rP19786: Prevent unintentional selection of all units following rP19444 / D326 when…

Trac Tickets

#4631

Summary

As reported in #4631 there are 2 ways to unintentionally select all units of some type after placing a foundation with a doubleclick.

This can be really bad in case we wanted to delete the foundation right afterwards and then accidentally deleted all units of some type.

A second bug: if you click on the unit and give it a preselected action (repair, patrol, guard) and double click as you perform the preselected action, you will see the same bug. The core problem is that clickedEntity is associated with a click event and any double or triple clicks that follow it, but is being retained for the next click event. It's supposed to get reset but that only happens if the next single click event's inputState is INPUT_NORMAL

Test Plan

See the two reproduce recipes in the ticket.

Also verify the similar bug that if you double click to guard, patrol, or repair, it doesn't select all units of the previously clicked type.

Also verify that double and triple clicking still work as normal to select units.

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

elexis created this revision.Jun 13 2017, 4:12 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1549/ for more details.

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
| 267| 267| 	if (!g_DevSettings.controlAll && !allOwnedByPlayer)
| 268| 268| 		return undefined;
| 269| 269| 
| 270|    |-	var target = undefined;
|    | 270|+	var target;
| 271| 271| 	if (!fromMinimap)
| 272| 272| 	{
| 273| 273| 		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
| 281| 281| 	var actions = Object.keys(unitActions).slice();
| 282| 282| 	actions.sort((a, b) => unitActions[a].specificness - unitActions[b].specificness);
| 283| 283| 
| 284|    |-	var actionInfo = undefined;
|    | 284|+	var actionInfo;
| 285| 285| 	if (preSelectedAction != ACTION_NONE)
| 286| 286| 	{
| 287| 287| 		for (var action of actions)

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

binaries/data/mods/public/gui/session/input.js
| 228| »   »   var·entState·=·GetExtendedEntityState(entityID);
|    | [NORMAL] JSHintBear:
|    | 'entState' is already defined.

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

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

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

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

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

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

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

binaries/data/mods/public/gui/session/input.js
| 503| »   »   &&·(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
| 533| »   »   »   »   var·rect·=·updateBandbox(bandbox,·ev,·true);
|    | [NORMAL] JSHintBear:
|    | 'rect' is already defined.

binaries/data/mods/public/gui/session/input.js
| 536| »   »   »   »   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
| 685| »   »   »   »   »   var·queued·=·Engine.HotkeyIsPressed("session.queue");
|    | [NORMAL] JSHintBear:
|    | 'queued' is already defined.

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

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

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

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

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

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

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

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

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

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

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

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

binaries/data/mods/public/gui/session/input.js
|1168| »   if(getEntityLimitAndCount(playerState,·buildTemplate).canBeAddedCount·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/input.js
|1340| »   var·batchTrainingPossible·=·limits.canBeAddedCount·==·undefined·||·limits.canBeAddedCount·>·1;
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with 'undefined'.

binaries/data/mods/public/gui/session/input.js
|1370| »   »   »   »   else·if·(limits.canBeAddedCount·==·undefined·||
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with 'undefined'.

binaries/data/mods/public/gui/session/input.js
|1374| »   »   »   »   »   »   multiplyEntityCosts(template,·g_BatchTrainingCount·+·batchIncrementSize)·}))
|    | [NORMAL] JSHintBear:
|    | 'template' used out of scope.

binaries/data/mods/public/gui/session/input.js
|1392| »   »   »   multiplyEntityCosts(template,·batchIncrementSize)·}))
|    | [NORMAL] JSHintBear:
|    | 'template' used out of scope.

binaries/data/mods/public/gui/session/input.js
|1447| »   if·((limits.canBeAddedCount·==·undefined·||
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with 'undefined'.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/190/ for more details.

Harbormaster completed remote builds in B2270: Diff 2543.Jun 13 2017, 5:01 AM

elexis added a reviewer: Imarok.Jun 13 2017, 8:12 PM

Imarok accepted this revision.Jun 14 2017, 6:32 PM

This revision is now accepted and ready to land.Jun 14 2017, 6:32 PM

This doesn't cover all cases. If you click on the unit and give it a preselected action (repair, patrol, guard) and double click as you perform the preselected action, you will see the same bug. The core problem is that clickedEntity is associated with a click event and any double or triple clicks that follow it, but is being retained for the next click event. It's supposed to get reset but that only happens if the next single click event's inputState is INPUT_NORMAL

If we handle INPUT_PRESELECTEDACTION as a separate case here, then if someone later adds another inputState handled after GUI, the same bug will show up again. Therefore I think the solution is not to change the code for building placements or preselected actions, and instead always reset clickedEntity unless the inputState is either INPUT_SELECTING or INPUT_NORMAL.

Not sure how to attach another patch to the revision. I would suggest this change instead of the current one:

Index: binaries/data/mods/public/gui/session/input.js
===================================================================
--- binaries/data/mods/public/gui/session/input.js	(revision 19728)
+++ binaries/data/mods/public/gui/session/input.js	(working copy)
@@ -831,6 +831,9 @@
 		updateAdditionalHighlight();
 	}
 
+	if (inputState != INPUT_NORMAL && inputState != INPUT_SELECTING)
+		clickedEntity = INVALID_ENTITY;
+
 	// State-machine processing:
 
 	switch (inputState)

causative requested changes to this revision.Jun 15 2017, 11:11 AM

This revision now requires changes to proceed.Jun 15 2017, 11:11 AM

elexis agreed it would be best for me to commandeer this

This revision is now accepted and ready to land.Jun 16 2017, 2:51 AM

Fix it in a different way that also fixes the bug with INPUT_PRESELECTEDACTION and is more resilient to future changes.

causative edited the summary of this revision. (Show Details)Jun 16 2017, 2:55 AM

causative edited the test plan for this revision. (Show Details)

Thanks, that patch is better indeed as it restricts all unwanted cases.
Since this function is called only on input events, it's not an issue if it is called too often. Calling it exactly if it's needed would require copying this line to multiple places, so this should be the optimum.

Checking for INPUT_NORMAL and INPUT_SELECTING is correct (first case trivial, second case occurs after the first mousedown). (The patch is complete because the entity is cleared in all except the two relevant cases)

Would be a great point to also add a short JSdoc comment to that variable explaining the doubeclickbehavior new in alpha 22, so that future devs won't break or remove it unintentionally.

added a comment

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1565/ for more details.

Harbormaster completed remote builds in B2270: Diff 2543.Jun 16 2017, 5:02 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1566/ for more details.

Harbormaster completed remote builds in B2302: Diff 2577.Jun 16 2017, 6:45 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1567/ for more details.

Harbormaster completed remote builds in B2303: Diff 2578.Jun 16 2017, 7:31 AM

remove details from comment

Good, that shoud help future readers of that file to grasp (and not break) the doubleclick behavior

Closed by commit rP19786: Prevent unintentional selection of all units following rP19444 / D326 when… (authored by elexis). · Explain WhyJun 16 2017, 1:06 PM

This revision was automatically updated to reflect the committed changes.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1568/ for more details.

Harbormaster completed remote builds in B2305: Diff 2579.Jun 16 2017, 1:41 PM

Fix doubleclick building selection edge case
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 2580

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

Fix doubleclick building selection edge caseClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 2580

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

Fix doubleclick building selection edge case
ClosedPublic
Actions

Revision Contents
Changeset List