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

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 data but they are not used (for now?), so we don't have to store them.

For testing, I tried to merge trunk which gave me to conflicts due to the kAlign changes in trunk. They should be easy to fix.

Well, the building window is no longer maximized when the construction is finished. Now it is simply closed. ;-)
The problem is with the BuildingWindow::is_dying_ variable. When the construction is finished, Building::destroy() is called, sending a NoteBuilding with Action::kDeleted. This sets is_dying_ to true. Only afterwards ConstructionSite::cleanup() is called and sends the Action::kStartWarp message. Since the building is already dying at this point, BuildingWindow::on_building_note() ignores the message and never stores the window for reopening. Moving the check for Action::kStartWarp out of the !is_dying_ check seems to fix the problem.

Minor nit (can be ignored): When the building is finished, the (minimized) window is brought to the front (!= maximized). Not really a problem in my opinion.

The other big problem is a crash on exit. When the game ends (either alt+f4 or per menu) and a headquarters or construction window is open, the game crashes with a std::bad_cast exception.

Two minor points: When upgrading or dismantling buildings the window is closed. That is not a issue of this branch but we might want to change this since the construction window stays open, too.