[Widelands-dev] [Merge] lp:~notabilis27/widelands/bug-mousewheel into lp:widelands

2016-07-10 Thread Notabilis
Notabilis has proposed merging lp:~notabilis27/widelands/bug-mousewheel into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1386740 in widelands: "Fix mousewheel function" https://bugs.launchpad.net/widelands/+bug/1386740 For more de

[Widelands-dev] [Merge] lp:~notabilis27/widelands/bug-tab-crash into lp:widelands

2016-07-10 Thread Notabilis
Notabilis has proposed merging lp:~notabilis27/widelands/bug-tab-crash into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1600633 in widelands: "Crash when pressing tab key in help menu" https://bugs.launchpad.net/widelands/+bug/1600633

[Widelands-dev] [Merge] lp:~notabilis27/widelands/casern into lp:widelands

2016-11-01 Thread Notabilis
The proposal to merge lp:~notabilis27/widelands/casern into lp:widelands has been updated. Description changed to: Implements a worker queue which allows production buildings to consume workers as input. Adds: - Adds worker queue for production buildings. - The "inputs" in the lua files for the

[Widelands-dev] [Merge] lp:~notabilis27/widelands/casern into lp:widelands

2016-11-01 Thread Notabilis
Notabilis has proposed merging lp:~notabilis27/widelands/casern into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1075562 in widelands: "Implement a casern for soldier recruting" https://bugs.launchpad.net/widelands/+bug/1075562 For mo

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands

2016-11-07 Thread Notabilis
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 w

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands

2016-11-08 Thread Notabilis
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

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands

2016-11-09 Thread Notabilis
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 wil

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands

2016-11-12 Thread Notabilis
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

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands

2016-11-13 Thread Notabilis
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 dis

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/toolbar_cleanup into lp:widelands

2016-11-20 Thread Notabilis
Review: Needs Fixing Not sure how to do a code review, but lets try. 1) I looked at r8139, so the two newest commits aren't tested by me. But I looked at the diffs, they seem to be okay. 2) I am generally not sure what I should do. This is refactoring and no bug fix / new feature so I guess I

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/toolbar_cleanup into lp:widelands

2016-11-21 Thread Notabilis
I can confirm the fixed crash. Since the crash is gone and the spacing is back I would say the branch is ready for merge. That is, if you design change is planned for another branch. -- https://code.launchpad.net/~widelands-dev/widelands/toolbar_cleanup/+merge/35 Your team Widelands Develope

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands

2016-11-23 Thread Notabilis
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 expedit

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands

2016-11-23 Thread Notabilis
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

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-free-workers into lp:widelands

2016-11-23 Thread Notabilis
Notabilis has proposed merging lp:~widelands-dev/widelands/bug-free-workers into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1643209 in widelands: "No-cost workers are not removed correctly" https://bugs.launchpad.net/widelands/+bug/16

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands

2016-11-28 Thread Notabilis
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

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/compiler_warnings_201611 into lp:widelands

2016-11-28 Thread Notabilis
I know, I shouldn't, but anyway: I only looked at the diff and most of it looks good. I do not like the change in network.h, though. I am not sure about it, but isn't the constructor taking the address of a temporary? -- https://code.launchpad.net/~widelands-dev/widelands/compiler_warnings_20161

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-collectors into lp:widelands

2016-11-29 Thread Notabilis
Notabilis has proposed merging lp:~widelands-dev/widelands/bug-collectors into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug-collectors/+merge/312092 When a game of Collectors ends and the

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-collectors into lp:widelands

2016-11-30 Thread Notabilis
Merging trunk is fine by me but I am not sure about your comments. I tried it with only one call of set_textdomain("win_conditions") at the beginning of the function. However, this does not translate the "Game over" string. When it should include the widelands-domain it does not seem to be work

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-collectors into lp:widelands

