Code review comment for lp:~widelands-dev/widelands/empire04_unused_key_return_on_dismantle

Revision history for this message
Arty (artydent) wrote :

Reviewed the code and tested quite a lot. Looks pretty good, but still doesn't work quite right. Not hard to fix though.

1) Now that you changed the design to requiring the buildcost table to be empty instead of listing wares with amount 0, the dismantle button does not appear for such empty tables. This is easily fixed in ‘wui/buildingwindow.cc’ by removing a ware check, i.e., removing lines 287 and 297 (and unindenting the block inbetween). I already tested this, it works fine.

2) Having no items shown in the box when hovering over the dismantle button looks good enough for me, but I can imagine that some might find this not nice. Maybe adding a little text “nothing” or so might be good. And (at a later point) maybe even a little icon.

3) See diff comments.

review: Needs Fixing

« Back to merge proposal