Page MenuHomeWildfire Games

Fix repair hotkey and remove build command
ClosedPublic

Authored by Imarok on Apr 18 2017, 3:45 PM.

Details

Summary

Fix rP18578. (repair hotkey used build as command)
Removes the build command, as its not really needed anywhere.

Test Plan

Test everything works as before and that the build command is really not needed.

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

Imarok created this revision.Apr 18 2017, 3:45 PM
Vulcan added a subscriber: Vulcan.Apr 18 2017, 4:29 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

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

elexis added a subscriber: elexis.Apr 18 2017, 5:19 PM

Test plan test? Perhaps we should also review the commit that broke it and the broken guard hotkey as mentioned in the proposal where you found out about this bug.

Not sure how that has passed the review at the time.
The "build" cmmand didn't exist at the time either:
https://code.wildfiregames.com/source/0ad/browse/ps/trunk/binaries/data/mods/public/simulation/helpers/Commands.js;18578

So looks ok to me, but review should be complete before we have to spawn another fix

In D358#14032, @elexis wrote:

Don't mix up sim commands with gui commands. (The gui command "build" existed at this time.)

Imarok planned changes to this revision.Apr 19 2017, 11:04 AM
Imarok added inline comments.
binaries/data/mods/public/gui/session/unit_actions.js
417 ↗(On Diff #1331)

I guess this is the cause, why I used "build".
(This mistake is there since the creation of unit_actions.js in rP15336)

Imarok updated this revision to Diff 1365.Apr 19 2017, 7:34 PM

Fix the part from rP15336

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

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

elexis accepted this revision.May 1 2017, 7:33 AM

Correct.

Tested this and in the fourth game I actually got into a situation where I could test the hotkey (since the only action on foundations and structures wth 100% capture points is repairing, since rams can't be repaired, and since the AI send an attack order each time I wanted to capture something).

So the bug was only noticed by people who wanted to repair their siege engine before restoring all capture points.

This revision is now accepted and ready to land.May 1 2017, 7:33 AM
elexis requested changes to this revision.May 1 2017, 7:51 AM

Actually it works with and without the patch.
I think you still need to return the GUI action and not the simulation command name (since commands are sent as a whole, not only the name and input.js looks like it would just call the execute function of this gui action) and that this patch only works because it calls the repair GUI action which is basically a duplicate of the build one.
Don't see the problem actually.

This revision now requires changes to proceed.May 1 2017, 7:51 AM
In D358#16624, @elexis wrote:

Actually it works with and without the patch.

Talking about D308?

I think you still need to return the GUI action and not the simulation command name (since commands are sent as a whole, not only the name and input.js looks like it would just call the execute function of this gui action) and that this patch only works because it calls the repair GUI action which is basically a duplicate of the build one.

This still returns the GUI action and not the sim command. And the build and the repair command are no duplicates, but just really similar.

Don't see the problem actually.

The problem is that a build command is issued when the user does a repair command. This worked well until now, because those two commands share some similarities. Though doing it like this is just wrong and furthermore doesn't work with D308.

Imarok retitled this revision from Fix repair hotkey to Fix repair hotkey and remove build command..May 12 2017, 4:40 PM
Imarok edited the summary of this revision. (Show Details)
Imarok edited the test plan for this revision. (Show Details)
Imarok updated this revision to Diff 1878.May 12 2017, 4:40 PM
Imarok edited edge metadata.
Imarok retitled this revision from Fix repair hotkey and remove build command. to Fix repair hotkey.
Imarok edited the summary of this revision. (Show Details)
Imarok edited the test plan for this revision. (Show Details)

remove build command

Imarok retitled this revision from Fix repair hotkey to Fix repair hotkey and remove build command.May 12 2017, 4:42 PM
Imarok edited the summary of this revision. (Show Details)
Imarok edited the test plan for this revision. (Show Details)

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!

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

Imarok added a subscriber: sanderd17.

Maybe @sanderd17 has an opinion on that, because afaics he introduced those two commands...

Looks good to me. The removed hunk is almost identical to the existing one and afaik sending a build command is just placing a building and automatically queueing a repair command do that. So merging those seems good.

This revision was automatically updated to reflect the committed changes.