2016-12-01 Thread Notabilis
Modifying the PO file did not work for me but I don't really now what I am doing so it is okay. Anyway I changed it back to only the win_conditions textdomain in the function. -- https://code.launchpad.net/~widelands-dev/widelands/bug-collectors/+merge/312092 Your team Widelands Developers is su

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-supply into lp:widelands

2016-12-03 Thread Notabilis
Notabilis has proposed merging lp:~widelands-dev/widelands/bug-supply into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1639444 in widelands: "Workers with wares inside ships can crash the game" https://bugs.launchpad.net/widelands/+b

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/feature-test-supply-crash into lp:widelands

2016-12-03 Thread Notabilis
Notabilis has proposed merging lp:~widelands-dev/widelands/feature-test-supply-crash into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1639444 in widelands: "Workers with wares inside ships can crash the game" https://bugs.lau

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands

2016-12-04 Thread Notabilis
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 reall

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-collectors into lp:widelands

2016-12-04 Thread Notabilis
Great, thanks for testing. :) -- https://code.launchpad.net/~widelands-dev/widelands/bug-collectors/+merge/312092 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-collectors. ___ Mailing list: https://launchpad.net

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1624216-horsepocalypse into lp:widelands

2016-12-04 Thread Notabilis
Assuming this branch waits for a tester: Code looks good to me and testing the provided savegame with trunk and this branch it seems to work as planned. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1624216-horsepocalypse/+merge/310221 Your team Widelands Developers is requested to r

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands

2016-12-04 Thread Notabilis
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, work

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1644553-buildhelp-button into lp:widelands

2016-12-04 Thread Notabilis
I can't say anything about whether delete or destroy() is the better option. In any case, the change fixes the linked bug. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1644553-buildhelp-button/+merge/312429 Your team Widelands Developers is requested to review the proposed merge of

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/feature-test-supply-crash into lp:widelands

2017-01-06 Thread Notabilis
The suggestions from the review has been dealt with. The Travis-tests are failing on purpose since the created regression test shows a bug in trunk at creation time of this branch. When merging trunk locally the tests run fine. If there are no further objections regarding this branch trunk can

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_campaign into lp:widelands

2017-01-06 Thread Notabilis
Notabilis has proposed merging lp:~widelands-dev/widelands/casern_campaign into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) For more details, see: https://code.launchpad.net/~widelands-dev/widelands/casern_campaign/+merge/314208 Adapted the tutorials and campaigns

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-trainingsite-worker-display into lp:widelands

2017-01-06 Thread Notabilis
Notabilis has proposed merging lp:~widelands-dev/widelands/bug-trainingsite-worker-display into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1635808 in widelands: "Display of worker in training sites is not updated" https://bugs.lau

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1656671 into lp:widelands

2017-01-19 Thread Notabilis
The code looks okay to me and the I can confirm the fix for the save game. New games are probably fixed as well, at least I wasn't able to trigger the bug. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1656671/+merge/315075 Your team Widelands Developers is subscribed to branch lp:~

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1658317 into lp:widelands

2017-01-22 Thread Notabilis
On my system everything compiles and all tests run now. @travis checkitnow -- https://code.launchpad.net/~widelands-dev/widelands/bug-1658317/+merge/315302 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1658317 into lp:widelands.

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactoring-input-queue into lp:widelands

2017-01-22 Thread Notabilis
Notabilis has proposed merging lp:~widelands-dev/widelands/refactoring-input-queue into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) For more details, see: https://code.launchpad.net/~widelands-dev/widelands/refactoring-input-queue/+merge/315313 Replaces usage of

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactoring-input-queue into lp:widelands

2017-01-22 Thread Notabilis
Nits are done, testing by someone else is still needed. -- https://code.launchpad.net/~widelands-dev/widelands/refactoring-input-queue/+merge/315313 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/refactoring-input-queue. ___

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1191295-no-seafaring-builder into lp:widelands

2017-01-24 Thread Notabilis
Notabilis has proposed merging lp:~widelands-dev/widelands/bug-1191295-no-seafaring-builder into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1191295 in widelands: "Seafaring: builder not listed in expedition list in port&quo

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1191295-no-seafaring-builder into lp:widelands

