Page MenuHomeWildfire Games
Differential D2397

Do not hardcode "delete" and "focus-rally" in whether a command button is active.
AbandonedPublic
Actions

Authored by Freagarach on Oct 28 2019, 4:11 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

rP18487 Introduced that the "deletable"-button is not enabled when there is no entity to delete by checking for the "delete" command and whether the entity state is undeletable. rP18773 Fixed multiple entities selected by introducing the same check as performed in unit_actions.js.
rP18347 Introduced the "g_Observer" check.
This patch introduces optional enabledPlayer- and enabledObserver-elements in the object returned to selection_panels.js, both removing hardcoding of "delete" and "focus-rally" and allowing other commands to specify whether the button ought to be enabled or disabled (e.g. unloading not allowed when the garrison holder is moving).

Test Plan

Verify that previous behaviour is maintained by:

  • Selecting an undeletable entity and checking that the button is disabled.
  • Selecting a deletable entity and checking that the button is enabled.

Diff Detail

Event Timeline

Freagarach created this revision.Oct 28 2019, 4:11 PM
Owners added a subscriber: Restricted Owners Package.Oct 28 2019, 4:11 PM
Vulcan added a comment.Oct 28 2019, 4:14 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/495/display/redirect

Vulcan added a comment.Oct 28 2019, 4:18 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
| 761| 761| 				addResearchToQueue(data.item.researchFacilityId, t);
| 762| 762| 			})(tech);
| 763| 763| 
| 764|    |-			button.onPressRight = (t => function () {
|    | 764|+			button.onPressRight = (t => function() {
| 765| 765| 				showTemplateDetails(
| 766| 766| 					t,
| 767| 767| 					GetTemplateData(data.unitEntStates.find(state => state.id == data.item.researchFacilityId).template).nativeCiv);

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

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

binaries/data/mods/public/gui/session/selection_panels.js
| 729| »   »   »   »   »   »   switch·(entity.check)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
|    | [NORMAL] ESLintBear (no-extra-semi):
|    | Unnecessary semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/unit_actions.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/unit_actions.js
|1099|1099| 							resumeGame();
|1100|1100| 						}
|1101|1101| 					]);
|1102|    |-			};
|    |1102|+			}
|1103|1103| 		},
|1104|1104| 	},
|1105|1105| 

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

binaries/data/mods/public/gui/session/unit_actions.js
|1102| »   »   »   };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1010/display/redirect

Harbormaster completed remote builds in B9890: Diff 10217.Oct 28 2019, 4:19 PM
elexis added a subscriber: elexis.Oct 28 2019, 4:48 PM

I guess you mean true and false, not "true" and "false". You can check for === undefined if you want default = true.
Active may be renamed to enabled, because it will be assigned to the enabled property of the GUIObject (i.e. less indirection, less vocabulary used to describe the same behavior).
The patch appears correct, without having it tested.
Also it appears like it's doing one less loop through all selected entities, i.e. a performance improvement. Can measure rudimentarily with let foo = Engine.GetMicroseconds(); ...; warn(Engine.GetMicroseconds() - foo);, or with those profiler commands I never tried Engine.ProfileStart("Trade Manager");, Engine.ProfileStop();.

Also its good to have this delete hardcoding removed. Consequentially the next question is what to do with data.item.name == "focus-rally"; in particular whether one can removed that as well without adding lots of function calls.

Freagarach updated this revision to Diff 10223.Oct 29 2019, 5:51 PM

active - enabled.

Vulcan added a comment.Oct 29 2019, 5:52 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/498/display/redirect

Vulcan added a comment.Oct 29 2019, 5:56 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-extra-semi):
|    | Unnecessary semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/unit_actions.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/unit_actions.js
|1099|1099| 							resumeGame();
|1100|1100| 						}
|1101|1101| 					]);
|1102|    |-			};
|    |1102|+			}
|1103|1103| 		},
|1104|1104| 	},
|1105|1105| 

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

