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
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.
__
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
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:
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
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
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
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?
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
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.
__
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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.
___
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
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.
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:~
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
301 - 362 of 362 matches
Mail list logo