2017-01-25 Thread Notabilis
I think I found a (at least prettier) solution for the problem: - Increase the packet number ob the portdock without changing anything else in the class. There can't be an expedition without a portdock and the dock is loaded first. So old games will fail gracefully when loading the new save game

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1191295-no-seafaring-builder into lp:widelands

2017-01-26 Thread Notabilis
I guess I could implement a version which does not change the save file format. However, this would mean: a) Open up some internals (mainly the request-object) of the WorkersQueue class to the expedition class b) Would only work as long as the number of workers is either 0 or 1 So it would be pos

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1658616-inputqueue-build19 into lp:widelands

2017-01-27 Thread Notabilis
Notabilis has proposed merging lp:~widelands-dev/widelands/bug-1658616-inputqueue-build19 into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1658616 in widelands: "InputQueue prevents old savegames from being loaded" https://bugs.lau

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1658616-inputqueue-build19 into lp:widelands

2017-01-27 Thread Notabilis
The proposal to merge lp:~widelands-dev/widelands/bug-1658616-inputqueue-build19 into lp:widelands has been updated. Description changed to: Added missing support for loading build19 save games with the InputQueues. Increases the packet version number of InputQueue, so games created by this b

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1191295-no-seafaring-builder into lp:widelands

2017-02-02 Thread Notabilis
Well, those who can read ... After some failed approaches I ended up with the creative data usage, just to push it and see your comment. Thanks for the hint with the warehouse packet version, I missed that one but I am using it now. Loading old expedition packets is not as pretty as I would lik

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1634736-victory-gamespeed into lp:widelands

2017-02-11 Thread Notabilis
Code looks good to me and in single player it works as wanted. What do you mean with your NOCOM? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1634736-victory-gamespeed/+merge/316973 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widela

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1658456-quantity-empire-soldier into lp:widelands

2017-02-11 Thread Notabilis
Notabilis has proposed merging lp:~widelands-dev/widelands/bug-1658456-quantity-empire-soldier into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1658456 in widelands: "Imperials: Soldier target quantity not changable" https://bugs.lau

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1661220-campaigns-barracks into lp:widelands

2017-02-11 Thread Notabilis
Notabilis has proposed merging lp:~widelands-dev/widelands/bug-1661220-campaigns-barracks into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1661220 in widelands: "Review campaigns in regard of needed barracks" https://bugs.launchpad.net

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/disambiguate_production_program_names into lp:widelands

2017-02-11 Thread Notabilis
I only looked at the diff and the lua-changes look good to me. I also like the extended documentation. I can't say much about the source code change. The code itself looks okay but I have no idea what exactly _() and pgettext_exp() are doing. -- https://code.launchpad.net/~widelands-dev/wideland

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/string-fixes into lp:widelands

2017-02-11 Thread Notabilis
Most of the diff looks good to me. Two nits: - "Ctrl + +" and "Ctrl + -" just look strange. But I haven't any better idea. - I would split up the "Ctrl + 1-9" help text into a "Ctrl + 1-9"-line for storing and a "1-9"-line for restoring the positions. Maybe also add a comment that the zoom level

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cleanup-networking-messages into lp:widelands

2017-02-11 Thread Notabilis
The diff looks good to me and Travis says it compiles. So I guess it can be merged. Just out of curiosity: Does anyone now by chance where "The metaserver was unable to connect to your game." is coming from in the current game version? I wasn't able to find the message in the source of the game

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1661220-campaigns-barracks into lp:widelands

2017-02-12 Thread Notabilis
Okay, no problem. Thanks for proofreading. Hm, I managed to kick your commit out of this branch?? Fascinating... Since we are already at it: In the training-objective attack is called the "major attribute of the Atlanteans". Why is this? I always assumed its evasion or defense since they are hig

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cleanup-networking-messages into lp:widelands

2017-02-12 Thread Notabilis
Review: Needs Fixing Oh, thanks for the explanation SirVer! This means we can't remove entries like igmessages["GAME_TIMEOUT"] since they are used/called by the metaserver. That is probably not the only not-really-but-seemingly-unused message in the client. -- https://code.launchpad.net/~widela

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cleanup-networking-messages into lp:widelands

2017-02-12 Thread Notabilis
A quick round with grep: Most of the STRINGS are sent by the metaserver. I couldn't find RESTARTING and GAME_NOT_CONNECTABLE so they can probably go. I also couldn't find the exception-strings BACKWAR[TD]S_RUNNING_TIME but that is no surprise. -- https://code.launchpad.net/~widelands-dev/widelan

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1657117-fishbreeder-messages into lp:widelands

2017-02-12 Thread Notabilis
Nice branch, I like the change. Testing with the provided save game showed both messages. Also, the code looks okay to me (as far as I understand it). Quite complicated, but I can't come up with a better way. I have two comments, though: 1) The send message says "if there are some fish left". Th

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1663490-ship-windows into lp:widelands

2017-02-12 Thread Notabilis
Notabilis has proposed merging lp:~widelands-dev/widelands/bug-1663490-ship-windows into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1663490 in widelands: "Ships can have multiple windows" https://bugs.launchpad.net/widelands/+bug/16

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1661220-campaigns-barracks into lp:widelands

2017-02-13 Thread Notabilis
Uh... I think one of us is reading the code wrong. Or maybe the code it behaving strange. As far as I see it: 1) Mark the objective as done (should be moved down to 3) 2) Wait one minute 3) Display dialog that objective is done 4) Wait 15 minutes 5) Start training site objective -- https://code.l

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1657117-fishbreeder-messages into lp:widelands

2017-02-13 Thread Notabilis
1) I care. At least a bit. The test map didn't had any fish in the upper part of the pond (Not "0 of 4" but really "none"). So neither the fisher nor the fish breeder can do any work there. But I guess it is a seldom case and can be ignored. 2) Okay, fine by me. I don't think it is important fo

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/string-fixes into lp:widelands

2017-02-19 Thread Notabilis
Review: Approve Thanks for the campaign fixes, the diff looks good to me now. -- https://code.launchpad.net/~widelands-dev/widelands/string-fixes/+merge/316931 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/string-fixes. __

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1663490-ship-windows into lp:widelands

2017-02-21 Thread Notabilis
Review: Resubmit Okay, good news first: I am using the UniqueWindow now and the code looks much better. Thanks for the hint. Bad news: In my previous commit, clicking on a ship with the window in the background or minimized focused and showed the window. This is no longer the case with the Uni

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/cleanup-networking-messages into lp:widelands

2017-02-21 Thread Notabilis
Review: Approve diff only I am a bit scared to say something here again, but: The diff looks good to me. ;-) I have neither tested nor compiled the changes, though. But grep couldn't find any of the to-be-removed strings in trunk or the metaserver source. So, uh... I guess it can be merged?

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1663490-ship-windows into lp:widelands

2017-02-21 Thread Notabilis
Currently, create() is only called by the ship windows and by popup messages (e.g. the current status with the collectors win condition but not messages like "no rocks"). When the notifications_buildingwindows branch is merged, buildings will use it, too. I just tried (and pushed) a modificatio

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/notifications_buildingwindows into lp:widelands

2017-02-26 Thread Notabilis
Review: Needs Fixing First of all: I like the intended change, the feature-changes as well as separating logic and wui. Unfortunately, there are two bigger problems with this branch. The diff looks good to me so far. A small nit: The std::map wanted_building_windows_ stores the coordinates as

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/notifications_buildingwindows into lp:widelands

2017-02-27 Thread Notabilis
Review: Approve Thanks for fixing, diffs looks good to me and it works as wanted when testing. Thanks for the change regarding upgrades and dismantling. I don't really understand your fix for the segfault. As far as I can see, your only change is storing the parent pointer now in the BuildingWi

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/notifications_buildingwindows into lp:widelands

