widelands-dev team mailing list archive

[Notabilis <notabilis27@xxxxxx>]

Nooo, not the beer! I liked the beer! :(

But thanks for the review. The small stuff (renaming, etc.) is done, what is left are the bigger changes. Open problems in no particular order:

1) Making WorkersQueue more similar to WaresQueue and replacing the user interface should be no problem. I will try to share as much code as possible between the queues respectively the interfaces.

2) The problem with higher-ranking workers seems to be a bug (or inconsistency) in the code. While warehouses check for an exact match to fulfill the request, the IdleWorkerSupply uses can_act_as(). This explains the strange behavior I encountered. For normal (worker-)workers I would prefer the can_act_as() approach while barracks should match exactly. What do you think about a flag in the request which describes whether the worker has to match exactly? Or maybe expand the "Requirements" for requests (new RequireExactWorker class or so)?

3) In production_program.cc:220 you requested a for-each loop. I first thought this would be a problem with the if() inside the loop which checks the iterator. But now I am wondering: Can this if() ever be fulfilled? When I am not missing anything the loop should always end earlier. So remove the if()-part and use a for-each loop?

4) What does "NOCOM" mean? I just can't figure it out. And what is the difference to "TODO"?

5) You increased the packet version for the serialization functions and they are now checking for a range. What is the idea behind it? Increase the number on every modification of the file but accept older versions until the method itself changes?

So much for now.
Thanks for the link to the documentation. I tried the parameter singlehtml before, seems that this does not include the code documentation. A full html worked fine.
-- 
https://code.launchpad.net/~widelands-dev/widelands/casern_workersqueue/+merge/309763
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands.