Allow preserving wares left in buildings when dismantling or enhancing

[Noordfrees]

Any wares left in the building will, if desired, be dropped out by the builder immediately upon his arrival at the PartiallyFinishedBuilding.
Whether this is desired can be set via a checkbox in the confirmation dialogue. This option is checked by default.
Ctrl-clicking the dismantle or enhance button will discard any wares left in the building.
The AI will always preserve wares when enhancing or dismantling sites.

Fixes #1431

[Niektory]

Seems to work as advertised. Potential issues:

• Contents of dismantle site windows of warehouses can exceed the size of the screen, making the buttons on the bottom of the window inaccessible.
• Wares waiting to be shipped in ports are not preserved.
• Preserve wares checkbox is shown for all building, even those that hold no wares, or those that aren't built yet (in case of enhancing a construction site).

[Noordfrees]

Thanks for testing :)

* Wares waiting to be shipped in ports are not preserved.

* Preserve wares checkbox is shown for all building, even those that hold no wares, or those that aren't built yet (in case of enhancing a construction site).

Fixed

* Contents of dismantle site windows of warehouses can exceed the size of the screen, making the buttons on the bottom of the window inaccessible.

Hm… most of the time there isn't a reason to dismantle a warehouse, is there? The only case I can think of is the 2nd frisian scenario where the player has to dismantle the port. But there the objective tells him to first move all wares to a new warehouse.
So I'm not sure if this is really a problem, and if so, what should the UI look like?

[spth]

Tried this branch for the first time today, loading an existing savegame. I can't build flags on medium building sites. Small and large sites, as well as the tiny sites (where flags are the only possibility) have the tab for building flags. But not medium sites.

[Noordfrees]

Tried this branch for the first time today, loading an existing savegame. I can't build flags on medium building sites. Small and large sites, as well as the tiny sites (where flags are the only possibility) have the tab for building flags. But not medium sites.

I cannot confirm this, can you upload a screenshot?

[Niektory]

The changes work fine. (I sometimes get segfaults when dismantling a port but it happens in master too)

• Contents of dismantle site windows of warehouses can exceed the size of the screen, making the buttons on the bottom of the window inaccessible.

Hm… most of the time there isn't a reason to dismantle a warehouse, is there? The only case I can think of is the 2nd frisian scenario where the player has to dismantle the port. But there the objective tells him to first move all wares to a new warehouse.
So I'm not sure if this is really a problem, and if so, what should the UI look like?

It should be fine to truncate the list of displayed wares to an amount that can be shown without problems, and display some sort of indicator that there are more.
As for a reason to dismantle a warehouse, it's similar to other buildings, to recover wares and space. Most of the time it's not needed, but occasionally it is :)

@spth Are you sure you didn't click a field next to an existing flag? You can't build flags next to each other.

[Noordfrees]

All right, using a scrollbar layout now when too many wares queues are present. This fixes the vertical oversize. For very large queues, I cut off the waresqueue display at the right edge of the window, which indicates to the user that there are many more items than visible in the queue.

[Niektory]

Nice! The scrollbar's color doesn't match the window though, and wares are displayed under it.

[Noordfrees]

I'm afraid changing the scrollbar style is out of scope for this branch:
https://github.com/widelands/widelands/blob/master/src/ui_basic/box.cc#L198-200

I would consider the fact that wares are drawn below the scrollbar desirable because it indicates that there are more wares than fit into the window…

[Niektory]

It's not the best indicator because it depends on the scrollbar position whether it can be seen or not.
I think something like this would be better:

AAAAAAAAAAAAAA +13 |↑|
BBB                | |
CCCCCCCCCCCCCC +5  | |
D                  |↓|

It's not very important though, so it can stay like it is too.

[Noordfrees]