2017-02-28 Thread Notabilis
Makes sense, thanks. -- https://code.launchpad.net/~widelands-dev/widelands/notifications_buildingwindows/+merge/317264 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/notifications_buildingwindows. ___ Mailing list:

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/notifications_sound into lp:widelands

2017-03-03 Thread Notabilis
Review: Approve Why does the description of this merge request sounds familiar? Strange... ;-) But the diff looks good to me and the sounds are playing in-game so it seems to be alright. I don't know much about cmake so I can't say much about moving widelands.h. Is there a real reason to move i

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/align_cleanup_textarea into lp:widelands

2017-03-05 Thread Notabilis
Review: Approve The diff looks good to me and I haven't noticed anything strange in-game. -- https://code.launchpad.net/~widelands-dev/widelands/align_cleanup_textarea/+merge/318322 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/align_cleanup_textarea. __

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/rendertarget_ints into lp:widelands

2017-04-23 Thread Notabilis
The diff looks okay to me and a short test game hasn't shown any optical glitches. But... ;-) Three minor remarks code-wise: - Is there a reason that Bob::calc_drawpos() (and derived methods) are still returning floats? Since it returns the pixel position on the screen I guess it is always an i

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1685645-workertable into lp:widelands

2017-04-24 Thread Notabilis
Notabilis has proposed merging lp:~widelands-dev/widelands/bug-1685645-workertable into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1685645 in widelands: "Workertable in buildings is broken" https://bugs.launchpad.net/widelands/+bug/16

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1687043-multiline-edit into lp:widelands

2017-04-29 Thread Notabilis
Notabilis has proposed merging lp:~widelands-dev/widelands/bug-1687043-multiline-edit into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1687043 in widelands: "Memory leak in Multilineeditbox" https://bugs.launchpad.net/widelands/+bug/16

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1655168-statistics-overlap into lp:widelands

2017-04-30 Thread Notabilis
Notabilis has proposed merging lp:~widelands-dev/widelands/bug-1655168-statistics-overlap into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1655168 in widelands: "statistic window: switch ealier to hours instead of minutes&quo

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1685331-oldsaves into lp:widelands

2017-04-30 Thread Notabilis
Notabilis has proposed merging lp:~widelands-dev/widelands/bug-1685331-oldsaves into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1685331 in widelands: "Barracks can't be built with old savegames" https://bugs.launchpad.net/widelan

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1685331-oldsaves into lp:widelands

2017-04-30 Thread Notabilis
No problem, is done. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1685331-oldsaves/+merge/323442 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1685331-oldsaves. ___ Mailing list: https://launchpad.

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1685331-oldsaves into lp:widelands

2017-04-30 Thread Notabilis
Just noticing: My last commit message is a bit broken. The first "Removed" should actually be an "Added". :-D -- https://code.launchpad.net/~widelands-dev/widelands/bug-1685331-oldsaves/+merge/323442 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1685331-o

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1685331-oldsaves into lp:widelands

2017-05-01 Thread Notabilis
The attached savegames have both already be saved with this branch, so the code of this branch does not react. Just my guess: You opened the single-player b19-save with trunk, noticed missing barracks, saved and opened the bug report. Then you opened and saved both games with this branch. Am I g

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1685331-oldsaves into lp:widelands

2017-05-01 Thread Notabilis
I attached another branch to the bug report, which should always enable barracks on loading a game. For the record: I am against merging the other branch since it breaks scenario savegames. https://code.launchpad.net/~widelands-dev/widelands/bug-1685331-always-enable -- https://code.launchpad.n

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1655168-statistics-overlap into lp:widelands

2017-05-01 Thread Notabilis
Somehow that savegame seems familiar... The problem is that the slider does not use the new function at all but has up to 7 hardcoded entries (see array time_in_ms). How many are shown is simply determined based on which one are less than the elapsed gametime. A simple fix would be to reduce the

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1655168-statistics-overlap into lp:widelands

