Fix rP18578. (repair hotkey used build as command)
Removes the build command, as its not really needed anywhere.
Details
- Reviewers
elexis sanderd17 - Commits
- rP20250: Fix repair hotkey and remove build command
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
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.
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
Don't mix up sim commands with gui commands. (The gui command "build" existed at this time.)
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.
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.
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.
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.
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.
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.