Yes, the +x approach would be nice :)
But this would mean quite some refactoring of InputQueueDisplays which I'm reluctant to start a few days before feature freeze. So I'd vote to keep it as it is now for b21, and open a follow-up issue for b22 for the scrollbar style and the inputqueue width.

[spth]

Tried this branch for the first time today, loading an existing savegame. I can't build flags on medium building sites. Small and large sites, as well as the tiny sites (where flags are the only possibility) have the tab for building flags. But not medium sites.

I cannot confirm this, can you upload a screenshot?

It happened to me earlier today. I only noticed it after playing for about half an hour, so I don't know if the problem was there from when I first loaded the savegame. I was playing as red empire on the Lesser Ring map.

I now tried to reproduce the issue, but failed to do so. When loading savegames saved with the branch or the normal version (all red empire on Lesser Ring), I always see the flag tab there, as it should be, thus I cannot provide a screenshot.

But when the problem happened earlier today, the tab was simply missing on all the medium (yellow) sites (just as if there was already a flag next to the site).

[tppq]

There are some issues that could work differently:

• If upgraded building needs the wares, too, then the construction site could keep them instead of sending away, to make less road blocks. On the other hand, the current way has also benefits (maybe the wares are needed elsewhere while upgrade is ongoing)
• If dismantling a warehouse with lots of everything, the wares needed by economy could be ejected first. Now the player waits for gazillion water to come out, before the helmets become available again.
• GUI improvement by Niektory.

That said, those are really minor tunings IMHO. I tried to play this game but failed to make it do anything wrong.

I also browsed through the code and it seemed to make sense. At what level of certainty the reviews should be? "seems to make sense" != "everything already understood and found rock solid"

[tppq]

Forgot to say the obvious: Great improvement!

[Noordfrees]

There are some issues that could work differently:

* If upgraded building needs the wares, too, then the construction site could keep them instead of sending away, to make less road blocks. On the other hand, the current way has also benefits (maybe the wares are needed elsewhere while upgrade is ongoing)

IMHO it would be better to send them away in that case as well. Otherwise this would take control away from the player for rare wares, e.g. when upgrading your metal workshop to an axe factory it would be good if the iron could be sent to another metal workshop in the meantime.

* If dismantling a warehouse with lots of everything, the wares needed by economy could be ejected first. Now the player waits for gazillion water to come out, before the helmets become available again.

How about dropping the wares out round-robin?

I also browsed through the code and it seemed to make sense. At what level of certainty the reviews should be? "seems to make sense" != "everything already understood and found rock solid"

If you think a certain piece of code might work incorrectly in any imaginable corner-case, shout aloud! If the semantics of any code piece aren't obvious, ask! Other than that, if it is working correctly, "makes sense" is good enough.

[tppq]

If you think a certain piece of code might work incorrectly in any imaginable corner-case, shout aloud!

Cases like "enemy gains control of upgradesite while unloading in progress" untested (I do not have suitable multiplayer savegame at hand). Have you verified (by reading the code with goggles of that color, for example,) that lost wares are deleted and not anywhere referenced after the event?

[Noordfrees]

Cases like "enemy gains control of upgradesite while unloading in progress" untested (I do not have suitable multiplayer savegame at hand). Have you verified (by reading the code with goggles of that color, for example,) that lost wares are deleted and not anywhere referenced after the event?

Good point :) I would never have thought of that, and this is exactly the kind of corner-case that causes bugs to be merged.
In this case I am 99% sure that this will not be a problem, since only wares (never workers) are preserved like this, and wares are not actually stored in buildings – they are deleted, the building keeps a number of the wares it "theoretically" contains, and "dropping out" means creating a new ware instance and decreasing the building's counter. I'll verify this by testing though
Edit: Tested and no issue found

[tppq]

How about dropping the wares out round-robin?

Much simpler (=less bugs) and equally effective.

[tppq]

I read this through twice and did not capture anything suspicious.

[Noordfrees]

Thanks for the reviews :)
I'll merge as soon as travis is through