2017-05-01 Thread Notabilis
Review: Resubmit Fixed overlap of slider labels by measuring their drawing width. This means that different languages will see different amounts of labels. Also sliders for the ware statistics and the general statistics differ since the windows have different widths. English and German seem to

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1685331-always-enable into lp:widelands

2017-05-02 Thread Notabilis
Review: Approve Code looks good and it seems to work fine. Tested it with b19 savegames with and without ports and with a current scenario savegame. A small remark: Increasing "tribe_buildings" could probably be replaced with "player->tribe().buildings().size()". -- https://code.launchpad.net/~

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/refac-netcode into lp:widelands

2017-05-09 Thread Notabilis
Notabilis has proposed merging lp:~widelands-dev/widelands/refac-netcode into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1689087 in widelands: "Implementing a relay server" https://bugs.launchpad.net/widelands/+bug/1689087 For mo

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/refac-netcode into lp:widelands

2017-05-09 Thread Notabilis
The proposal to merge lp:~widelands-dev/widelands/refac-netcode into lp:widelands has been updated. Description changed to: Hint: You don't need to know anything about networking to review this. Moving some code around to hide most SDLNet specific calls inside two classes. This branch should n

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/refac-netcode into lp:widelands

2017-05-10 Thread Notabilis
Thanks for the review! I included nearly all of them in the commit, except for you "Game or Net"-comment which was already correct. -- https://code.launchpad.net/~widelands-dev/widelands/refac-netcode/+merge/323798 Your team Widelands Developers is requested to review the proposed merge of lp:~w

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/refac-netcode into lp:widelands

2017-05-11 Thread Notabilis
Thanks, is done. I used the *Impl structs to hide that we are using SDLNet, but admittedly it doesn't really matter. -- https://code.launchpad.net/~widelands-dev/widelands/refac-netcode/+merge/323798 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/refac-netcode

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1690649-netclient into lp:widelands

2017-05-16 Thread Notabilis
Notabilis has proposed merging lp:~widelands-dev/widelands/bug-1690649-netclient into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1690649 in widelands: "crash in NetClient::is_connected()" https://bugs.launchpad.net/widelands/+bug/16

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1691335-network-empty-map-wincondition into lp:widelands

2017-05-17 Thread Notabilis
Review: Approve Code looks good and I wasn't able to make it crash any longer. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1691335-network-empty-map-wincondition/+merge/324160 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1691335-network-em

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

2017-05-20 Thread Notabilis
Notabilis has proposed merging lp:~widelands-dev/widelands/net-boost-asio into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1689087 in widelands: "Implementing a relay server" https://bugs.launchpad.net/widelands/+bug/1689087 For mo

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1636966-segfault-in-battle into lp:widelands

2017-05-20 Thread Notabilis
I think your argumentation makes sense and it is quite likely that the game was lagging when I encountered the bug. I wasn't able to force the bug by fighting with a low framerate, though. Diff looks good, too. When I read the code right, the soldier dies / is deleted after 1 second gametime wh

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

2017-05-21 Thread Notabilis
Thanks for the review! I included all your suggestions, even though the "prefer IPv6" part is somewhere else in the code. -- https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

2017-05-23 Thread Notabilis
Seems like its building on Windows (or at least on Appveyor) now. A confirmation and a short test would be appreciated, though. Also, building and testing on MacOS is still missing. -- https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364 Your team Widelands Developers

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

2017-05-25 Thread Notabilis
Review: Resubmit Thanks a lot for testing! Sorry, somehow missed this yesterday. - Thanks for fixing the find-broadcast code, it compiles and works on my system, too. - Trunk is merged, luckily without conflicts. - I went through the boost history, seems like version 1.48 is good enough. A lot o

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

2017-05-26 Thread Notabilis
Added log outputs for all errors, hopefully this will help finding out whats going on. I have some dates tomorrow, but I will be in the lobby as much as possible. Sunday would work, too. -- https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364 Your team Widelands Devel

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

