Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
The reason that the warehouse statistics don't list them is that people might try to set a policy on them. Players might still be interested in the inventory though. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Good question! In the case of workers, this is not required. Now that you are mentioning it I remembered (and testing confirmed) that this is dealt with somewhere else. When destroying a building (and its queues) wares are lost, so we have to remove them from the economy. On the other hand, workers become fugitives. When destroying an unconnected (i.e. no streets) barracks the inventory statistics no longer show the workers of the barracks. When the fugitives find a connected street they are added again. I haven't looked in the code but I would guess they take care of that themselves. While testing I noticed that the inventory statistics are listing no-cost workers while the warehouse statistics are not. Is this a bug or intentional behavior? -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
This is WaresQueue does in the cleanup: if (filled_ && owner_.get_economy()) owner_.get_economy()->remove_wares(index_, filled_); filled_ = 0; etc... I guess we should to the same for the workers queue now, since it's now more like the waresqueue and not like the soldercontrol? -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Seems like the cleanup-bug is fixed. The problem was that the WorkersQueue was free-ing the stored workers on cleanup while the building was also free-ing them. The fix is to simply drop the pointers to the workers when removing the queue and let the building handle them. However, I don't really like the fix. For one thing, I could not really figure out which methods are called at which time during cleanup. The code I looked at does not matches my observations while debugging. Most likely this is a misunderstanding by me and no bug in the code, though. The more serious problem is the sporadic nature of the bug. Neither my understanding of the bug nor the fix are accounting for the fact that the bug only occurs sometimes. It could happen when some code (which one?) sets the to-be-freed pointers to some value (which and why?) that happens to be NULL most of the time. Not that this makes any sense for me. Another possibility could be a random order of cleanup on exit. So while the bug seems to be fixed I don't understand it and there might be a memory leak present now. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Continuous integration builds have changed state: Travis build 1697. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/180993235. Appveyor build 1537. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_casern_workersqueue-1537. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Continuous integration builds have changed state: Travis build 1660. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/179421689. Appveyor build 1500. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_casern_workersqueue-1500. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Bunnybot encountered an error while working on this merge proposal:
('The read operation timed out',)
--
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.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Thank you both for looking at this. When looking sooner in the travis log I found these lines: test/maps/plain.wmf/scripting/test_casern.lua ... Running Widelands ... *** Error in `build/src/widelands': double free or corruption (!prev): 0x0670d1e0 *** FAIL So you are right SirVer: A double free in any case. But I would guess some memory corruption, too, since it does not appear every time. Through running the test a lot of times I was able to get the following backtrace. Seems like the CmdQueue has problems when cleaning up on program exit. [...] MO(7,barbarians_barracks): Recruit leaving MO(2,barbarians_lumberjacks_hut): Lumberjack leaving Thread 1 "widelands" received signal SIGSEGV, Segmentation fault. 0x7518b8c0 in _int_free (av=0x754acb00 , p=0x5d89b6e0, have_lock=0) at malloc.c:4049 4049malloc.c: No such file or directory. A debugging session is active. Inferior 1 [process 6294] will be killed. Quit anyway? (y or n) n Not confirmed. (gdb) bt #0 0x7518b8c0 in _int_free (av=0x754acb00 , p=0x5d89b6e0, have_lock=0) at malloc.c:4049 #1 0x55ff2ec4 in __gnu_cxx::new_allocator::deallocate (this=0x58b2aae0, __p=0x5d89b6f0) at /usr/include/c++/6/ext/new_allocator.h:110 #2 0x55ff2828 in std::allocator_traits<std::allocator >::deallocate (__a=..., __p=0x5d89b6f0, __n=8) at /usr/include/c++/6/bits/alloc_traits.h:442 #3 0x55ff1efe in std::_Vector_base<Widelands::CmdQueue::CmdItem, std::allocator >::_M_deallocate (this=0x58b2aae0, __p=0x5d89b6f0, __n=8) at /usr/include/c++/6/bits/stl_vector.h:178 #4 0x55ff19bd in std::_Vector_base<Widelands::CmdQueue::CmdItem, std::allocator >::~_Vector_base (this=0x58b2aae0, __in_chrg=) at /usr/include/c++/6/bits/stl_vector.h:160 #5 0x55ff16b9 in std::vector<Widelands::CmdQueue::CmdItem, std::allocator >::~vector (this=0x58b2aae0, __in_chrg=) at /usr/include/c++/6/bits/stl_vector.h:427 #6 0x55ff1658 in std::priority_queue<Widelands::CmdQueue::CmdItem, std::vector<Widelands::CmdQueue::CmdItem, std::allocator >, std::less >::~priority_queue (this=0x58b2aae0, __in_chrg=) at /usr/include/c++/6/bits/stl_queue.h:397 #7 0x55ff30d1 in std::_Destroy<std::priority_queue<Widelands::CmdQueue::CmdItem, std::vector<Widelands::CmdQueue::CmdItem, std::allocator >, std::less > > (__pointer=0x58b2aae0) at /usr/include/c++/6/bits/stl_construct.h:93 #8 0x55ff2c34 in std::_Destroy_aux::__destroy<std::priority_queue<Widelands::CmdQueue::CmdItem, std::vector<Widelands::CmdQueue::CmdItem, std::allocator >, std::less >*> (__first=0x58b2aae0, __last=0x58c84ae0) at /usr/include/c++/6/bits/stl_construct.h:103 #9 0x55ff21d7 in std::_Destroy<std::priority_queue<Widelands::CmdQueue::CmdItem, std::vector<Widelands::CmdQueue::CmdItem, std::allocator >, std::less >*> (__first=0x58a84ae0, __last=0x58c84ae0) at /usr/include/c++/6/bits/stl_construct.h:126 #10 0x55ff1c27 in std::_Destroy<std::priority_queue<Widelands::CmdQueue::CmdItem, std::vector<Widelands::CmdQueue::CmdItem, std::allocator >, std::less >*, std::priority_queue<Widelands::CmdQueue::CmdItem, std::vector<Widelands::CmdQueue::CmdItem, std::allocator >, std::less > > (__first=0x58a84ae0, __last=0x58c84ae0) at /usr/include/c++/6/bits/stl_construct.h:151 #11 0x55ff182b in std::vector<std::priority_queue<Widelands::CmdQueue::CmdItem, std::vector<Widelands::CmdQueue::CmdItem, std::allocator >, std::less >, std::allocator<std::priority_queue<Widelands::CmdQueue::CmdItem, std::vector<Widelands::CmdQueue::CmdItem, std::allocator >, std::less > > >::~vector (this=0x7fffd5a0, __in_chrg=) at /usr/include/c++/6/bits/stl_vector.h:426 #12 0x55ff0c2e in Widelands::CmdQueue::~CmdQueue (this=0x7fffd590, __in_chrg=) at ../src/logic/cmd_queue.cc:46 #13 0x55eda50e in Widelands::Game::~Game (this=0x7fffd3c0, __in_chrg=) at ../src/logic/game.cc:133 #14 0x55dc3343 in WLApplication::run (this=0x56ae3a90) at ../src/wlapplication.cc:423 #15 0x55dc1a1d in main (argc=10, argv=0x7fffd888) at ../src/main.cc:49 -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Continuous integration builds have changed state: Travis build 1648. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/178540332. Appveyor build 1487. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_casern_workersqueue-1487. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Oh, forgot a comment: Priorities for WorkerQueues are and will stay disabled. I am not completely sure what priorities are doing, but they only seem to influence how likely it is that a carrier picks up a ware on a flag. Obviously, this does not make sense for workers. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Replacing calls to WaresQueue with calls to the new InputQueue is done in another branch: https://code.launchpad.net/~widelands-dev/widelands/refactoring-input-queue It can be merged either in this branch or into trunk after this branch is merged. Adding a workersQueue for the builder on expeditions is not done yet. I plan to do so after the this branch and the refactoring branch are merged into trunk (don't like to create a branch for the bug in trunk which depends on other branches). Review comments from the first review are done. Additionally, I created a regression test for the caserns, since it is the only building using the WorkersQueue currently. In r7434 I fixed a bug which crashes scripts when they try to add a worker-worker to a building which already contains input-workers. Should have probably become an own branch, but the fix is small and I was lazy. So as far as I am concerned: The branch(es) is(/are) ready for the next review. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Continuous integration builds have changed state: Travis build 1611. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/175973829. Appveyor build 1449. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_casern_workersqueue-1449. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Continuous integration builds have changed state: Travis build 1608. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/175798398. Appveyor build 1446. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_casern_workersqueue-1446. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
I am always in favour of removing code duplication wherever possible, so please go ahead if you want. If it's older code, you can also do that in a follow-up branch to keep the current diff smaller. Regarding the builder, we actually have an open bug for that, so yes, please :) https://bugs.launchpad.net/widelands/+bug/1191295 Maybe also in a follow-up branch? -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Continuous integration builds have changed state: Travis build 1604. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/175631314. Appveyor build 1442. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_casern_workersqueue-1442. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Continuous integration builds have changed state: Travis build 1603. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/175565469. Appveyor build 1441. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_casern_workersqueue-1441. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Thanks for the answers earlier. Wares- and WorkerQueues now have a common interface so they can be used interchangeable. Note that the code is not really tested yet. Both are using the same gui class for display, list on the first tab. Support for priorities on the graphical worker queues is disabled currently, I have to look into it if that makes sense (and re-enable it when it does). Since the classes are more or less the same now, it should be possible to replace the current Building::waresqueue(index) and Building::workersqueue(index) with a single Building::inputqueue(index, type) respectively a single ProductionSite::inputqueues() which returns both types of queues. This would clean up some code duplications where the same code is run on both queues. If no-one protests I will probably do so. Another, minor thing: I haven't looked into it, but when creating an expedition a builder is requested (at least, I assume he is). Now that we have worker queues: Should there be one for the builder in the tab of the port? -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Continuous integration builds have changed state: Travis build 1597. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/175308095. Appveyor build 1435. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_casern_workersqueue-1435. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Oops - definitely needs increasing to 6. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
GunChleoc, can you take a look at you "versioning" changes in
map_buildingdata_packet.cc, please? The current version does not pass the
regression tests.
The problem is the line:
if (packet_version > 5) {
Since kCurrentPacketVersionProductionsite is still set to 5, the code differs
on read and write. I would guess that kCurrentPacketVersionProductionsite
should have been increased to 6 but I am not sure enough about it to dare to
fix it myself. Can you fix it or tell me to do it?
--
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.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Sorry, overlooked your comment - think now that the UI change should go in before the merge, because the code changes for that will be rather big, and we should get a superclass for wares and workers queues. For the documentation, see https://bazaar.launchpad.net/~widelands-dev/widelands/trunk/view/head:/doc/sphinx/README Best generate as HTML, so you won't need the website project to view. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
First round of code review is done - see my last commit. I will have to look into your economy questions - I'm not really familiar with the economy code myself yet. Great contribution overall! I have mostly some minors nits and refactoring ideas - feel free to drop any of those if you don't agree or turn them into TODO comments if you think the changes would be too big for this branch. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
I split up the documentation and updated some references. Now I am only wondering: Is there a script which extracts this documentation somewhere? I only found some generic documentation but no list of provided Lua classes or something like that. What else is to do in this branch? Will the UI be reworked before a merge to trunk or merge first and do the update later? -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
The documentation is not dead. Various classes implement this interface and refer to this as documentation. We can delete this, but then we have to add the same documentation to all classes that implement the semantics. As long as they do not diverge, I think having them only documented once is preferable. Or in other terms: the class exists for users, just not in the implementation- and that is an implementation detail. > Am 08.11.2016 um 09:48 schrieb GunChleoc <f...@foramnagaidhlig.net>: > > I think we should delete the documentation then - no need to have dead code > documented, it is confusing. > -- > https://code.launchpad.net/~widelands-dev/widelands/casern_workersqueue/+merge/309763 > You are subscribed to branch lp:widelands. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
I think we should delete the documentation then - no need to have dead code documented, it is confusing. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
> Is this something which really exists in the code or documentation-only to > avoid writing the same stuff over and over again? The latter. I think it used to exist, but now it is just an interface without any code associated with it. Splitting up sounds fine to me. I do not think we can give stability guarantees for the Lua interface at this point in time, so no need to try hard to keep it backwards compatible. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Thanks Travis but I already knew that the test were broken. They should be fixed now. However, I have a problem with the scripting documentation. What is the "HasWares" interface which is described in lua_map.cc? Is this something which really exists in the code or documentation-only to avoid writing the same stuff over and over again? Either case, it does no longer really match the *_input methods of production sites. I was thinking about splitting it up into a HasWares interface for flags and warehouses and create a nearly identical interface HasInputs which is for production and training sites and also accepts workers as inputs. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Continuous integration builds have changed state: Travis build 1567. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/172859367. Appveyor build 1406. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_casern_workersqueue-1406. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Bunnybot encountered an error while working on this merge proposal:
('The read operation timed out',)
--
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.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
