[Widelands-dev] [Merge] lp:~widelands-dev/widelands/notifications_economy_window into lp:widelands
GunChleoc has proposed merging
lp:~widelands-dev/widelands/notifications_economy_window into lp:widelands.
Commit message:
Economy windows are now handled by a new notification 'NoteEconomyWindow', so
that economy no longer knows about on ui_basic.
Requested reviews:
Widelands Developers (widelands-dev)
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/notifications_economy_window/+merge/311572
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/notifications_economy_window into lp:widelands.
=== modified file 'src/economy/CMakeLists.txt'
--- src/economy/CMakeLists.txt 2015-11-28 22:29:26 +
+++ src/economy/CMakeLists.txt 2016-11-23 08:45:55 +
@@ -50,7 +50,7 @@
logic
logic_widelands_geometry
map_io
-ui_basic
+notifications
wui
)
add_subdirectory(test)
=== modified file 'src/economy/economy.cc'
--- src/economy/economy.cc 2016-09-27 06:30:47 +
+++ src/economy/economy.cc 2016-11-23 08:45:55 +
@@ -41,7 +41,7 @@
namespace Widelands {
-Economy::Economy(Player& player) : owner_(player), request_timerid_(0) {
+Economy::Economy(Player& player) : owner_(player), request_timerid_(0), has_window_(false) {
const TribeDescr& tribe = player.tribe();
DescriptionIndex const nr_wares = player.egbase().tribes().nrwares();
DescriptionIndex const nr_workers = player.egbase().tribes().nrworkers();
@@ -73,6 +73,9 @@
}
Economy::~Economy() {
+ const size_t economy_number = owner_.get_economy_number(this);
+ Notifications::publish(
+ NoteEconomyWindow(economy_number, economy_number, NoteEconomyWindow::Action::kClose));
owner_.remove_economy(*this);
if (requests_.size())
@@ -525,14 +528,12 @@
}
}
- // If the options window for e is open, but not the one for *this, the user
- // should still have an options window after the merge. Create an options
- // window for *this where the options window for e is, to give the user
- // some continuity.
- if (e.optionswindow_registry_.window && !optionswindow_registry_.window) {
- optionswindow_registry_.x = e.optionswindow_registry_.x;
- optionswindow_registry_.y = e.optionswindow_registry_.y;
- show_options_window();
+ // If the options window for e is open, but not the one for this, the user
+ // should still have an options window after the merge.
+ if (e.has_window() && !has_window()) {
+ Notifications::publish(NoteEconomyWindow(e.owner().get_economy_number(&e),
+ owner_.get_economy_number(this),
+ NoteEconomyWindow::Action::kRefresh));
}
for (std::vector::size_type i = e.get_nrflags() + 1; --i;) {
=== modified file 'src/economy/economy.h'
--- src/economy/economy.h 2016-08-04 15:49:05 +
+++ src/economy/economy.h 2016-11-23 08:45:55 +
@@ -33,7 +33,8 @@
#include "logic/map_objects/map_object.h"
#include "logic/map_objects/tribes/warelist.h"
#include "logic/map_objects/tribes/wareworker.h"
-#include "ui_basic/unique_window.h"
+#include "notifications/note_ids.h"
+#include "notifications/notifications.h"
namespace Widelands {
@@ -48,6 +49,22 @@
struct Router;
struct Supply;
+struct NoteEconomyWindow {
+ CAN_BE_SENT_AS_NOTE(NoteId::EconomyWindow)
+
+ size_t old_economy;
+ size_t new_economy;
+
+ enum class Action { kRefresh, kClose };
+ const Action action;
+
+ NoteEconomyWindow(size_t init_old,
+ size_t init_new,
+ const Action& init_action)
+ : old_economy(init_old), new_economy(init_new), action(init_action) {
+ }
+};
+
/**
* Each Economy represents all building and flags, which are connected over the same
* street network. In general a player can own multiple Economys, which
@@ -173,9 +190,11 @@
return worker_target_quantities_[i];
}
- void show_options_window();
- UI::UniqueWindow::Registry& optionswindow_registry() {
- return optionswindow_registry_;
+ bool has_window() const {
+ return has_window_;
+ }
+ void set_has_window(bool yes) {
+ has_window_ = yes;
}
const WareList& get_wares() const {
@@ -259,7 +278,7 @@
uint32_t request_timerid_;
static std::unique_ptr soldier_prototype_;
- UI::UniqueWindow::Registry optionswindow_registry_;
+ bool has_window_;
// 'list' of unique providers
std::map available_supplies_;
=== modified file 'src/notifications/note_ids.h'
--- src/notifications/note_ids.h 2016-09-28 09:35:53 +
+++ src/notifications/note_ids.h 2016-11-23 08:45:55 +
@@ -35,6 +35,7 @@
ProductionSiteOutOfResources,
TrainingSiteSoldierTrained,
ShipMessage,
+ EconomyWindow,
ShipWindow,
GraphicResolutionChanged,
NoteExpeditionCanceled
=== modified file 'src/wui/CMakeLists.txt'
--- src/wui/CMakeLists.txt 2016-10-26 06:54:00 +
+++ src/wui/CMakeLists.txt 2016-11-23 08:45:55 +
@@ -126,6 +126,8 @@
debugconsole.cc
debugconsole.h
dismantlesitewindow.cc
+economy_options_window.cc
+economy_options_window.h
[Widelands-dev] [Build #11244592] i386 build of widelands 1:18-ppa0-bzr8186-201611230847~ubuntu14.04.1 in ubuntu trusty RELEASE [~widelands-dev/ubuntu/widelands-daily]
* Source Package: widelands * Version: 1:18-ppa0-bzr8186-201611230847~ubuntu14.04.1 * Architecture: i386 * Archive: ~widelands-dev/ubuntu/widelands-daily * Component: main * State: Failed to build * Duration: 13 minutes * Build Log: https://launchpad.net/~widelands-dev/+archive/ubuntu/widelands-daily/+build/11244592/+files/buildlog_ubuntu-trusty-i386.widelands_1%3A18-ppa0-bzr8186-201611230847~ubuntu14.04.1_BUILDING.txt.gz * Builder: https://launchpad.net/builders/lcy01-32 * Source: not available If you want further information about this situation, feel free to contact a member of the Launchpad Buildd Administrators team. -- i386 build of widelands 1:18-ppa0-bzr8186-201611230847~ubuntu14.04.1 in ubuntu trusty RELEASE https://launchpad.net/~widelands-dev/+archive/ubuntu/widelands-daily/+build/11244592 You are receiving this email because you created this version of this package. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Build #11244591] amd64 build of widelands 1:18-ppa0-bzr8186-201611230847~ubuntu14.04.1 in ubuntu trusty RELEASE [~widelands-dev/ubuntu/widelands-daily]
* Source Package: widelands * Version: 1:18-ppa0-bzr8186-201611230847~ubuntu14.04.1 * Architecture: amd64 * Archive: ~widelands-dev/ubuntu/widelands-daily * Component: main * State: Failed to build * Duration: 14 minutes * Build Log: https://launchpad.net/~widelands-dev/+archive/ubuntu/widelands-daily/+build/11244591/+files/buildlog_ubuntu-trusty-amd64.widelands_1%3A18-ppa0-bzr8186-201611230847~ubuntu14.04.1_BUILDING.txt.gz * Builder: https://launchpad.net/builders/lcy01-33 * Source: not available If you want further information about this situation, feel free to contact a member of the Launchpad Buildd Administrators team. -- amd64 build of widelands 1:18-ppa0-bzr8186-201611230847~ubuntu14.04.1 in ubuntu trusty RELEASE https://launchpad.net/~widelands-dev/+archive/ubuntu/widelands-daily/+build/11244591 You are receiving this email because you created this version of this package. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_seafaring_split into lp:widelands
I like to add that the entire move was your suggestion! I don't have precise conceptions for it. What does the compiler say? So I can address all references in defaultai.h or only the public ones? -- https://code.launchpad.net/~widelands-dev/widelands/ai_seafaring_split/+merge/311544 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/ai_seafaring_split into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_seafaring_split into lp:widelands
It seems convenient to me that you (toptopple) make all changes in separate file and not interfere with other logic of AI. Compiler is happy and game with expedition works. Main struct here is DefaultAI and both files defaulai.cc and defaultai_seafaring.cc are defining functions inside the struct. But let somebody more proficient say if it is OK... -- https://code.launchpad.net/~widelands-dev/widelands/ai_seafaring_split/+merge/311544 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/ai_seafaring_split into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_seafaring_split into lp:widelands
Review: Approve Checked the code and don't find objections. --> Approve One thing you might consider still is to group declarations in defaultai.h as to current AI segmentation, here: seafaring code. Another thing you could think about is to, in the longer run, promote segmentation of AI code by seeking to group code references (e.g. declarations) after semantic relation (topic). This would enhance maintainability of the AI code. -- https://code.launchpad.net/~widelands-dev/widelands/ai_seafaring_split/+merge/311544 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/ai_seafaring_split. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
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 expeditions is not done yet. I plan to do so after the this branch and the refactoring branch are merged into trunk (don't like to create a branch for the bug in trunk which depends on other branches). Review comments from the first review are done. Additionally, I created a regression test for the caserns, since it is the only building using the WorkersQueue currently. In r7434 I fixed a bug which crashes scripts when they try to add a worker-worker to a building which already contains input-workers. Should have probably become an own branch, but the fix is small and I was lazy. So as far as I am concerned: The branch(es) is(/are) ready for the next review. -- https://code.launchpad.net/~widelands-dev/widelands/casern_workersqueue/+merge/309763 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
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.net/~widelands-dev/widelands/casern_workersqueue/+merge/309763 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-free-workers into lp:widelands
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/1643209
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-free-workers/+merge/311661
When more than 100 workers without buildcost enter a warehouse and the
warehouse is destroyed after some time, the workers are neither removed nor
send away. In debug builds an assert will fail which crashes the game, in
release builds it is probably only a memory leak.
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-free-workers into lp:widelands.
=== modified file 'src/logic/map_objects/tribes/warehouse.cc'
--- src/logic/map_objects/tribes/warehouse.cc 2016-11-17 06:08:27 +
+++ src/logic/map_objects/tribes/warehouse.cc 2016-11-23 20:53:07 +
@@ -826,7 +826,7 @@
supply_->add_workers(worker_index, 1);
- // We remove carriers, but we keep other workers around.
+ // We remove free workers, but we keep other workers around.
// TODO(unknown): Remove all workers that do not have properties such as experience.
// And even such workers should be removed and only a small record
// with the experience (and possibly other data that must survive)
@@ -834,8 +834,8 @@
// When this is done, the get_incorporated_workers method above must
// be reworked so that workers are recreated, and rescheduled for
// incorporation.
- if (upcast(Carrier, carrier, w)) {
- carrier->remove(egbase);
+ if (w->descr().is_buildable() && w->descr().buildcost().empty()) {
+ w->remove(egbase);
return;
}
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_seafaring_split into lp:widelands
I reordered declarations in defaultai.h a bit, perhaps it is better now. I would like to have also opinion of SirVer or Gunchleoc on this kind of change... -- https://code.launchpad.net/~widelands-dev/widelands/ai_seafaring_split/+merge/311544 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/ai_seafaring_split. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands
Continuous integration builds have changed state: Travis build 1645. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/178415914. Appveyor build 1483. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_casern_workersqueue-1483. -- https://code.launchpad.net/~widelands-dev/widelands/casern_workersqueue/+merge/309763 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/casern_workersqueue into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/ai_seafaring_split into lp:widelands
My opinion: great change! Thanks for refactoring. At work we have a loose rule that no function should be longer than 50 lines and no file longer than 1000 lines. This forces to think in modules, to reduce coupling between components and to implement in small pieces that are easy to understand on their own. This change moves into that direction, so I like it! :) If you can also split the library in CMakeLists.txt into 2 (or more) that do not have cyclic dependencies, this might even speed up compilation. -- https://code.launchpad.net/~widelands-dev/widelands/ai_seafaring_split/+merge/311544 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/ai_seafaring_split. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-free-workers into lp:widelands
Review: Approve good change, good catch. let's wait for travis before merging. -- https://code.launchpad.net/~widelands-dev/widelands/bug-free-workers/+merge/311661 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-free-workers. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/notifications_economy_window into lp:widelands
Review: Needs Fixing
I strongly approve of these moves towards making the logic UI agnostic!!!
I have a few suggestions to further separate UI and logic inline.
Diff comments:
>
> === modified file 'src/economy/economy.cc'
> --- src/economy/economy.cc2016-09-27 06:30:47 +
> +++ src/economy/economy.cc2016-11-23 09:27:13 +
> @@ -525,14 +528,12 @@
> }
> }
>
> - // If the options window for e is open, but not the one for *this, the
> user
> - // should still have an options window after the merge. Create an
> options
> - // window for *this where the options window for e is, to give the user
> - // some continuity.
> - if (e.optionswindow_registry_.window &&
> !optionswindow_registry_.window) {
> - optionswindow_registry_.x = e.optionswindow_registry_.x;
> - optionswindow_registry_.y = e.optionswindow_registry_.y;
> - show_options_window();
> + // If the options window for e is open, but not the one for this, the
> user
Should this not also be handled in the UI? You could publish a
EconomyNote(Merged, Deleted) and have the windows listen and recreate
themselves.
> + // should still have an options window after the merge.
> + if (e.has_window() && !has_window()) {
> +
> Notifications::publish(NoteEconomyWindow(e.owner().get_economy_number(&e),
> +
> owner_.get_economy_number(this),
> +
> NoteEconomyWindow::Action::kRefresh));
> }
>
> for (std::vector::size_type i = e.get_nrflags() + 1; --i;) {
>
> === modified file 'src/economy/economy.h'
> --- src/economy/economy.h 2016-08-04 15:49:05 +
> +++ src/economy/economy.h 2016-11-23 09:27:13 +
> @@ -48,6 +49,20 @@
> struct Router;
> struct Supply;
>
> +struct NoteEconomyWindow {
While this is *for* windows, it is *about* economies: NoteEconomy. Rephrase so
that this is clear: kChanged, kMerged, kDeleted instead of kRefresh and kClose.
Also make clear that if merge(1 -> 2) happen if you sent the merge message or
also the delete(1) message.
> + CAN_BE_SENT_AS_NOTE(NoteId::EconomyWindow)
> +
> + size_t old_economy;
add comments what these values are.
> + size_t new_economy;
> +
> + enum class Action { kRefresh, kClose };
> + const Action action;
> +
> + NoteEconomyWindow(size_t init_old, size_t init_new, const Action&
> init_action)
> +: old_economy(init_old), new_economy(init_new), action(init_action) {
> + }
> +};
> +
> /**
> * Each Economy represents all building and flags, which are connected over
> the same
> * street network. In general a player can own multiple Economys, which
> @@ -173,9 +188,11 @@
> return worker_target_quantities_[i];
> }
>
> - void show_options_window();
this should not be required with my proposal. Why should the economy care that
somebody has a window for it open?
> - UI::UniqueWindow::Registry& optionswindow_registry() {
> - return optionswindow_registry_;
> + bool has_window() const {
> + return has_window_;
> + }
> + void set_has_window(bool yes) {
> + has_window_ = yes;
> }
>
> const WareList& get_wares() const {
>
> === modified file 'src/wui/CMakeLists.txt'
> --- src/wui/CMakeLists.txt2016-10-26 06:54:00 +
> +++ src/wui/CMakeLists.txt2016-11-23 09:27:13 +
> @@ -126,6 +126,8 @@
> debugconsole.cc
> debugconsole.h
> dismantlesitewindow.cc
> +economy_options_window.cc
try pulling it out into a separate library while you're on it? Of course only
if you can do it without adding cyclic dependencies.
> +economy_options_window.h
> encyclopedia_window.cc
> encyclopedia_window.h
> fieldaction.cc
>
> === renamed file 'src/wui/transport_ui.cc' =>
> 'src/wui/economy_options_window.cc'
> --- src/wui/transport_ui.cc 2016-10-24 12:23:23 +
> +++ src/wui/economy_options_window.cc 2016-11-23 09:27:13 +
> @@ -17,247 +17,189 @@
> *
> */
>
> -#include "economy/economy.h"
> +#include "wui/economy_options_window.h"
> +
> +#include
> +
> #include "graphic/graphic.h"
> -#include "graphic/rendertarget.h"
> -#include "logic/map_objects/tribes/tribe_descr.h"
> #include "logic/map_objects/tribes/ware_descr.h"
> +#include "logic/map_objects/tribes/worker_descr.h"
> #include "logic/player.h"
> #include "logic/playercommand.h"
> #include "ui_basic/button.h"
> -#include "ui_basic/tabpanel.h"
> -#include "ui_basic/unique_window.h"
> -#include "wui/interactive_gamebase.h"
> -#include "wui/waresdisplay.h"
> -
> -#include
> -
> -using Widelands::Economy;
> -using Widelands::EditorGameBase;
> -using Widelands::Game;
> -using Widelands::WareDescr;
> -using Widelands::DescriptionIndex;
> -using Widelands::WorkerDescr;
>
> static const char pic_tab_wares[] =
> "images/wui/bu
