Code review comment for lp:~victor-pelt/widelands/workers

Revision history for this message
Nicolai Hähnle (nha) wrote :

1. In Player::evict:
1a. the bulldoze parameter is meaningless
1b. the check should be PCap_Evict instead of PCap_Bulldoze
1c. never throw an exception in code that is triggered from PlayerCommands, otherwise a rogue client could cause a server crash. So just print a diagnostic log() instead of the exception (also, there is a typo in the message)

2. Cmd_Evict:
2a. again, the recurse parameter makes no sense
2b. what's the point of starting a newly created command save format at version 2?

3. I am also curious about what the part in the worker program is good for.

4. You typo'd "bulldoze" in a file name. (Ironically, I did the exact same refactoring of BulldozeConfirm in my branch lp:~nha/widelands/refactor-building-ui; too bad I didn't see your changes before, we'll have to do some manual merging there)

5. In your wui changes, the PCap_Evict check is commented out, respectively incorrectly a PCap_Bulldoze check in the EvictConfirm dialog.

review: Needs Fixing

« Back to merge proposal