binaries/data/mods/public/gui/session/unit_actions.js
|1102| »   »   »   };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
| 761| 761| 				addResearchToQueue(data.item.researchFacilityId, t);
| 762| 762| 			})(tech);
| 763| 763| 
| 764|    |-			button.onPressRight = (t => function () {
|    | 764|+			button.onPressRight = (t => function() {
| 765| 765| 				showTemplateDetails(
| 766| 766| 					t,
| 767| 767| 					GetTemplateData(data.unitEntStates.find(state => state.id == data.item.researchFacilityId).template).nativeCiv);

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

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

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1013/display/redirect

Harbormaster completed remote builds in B9899: Diff 10223.Oct 29 2019, 5:57 PM
elexis added a comment.Oct 29 2019, 6:00 PM

or !!data.item.enabled

it would be good to move the focus-rally too without adding too many function calls

Freagarach added a comment.Oct 29 2019, 7:05 PM

Yeah, one thing at a time xD I was still figuring how that could be achieved.

Freagarach edited the summary of this revision. (Show Details)Oct 31 2019, 9:18 AM
Stan added a subscriber: Stan.Oct 31 2019, 3:39 PM
Stan added inline comments.
binaries/data/mods/public/gui/session/selection_panels.js
138

This check sounds somehow redundant. Any way to make it true/false instead of tristate ?

Silier added a subscriber: Silier.Oct 31 2019, 5:55 PM
Silier added inline comments.
binaries/data/mods/public/gui/session/selection_panels.js
138

if enabled == true is wanted default behaviour, one can invert and do disabled property instead or write this one like
!(data.item.enabled == false), but I would prefer to see first mentioned variant just for clarification of the condition

Freagarach updated this revision to Diff 10284.Nov 7 2019, 8:21 PM
Freagarach retitled this revision from Do not hardcode "delete" in whether a command button is active. to Do not hardcode "delete" and "focus-rally" in whether a command button is active..
Freagarach edited the summary of this revision. (Show Details)

Include "focus-rally".

Vulcan added a comment.Nov 7 2019, 8:23 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/545/display/redirect

Vulcan added a comment.Nov 7 2019, 8:29 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
| 762| 762| 				addResearchToQueue(data.item.researchFacilityId, t);
| 763| 763| 			})(tech);
| 764| 764| 
| 765|    |-			button.onPressRight = (t => function () {
|    | 765|+			button.onPressRight = (t => function() {
| 766| 766| 				showTemplateDetails(
| 767| 767| 					t,
| 768| 768| 					GetTemplateData(data.unitEntStates.find(state => state.id == data.item.researchFacilityId).template).nativeCiv);

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

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

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

binaries/data/mods/public/gui/session/unit_actions.js
| 557| »   »   »   switch·(tradingDetails.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1059/display/redirect

Harbormaster completed remote builds in B9968: Diff 10284.Nov 7 2019, 8:29 PM
Freagarach updated this revision to Diff 10288.Nov 8 2019, 8:56 AM
  • Less redundant checking.
  • Ternary for observer check.
  • Back to checking for undefined explicitly.
Vulcan added a comment.Nov 8 2019, 8:57 AM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/548/display/redirect

Vulcan added a comment.Nov 8 2019, 9:00 AM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
| 760| 760| 				addResearchToQueue(data.item.researchFacilityId, t);
| 761| 761| 			})(tech);
| 762| 762| 
| 763|    |-			button.onPressRight = (t => function () {
|    | 763|+			button.onPressRight = (t => function() {
| 764| 764| 				showTemplateDetails(
| 765| 765| 					t,
| 766| 766| 					GetTemplateData(data.unitEntStates.find(state => state.id == data.item.researchFacilityId).template).nativeCiv);

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

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

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

binaries/data/mods/public/gui/session/unit_actions.js
| 557| »   »   »   switch·(tradingDetails.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1062/display/redirect

Harbormaster completed remote builds in B9972: Diff 10288.Nov 8 2019, 9:00 AM
elexis added inline comments.Nov 9 2019, 2:38 PM
binaries/data/mods/public/gui/session/selection_panels.js
139

Some repetition:

2019-10-29-QuakeNet-#0ad-dev.log:18:07 < elexis> (an observerEnabled playerEnabled might be one way to do it, but only one enabled property sounds nicer)
2019-11-07-QuakeNet-#0ad-dev.log:18:53 < elexis> I thought of enabledPlayer and enabledObserver
for the review:
2019-11-07-QuakeNet-#0ad-dev.log:20:55 < elexis> what's the difference between data.item.enabledObserver and data.item.enabledObserver == true?

I think the answer to the last one is that enabled assignment will show a warning if it receives a value that is not of type boolean? In that case its more common to use !! (although Im not so sure if Boolean() wouldn't appear cleaner some time in the future)

Less redundant checking

i.e. less or none?

elexis added a comment.Nov 9 2019, 2:42 PM

Or just !== false

Freagarach added a child revision: D2396: Support player assigning idle villagers with a command button..Nov 14 2019, 6:55 PM
Freagarach added a comment.Nov 15 2019, 9:35 AM

D2343 Would make the enabledObserver thing unnecessary.

Freagarach added a child revision: D2343: Combine "EntityCommand" and "AllyEntityCommand"..Nov 24 2019, 9:54 PM
Freagarach abandoned this revision.Nov 25 2019, 9:28 AM
Freagarach mentioned this in D2343: Combine "EntityCommand" and "AllyEntityCommand"..

Merged in D2343.

Freagarach removed a child revision: D2343: Combine "EntityCommand" and "AllyEntityCommand"..Nov 25 2019, 9:28 AM
Freagarach removed a child revision: D2396: Support player assigning idle villagers with a command button..

Revision Contents
Changeset List

PathSize
binaries/
data/
mods/
public/
gui/
session/
7 lines
3 lines

Diff 10288

View Options

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

Loading...
View Options

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

Loading...
Wildfire Games · Phabricator