2017-05-27 Thread Notabilis
Thanks for the review, I will address most of it. I commented some of your comments, on two points I disagree with you and would prefer the way it currently is. Feel free to insist if you think I am wrong. Diff comments: > > === modified file 'src/network/netclient.cc' > --- src/network/netclie

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

2017-05-28 Thread Notabilis
Review is dealt with. The code looks prettier now, thanks. I have to do some more reading about IPv6 and OSX. I fear we will need some platform specific code there. I will try to push something for testing in the next days. -- https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

2017-05-28 Thread Notabilis
Here, I have something more to play with for you. ;-) When I understand it right, with the current version you are always getting the log message "closing IPv6 socket" when entering the LAN lobby, right? With my newest push this message hopefully does not appear any longer. And if it works real

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

2017-05-29 Thread Notabilis
Travis is only complaining about my coding style, which I have to agree with. The current apple-test-code is totally ugly. ;-) The failing builds are the debug builds, seems like release builds aren't doing codechecks. When I understood it right, the new code worked on your system, or? If you wa

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

2017-05-30 Thread Notabilis
Fine by me, I will try to do so. Thanks a lot for all the testing, it was a great help! -- https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-boost-asio. _

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

2017-05-31 Thread Notabilis
So, code is cleaned up. A review of the new apple specific parts and maybe another short test, then it can go? -- https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-boost-asio. _

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

2017-06-01 Thread Notabilis
Uh uh, my fault. This should have actually been an #ifdef. Is fixed now. -- https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-boost-asio.

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

2017-06-03 Thread Notabilis
See the positive side: You were able to compile the branch! ;-) Seems like some kind of memory corruption though I am not sure where it is coming from. The line in GameHost indicates that the server crashed. What were you doing when it happened? Could it be that your network went offline (just a

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

2017-06-04 Thread Notabilis
Thanks for the explanation and fixing Bunnybot! Good to see that it is working again. I did a short test with two network interfaces on Linux. With the current code one packet is sent on both interfaces, so the kernel (or so) is already dealing with duplicating them. With the apple-code, two pa

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

2017-06-04 Thread Notabilis
Pushed a fix for an assert(state_ == OFFLINE) bug which I encountered. The problem was that on crash of a hosted game the logic jumped back to the main menu without closing the connection to the metaserver. So when the player tries to connect again, he already is connected to the metaserver (and

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

2017-06-05 Thread Notabilis
Today it does not happen for me anymore. Really strange effect, no idea what happened there. So, if no-one has any requests or suggestions left, lets merge this? -- https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364 Your team Widelands Developers is subscribed to br

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

2017-06-07 Thread Notabilis
The proposal to merge lp:~widelands-dev/widelands/net-boost-asio into lp:widelands has been updated. Commit Message changed to: Replaced SDL-Net with Boost.Asio. Added IPv6 support for non-metaserver games. For more details, see: https://code.launchpad.net/~widelands-dev/widelands/net-boost-asi

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

2017-06-07 Thread Notabilis
Thanks for testing and the review. I fixed all your nits except the iterator. A foreach-loop would not work since I need it to get the key of the std::map and also since I am modifying the map while running through it. The temporary key-variable (for it->first) could be probably removed when I w

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-boost-asio into lp:widelands

2017-06-09 Thread Notabilis
Oh, thanks for the hint! I wasn't aware that this loop allows access to the key. It does not solve the removal-problem, however. When I remove the client while in the loop the behavior of the rest of the loop will be undefined. I could add a local std::set and store the to-be-removed ids in there

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-internetgaming-ipv6 into lp:widelands

2017-06-20 Thread Notabilis
Notabilis has proposed merging lp:~widelands-dev/widelands/net-internetgaming-ipv6 into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1689087 in widelands: "Implementing a relay server" https://bugs.launchpad.net/widelands/+bug/1689087

  1   2   3   4   >