Merge lp://qastaging/~widelands-dev/widelands/constructionsite_options into lp://qastaging/widelands

Continuous integration builds have changed state:

Travis build 4971. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/532431388.
Appveyor build 4752. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_constructionsite_options-4752.

I get these compiler warnings a couple of times.

/Users/toni/Launchpad/widelands-repo/working_tree/src/logic/map_objects/tribes/building_settings.h:42:8: warning: 'Widelands::BuildingSettings' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
struct BuildingSettings {
       ^
/Users/toni/Launchpad/widelands-repo/working_tree/src/logic/map_objects/tribes/building_settings.h:57:8: warning: 'Widelands::ProductionsiteSettings' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
struct ProductionsiteSettings : public BuildingSettings {
       ^
/Users/toni/Launchpad/widelands-repo/working_tree/src/logic/map_objects/tribes/building_settings.h:74:8: warning: 'Widelands::MilitarysiteSettings' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
struct MilitarysiteSettings : public BuildingSettings {
       ^
/Users/toni/Launchpad/widelands-repo/working_tree/src/logic/map_objects/tribes/building_settings.h:86:8: warning: 'Widelands::TrainingsiteSettings' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
struct TrainingsiteSettings : public ProductionsiteSettings {
       ^
/Users/toni/Launchpad/widelands-repo/working_tree/src/logic/map_objects/tribes/building_settings.h:97:8: warning: 'Widelands::WarehouseSettings' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
struct WarehouseSettings : public BuildingSettings {
       ^
5 warnings generated.

Some comments regarding the UI.

I think we should keep the UI consistent. Instead of checkboxes please use the icons/buttons that are used in the building's window. The same applies for the priority settings. We use traffic lights instead of buttons.

Fixed the compiler warnings and made the UI design consistent. I did not convert the Stop and Launch Expedition to toggle-buttons though because it is never clear with such buttons whether the icon shows the current state or the action taken by clicking it IMHO.

By the way, when clicking the Enhance button, the window will annoyingly switch to the first tab – this is unrelated to this branch, see bug 1818857.

hmm. I would prefer icons but you are completely right. I opened a topic in the forum:

https://wl.widelands.org/forum/topic/4518/?page=1#post-27957

Four warnings left:

src/logic/playercommand.cc:2288:6: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
                                        default:

src/logic/playercommand.cc:2384:6: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
                                        default:

src/wui/constructionsitewindow.cc:197:3: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
                default:

src/logic/map_objects/tribes/building_settings.cc:176:5: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
                                default:

Your buttons are a little too big. The buttons for in-/decreasing the wares are 24x24 while yours are 25x30.

Also, some wares are covered by the button; see the fish in the tavern (I guess that is because of your buttons being to big)

With this branch I get crashes after a while. Not sure if the culprit is this branch or something in trunk, though.

/Users/toni/Launchpad/widelands-repo/working_tree/src/graphic/animation.cc:451] Requested unknown animation with id: -432781024
==54196==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000000000 bp 0x7ffee6344650 sp 0x7ffee63445a8 T0)

Took care of the warnings and fixed the input queue layout.

> src/graphic/animation.cc:451] Requested unknown animation with id: -432781024

A backtrace would be helpful…
Did you get this while an (enhanced?) constructionsite was being built, or when a constructionsite was being completed? I got similar crashes before, but I thought I had caught all corner cases now…

Assertion failed: (military_site_->capacity_ != capacity), function set_soldier_capacity, file /Users/toni/Launchpad/widelands-repo/working_tree/src/logic/map_objects/tribes/militarysite.cc, line 85.

Assert fail fixed :)

Looks like this branch breaks savegame compatibility. When trying to load a game which runs fine in trunk i get:

Writing Buildingdata Data ... widelands: /home/kaputtnik/Quellcode/widelands-repo/constructionsite_options/src/map_io/map_buildingdata_packet.cc:974: void Widelands::MapBuildingdataPacket::write_constructionsite(const Widelands::ConstructionSite&, FileWrite&, Widelands::Game&, Widelands::MapObjectSaver&): Assertion `constructionsite.settings_' failed.
Abgebrochen (Speicherabzug geschrieben)

Oops… should be fixed now

And another assertion.

Assertion failed: (animation_index < animations.size()), function draw, file /Users/toni/Launchpad/widelands-repo/working_tree/src/logic/map_objects/tribes/constructionsite.cc, line 85.

This seems to be the same issue as the crash with animation ID -432781024, but with a more usable file&line number. Will push a fix soonish

Since the bug is not reproducible, I can´t be certain, but it should most likely be gone now

Seems to be fixed.

Trying to load a savgame from trunk prints now in a window (and console):

Game data error
buildingdata: building 524547: not found

Loading in trunk works fine. Save game: https://bugs.launchpad.net/widelands/+bug/1597310/+attachment/5264357/+files/construction_site_settings.wgf

I am getting a similar error when loading our homepage reference screenshot savegame Build, so we already have a change to saveloading in trunk where we overlooked having to increase the packet number.

Continuous integration builds have changed state:

Travis build 5003. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/533470509.
Appveyor build 4784. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_constructionsite_options-4784.

Would it be possible to move the tabs for Warehouses' and Ports' to the top, instead of having to tab rows?

OK, will change it like this

It would be nice if the tab was remembered after using the "enhance" button.

The target of the help button does not get updated when enhancing, e.g. construction site for deeper coal mine will show the coal mine instead.

"Launch expedition" should be "Start an expedition" like on the button for the finished port.

It would be nice if the extra options like "Start an expedition" or "Stop" would have the same UI buttons as the finished sites. I can live with the checkboxes though.

Also, this:

=================================================================
==6265==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 2744 byte(s) in 49 object(s) allocated from:
    #0 0x7ff86f576458 in operator new(unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xe0458)
    #1 0x55643637edfd in Widelands::MilitarySite::create_building_settings() const ../src/logic/map_objects/tribes/militarysite.cc:978
    #2 0x556435cb4c9b in Widelands::Player::enhance_or_dismantle(Widelands::Building*, unsigned char) ../src/logic/player.cc:763
    #3 0x556435cb47a3 in Widelands::Player::dismantle_building(Widelands::Building*) ../src/logic/player.cc:743
    #4 0x5564366afc07 in Widelands::CmdDismantleBuilding::execute(Widelands::Game&) ../src/logic/playercommand.cc:701
    #5 0x5564366a690e in Widelands::CmdQueue::run_queue(int, unsigned int&) ../src/logic/cmd_queue.cc:122
    #6 0x556435c8be44 in Widelands::Game::think() ../src/logic/game.cc:599
    #7 0x55643605fe76 in InteractiveBase::think() ../src/wui/interactive_base.cc:475
    #8 0x5564360b3bde in InteractivePlayer::think() ../src/wui/interactive_player.cc:228
    #9 0x556435eec265 in UI::Panel::do_think() ../src/ui_basic/panel.cc:483
    #10 0x556435ee95ca in UI::Panel::do_run() ../src/ui_basic/panel.cc:184
    #11 0x556435983b5d in UI::Panel::Returncodes UI::Panel::run<UI::Panel::Returncodes>() ../src/ui_basic/panel.h:104
    #12 0x556435c8b669 in Widelands::Game::run(UI::ProgressWindow*, Widelands::Game::StartGameType, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ../src/logic/game.cc:569
    #13 0x55643596f976 in WLApplication::new_game() ../src/wlapplication.cc:1354
    #14 0x55643596da4a in WLApplication::mainmenu_singleplayer() ../src/wlapplication.cc:1208
    #15 0x55643596cb95 in WLApplication::mainmenu() ../src/wlapplication.cc:1114
    #16 0x556435963b7b in WLApplication::run() ../src/wlapplication.cc:470
    #17 0x55643595f8ce in main ../src/main.cc:44
    #18 0x7ff86c9a9b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)

> "Launch expedition" should be "Start an expedition" like on the button for the finished port.
> The target of the help button does not get updated when enhancing, e.g. construction site for deeper coal mine will show the coal mine instead.
> ASan

Should all be fixed now

> It would be nice if the tab was remembered after using the "enhance" button.

This is exactly the same as bug 1818857 so it should be fixed independent from this branch. There will likely be one catch-all fix for both flavours of the bug.

> It would be nice if the extra options like "Start an expedition" or "Stop" would have the same UI buttons as the finished sites. I can live with the checkboxes though.

What I could do is to render the appropriate icon next to the checkbox if you like. But unless we change the other UI elements in such a way that they show clearly what the current state is, there is no alternative to the checkboxes IMHO, since the current icons are only acceptable because one can see the active state elsewhere (stats string / expedition tab), which can not happen here.

OK, let's leave the design as it is :)

This doesn't compile:

ow.cc.o -c ../src/wui/dismantlesitewindow.cc
In file included from ../src/wui/dismantlesitewindow.h:25:0,
                 from ../src/wui/dismantlesitewindow.cc:20:
../src/wui/buildingwindow.h: In member function ‘void BuildingWindow::set_building_descr_for_help(const Widelands::BuildingDescr&)’:
../src/wui/buildingwindow.h:99:30: error: ‘void Widelands::BuildingDescr::operator=(const Widelands::BuildingDescr&)’ is private within this context
   building_descr_for_help_ = d;
                              ^

Once that has been fixed, we still need to look at the code.

Continuous integration builds have changed state:

Travis build 5074. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/538200870.
Appveyor build 4854. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_constructionsite_options-4854.

Continuous integration builds have changed state:

Travis build 5076. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/538299033.
Appveyor build 4856. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_constructionsite_options-4856.

I finally got around to doing the code review. This branch still has a lot of room for streamlining the code and the performance.

Implemented your comments

Continuous integration builds have changed state:

Travis build 5185. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/544759817.
Appveyor build 4965. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_constructionsite_options-4965.

The changes look good - there are still some open comments, though. Sorry it took me a while to get back to this.

No problem. I have very little time for Widelands at the moment anyway :)

Continuous integration builds have changed state:

Travis build 5203. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/547642295.
Appveyor build 4982. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_constructionsite_options-4982.

LGTM now :)

I'd like to retest this due to the code changes made during review.

Tested and still working - thanks for this great feature!

@bunnybot merge

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 5203. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/547642295.

inputqueues again

@bunnybot merge force

Error merging this proposal:

Output:
stdout:

stderr:
Unable to obtain lock held by <email address hidden> on taotie (process #21580), acquired 19 hours, 2 minutes ago.
See "bzr help break-lock" for more.
bzr: ERROR: Could not acquire lock "(remote lock)": bzr+ssh://bazaar.launchpad.net/~widelands-dev/widelands/trunk/

@bunnybot merge force