widelands-dev team mailing list archive
-
widelands-dev team
-
Mailing list archive
-
Message #08844
Re: [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.cc 2016-09-27 06:30:47 +0000
> +++ src/economy/economy.cc 2016-11-23 09:27:13 +0000
> @@ -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<Flag*>::size_type i = e.get_nrflags() + 1; --i;) {
>
> === modified file 'src/economy/economy.h'
> --- src/economy/economy.h 2016-08-04 15:49:05 +0000
> +++ src/economy/economy.h 2016-11-23 09:27:13 +0000
> @@ -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.txt 2016-10-26 06:54:00 +0000
> +++ src/wui/CMakeLists.txt 2016-11-23 09:27:13 +0000
> @@ -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 +0000
> +++ src/wui/economy_options_window.cc 2016-11-23 09:27:13 +0000
> @@ -17,247 +17,189 @@
> *
> */
>
> -#include "economy/economy.h"
> +#include "wui/economy_options_window.h"
> +
> +#include <boost/lexical_cast.hpp>
> +
> #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 <boost/lexical_cast.hpp>
> -
> -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/buildings/menu_tab_wares.png";
> static const char pic_tab_workers[] = "images/wui/buildings/menu_tab_workers.png";
>
> -struct EconomyOptionsWindow : public UI::UniqueWindow {
> - EconomyOptionsWindow(InteractiveGameBase& parent, Economy& economy)
> - : UI::UniqueWindow(&parent,
> - "economy_options",
> - &economy.optionswindow_registry(),
> - 0,
> - 0,
> - _("Economy options")),
> - tabpanel_(this, 0, 0, g_gr->images().get("images/ui_basic/but1.png")) {
> - set_center_panel(&tabpanel_);
> -
> - tabpanel_.add("wares", g_gr->images().get(pic_tab_wares),
> - new EconomyOptionsWarePanel(&tabpanel_, parent, economy), _("Wares"));
> - tabpanel_.add("workers", g_gr->images().get(pic_tab_workers),
> - new EconomyOptionsWorkerPanel(&tabpanel_, parent, economy), _("Workers"));
> +EconomyOptionsWindow::EconomyOptionsWindow(InteractiveGameBase& parent, Widelands::Economy& economy)
> + : UI::Window(&parent, "economy_options", 0, 0, 0, 0, _("Economy options")),
> + economy_number_(economy.owner().get_economy_number(&economy)),
> + owner_(economy.owner()),
> + tabpanel_(this, 0, 0, g_gr->images().get("images/ui_basic/but1.png")),
> + ware_panel_(
> + new EconomyOptionsPanel(&tabpanel_, parent, Widelands::wwWARE, economy_number_, owner_)),
> + worker_panel_(
> + new EconomyOptionsPanel(&tabpanel_, parent, Widelands::wwWORKER, economy_number_, owner_)) {
> + set_center_panel(&tabpanel_);
> +
> + tabpanel_.add("wares", g_gr->images().get(pic_tab_wares), ware_panel_, _("Wares"));
> + tabpanel_.add("workers", g_gr->images().get(pic_tab_workers), worker_panel_, _("Workers"));
> + economy.set_has_window(true);
> +
> + economynotes_subscriber_ = Notifications::subscribe<Widelands::NoteEconomyWindow>(
> + [this](const Widelands::NoteEconomyWindow& note) {
> + if (note.old_economy == economy_number_) {
Pull this out into a method: OnEconomyNote to avoid right drift and give the logic a place to live.
> + switch (note.action) {
> + case Widelands::NoteEconomyWindow::Action::kRefresh:
> + economy_number_ = note.new_economy;
> + ware_panel_->set_economy_number(note.new_economy);
> + worker_panel_->set_economy_number(note.new_economy);
> + owner_.get_economy_by_number(economy_number_)->set_has_window(true);
> + move_to_top();
> + break;
> + case Widelands::NoteEconomyWindow::Action::kClose:
> + // Make sure that the panels stop thinking first.
> + ware_panel_->die();
> + worker_panel_->die();
> + owner_.get_economy_by_number(economy_number_)->set_has_window(false);
> + die();
> + break;
> + }
> + }
> + });
> +}
> +
> +EconomyOptionsWindow::~EconomyOptionsWindow() {
> + owner_.get_economy_by_number(economy_number_)->set_has_window(false);
> +}
> +
> +EconomyOptionsWindow::TargetWaresDisplay::TargetWaresDisplay(UI::Panel* const parent,
> + int32_t const x,
> + int32_t const y,
> + const Widelands::TribeDescr& tribe,
> + Widelands::WareWorker type,
> + bool selectable,
> + size_t economy_number,
> + Widelands::Player& owner)
> + : AbstractWaresDisplay(parent, x, y, tribe, type, selectable),
> + economy_number_(economy_number),
> + owner_(owner) {
> + const Widelands::TribeDescr& owner_tribe = owner_.tribe();
> + if (type == Widelands::wwWORKER) {
> + for (const Widelands::DescriptionIndex& worker_index : owner_tribe.workers()) {
> + const Widelands::WorkerDescr* worker_descr = owner_tribe.get_worker_descr(worker_index);
> + if (!worker_descr->has_demand_check()) {
> + hide_ware(worker_index);
> + }
> + }
> + } else {
> + for (const Widelands::DescriptionIndex& ware_index : owner_tribe.wares()) {
> + const Widelands::WareDescr* ware_descr = owner_tribe.get_ware_descr(ware_index);
> + if (!ware_descr->has_demand_check(owner_tribe.name())) {
> + hide_ware(ware_index);
> + }
> + }
> }
> -
> -private:
> - UI::TabPanel tabpanel_;
> -
> - struct TargetWaresDisplay : public AbstractWaresDisplay {
> - TargetWaresDisplay(UI::Panel* const parent,
> - int32_t const x,
> - int32_t const y,
> - const Widelands::TribeDescr& tribe,
> - Widelands::WareWorker type,
> - bool selectable,
> - Economy& economy)
> - : AbstractWaresDisplay(parent, x, y, tribe, type, selectable), economy_(economy) {
> - const Widelands::TribeDescr& owner_tribe = economy_.owner().tribe();
> - if (type == Widelands::wwWORKER) {
> - for (const DescriptionIndex& worker_index : owner_tribe.workers()) {
> - const WorkerDescr* worker_descr = owner_tribe.get_worker_descr(worker_index);
> - if (!worker_descr->has_demand_check()) {
> - hide_ware(worker_index);
> - }
> - }
> - } else {
> - for (const DescriptionIndex& ware_index : owner_tribe.wares()) {
> - const WareDescr* ware_descr = owner_tribe.get_ware_descr(ware_index);
> - if (!ware_descr->has_demand_check(owner_tribe.name())) {
> - hide_ware(ware_index);
> - }
> - }
> - }
> - }
> -
> - protected:
> - std::string info_for_ware(Widelands::DescriptionIndex const ware) override {
> - return boost::lexical_cast<std::string>(
> - get_type() == Widelands::wwWORKER ? economy_.worker_target_quantity(ware).permanent :
> - economy_.ware_target_quantity(ware).permanent);
> - }
> -
> - private:
> - Economy& economy_;
> - };
> -
> - /**
> - * Wraps the wares display together with some buttons
> - */
> - struct EconomyOptionsWarePanel : UI::Box {
> - bool can_act_;
> - TargetWaresDisplay display_;
> - Economy& economy_;
> -
> - EconomyOptionsWarePanel(UI::Panel* parent, InteractiveGameBase& igbase, Economy& economy)
> - : UI::Box(parent, 0, 0, UI::Box::Vertical),
> - can_act_(igbase.can_act(economy.owner().player_number())),
> - display_(this, 0, 0, economy.owner().tribe(), Widelands::wwWARE, can_act_, economy),
> - economy_(economy) {
> - add(&display_, UI::Align::kLeft, true);
> -
> - UI::Box* buttons = new UI::Box(this, 0, 0, UI::Box::Horizontal);
> - add(buttons, UI::Align::kLeft);
> -
> - UI::Button* b = nullptr;
> -
> -#define ADD_WARE_BUTTON(callback, text, tooltip) \
> - b = new UI::Button(buttons, #callback, 0, 0, 34, 34, \
> - g_gr->images().get("images/ui_basic/but4.png"), text, tooltip); \
> - b->set_enabled(can_act_); \
> - b->sigclicked.connect(boost::bind(&EconomyOptionsWarePanel::callback, this)); \
> - buttons->add(b, UI::Align::kHCenter);
> - ADD_WARE_BUTTON(decrease_target, "-", _("Decrease target"))
> - b->set_repeating(true);
> - ADD_WARE_BUTTON(increase_target, "+", _("Increase target"))
> - b->set_repeating(true);
> - buttons->add_space(8);
> - ADD_WARE_BUTTON(reset_target, "R", _("Reset to default"))
> - }
> -
> - void decrease_target() {
> -
> - for (const DescriptionIndex& ware_index : economy_.owner().tribe().wares()) {
> - if (display_.ware_selected(ware_index)) {
> - const Economy::TargetQuantity& tq = economy_.ware_target_quantity(ware_index);
> - if (0 < tq.permanent) {
> - Widelands::Player& player = economy_.owner();
> - Game& game = dynamic_cast<Game&>(player.egbase());
> - game.send_player_command(*new Widelands::CmdSetWareTargetQuantity(
> - game.get_gametime(), player.player_number(),
> - player.get_economy_number(&economy_), ware_index, tq.permanent - 1));
> - }
> - }
> - }
> - }
> -
> - void increase_target() {
> - for (const DescriptionIndex& ware_index : economy_.owner().tribe().wares()) {
> - if (display_.ware_selected(ware_index)) {
> - const Economy::TargetQuantity& tq = economy_.ware_target_quantity(ware_index);
> - Widelands::Player& player = economy_.owner();
> - Game& game = dynamic_cast<Game&>(player.egbase());
> +}
> +
> +void EconomyOptionsWindow::TargetWaresDisplay::set_economy_number(size_t economy_number) {
> + economy_number_ = economy_number;
> +}
> +
> +std::string
> +EconomyOptionsWindow::TargetWaresDisplay::info_for_ware(Widelands::DescriptionIndex const ware) {
> + Widelands::Economy& economy = *owner_.get_economy_by_number(economy_number_);
> + return boost::lexical_cast<std::string>(get_type() == Widelands::wwWORKER ?
> + economy.worker_target_quantity(ware).permanent :
> + economy.ware_target_quantity(ware).permanent);
> +}
> +
> +/**
> + * Wraps the wares/workers display together with some buttons
> + */
> +EconomyOptionsWindow::EconomyOptionsPanel::EconomyOptionsPanel(UI::Panel* parent,
> + InteractiveGameBase& igbase,
> + Widelands::WareWorker type,
> + size_t economy_number,
> + Widelands::Player& owner)
> + : UI::Box(parent, 0, 0, UI::Box::Vertical),
> + type_(type),
> + economy_number_(economy_number),
> + owner_(owner),
> + can_act_(igbase.can_act(owner.player_number())),
> + display_(this, 0, 0, owner.tribe(), type_, can_act_, economy_number, owner) {
> + add(&display_, UI::Align::kLeft, true);
> +
> + UI::Box* buttons = new UI::Box(this, 0, 0, UI::Box::Horizontal);
> + add(buttons, UI::Align::kLeft);
> +
> + UI::Button* b =
> + new UI::Button(buttons, "decrease_target", 0, 0, 34, 34,
> + g_gr->images().get("images/ui_basic/but4.png"), "-", _("Decrease target"));
> + b->set_enabled(can_act_);
> + b->sigclicked.connect(boost::bind(&EconomyOptionsPanel::change_target, this, -1));
> + buttons->add(b, UI::Align::kHCenter);
> + b->set_repeating(true);
> + buttons->add_space(8);
> +
> + b = new UI::Button(buttons, "increase_target", 0, 0, 34, 34,
> + g_gr->images().get("images/ui_basic/but4.png"), "+", _("Increase target"));
> + b->set_enabled(can_act_);
> + b->sigclicked.connect(boost::bind(&EconomyOptionsPanel::change_target, this, 1));
> + buttons->add(b, UI::Align::kHCenter);
> + b->set_repeating(true);
> + buttons->add_space(8);
> +
> + b = new UI::Button(buttons, "reset_target", 0, 0, 34, 34,
> + g_gr->images().get("images/ui_basic/but4.png"), "R", _("Reset to default"));
> + b->set_enabled(can_act_);
> + b->sigclicked.connect(boost::bind(&EconomyOptionsPanel::reset_target, this));
> + buttons->add(b, UI::Align::kHCenter);
> +}
> +
> +void EconomyOptionsWindow::EconomyOptionsPanel::set_economy_number(size_t economy_number) {
> + economy_number_ = economy_number;
> + display_.set_economy_number(economy_number);
> +}
> +
> +void EconomyOptionsWindow::EconomyOptionsPanel::change_target(int amount) {
> + Widelands::Economy& economy = *owner_.get_economy_by_number(economy_number_);
> + Widelands::Game& game = dynamic_cast<Widelands::Game&>(owner_.egbase());
> + const bool is_wares = type_ == Widelands::wwWARE;
> + const auto& items = is_wares ? owner_.tribe().wares() : owner_.tribe().workers();
> + for (const Widelands::DescriptionIndex& index : items) {
> + if (display_.ware_selected(index)) {
> + const Widelands::Economy::TargetQuantity& tq =
> + is_wares ? economy.ware_target_quantity(index) : economy.worker_target_quantity(index);
> + // Don't allow negative new amount.
> + if (amount >= 0 || -amount <= static_cast<int>(tq.permanent)) {
> + if (is_wares) {
> game.send_player_command(*new Widelands::CmdSetWareTargetQuantity(
> - game.get_gametime(), player.player_number(), player.get_economy_number(&economy_),
> - ware_index, tq.permanent + 1));
> - }
> - }
> - }
> -
> - void reset_target() {
> -
> - for (const DescriptionIndex& ware_index : economy_.owner().tribe().wares()) {
> - if (display_.ware_selected(ware_index)) {
> - Widelands::Player& player = economy_.owner();
> - Game& game = dynamic_cast<Game&>(player.egbase());
> - game.send_player_command(*new Widelands::CmdResetWareTargetQuantity(
> - game.get_gametime(), player.player_number(), player.get_economy_number(&economy_),
> - ware_index));
> - }
> - }
> - }
> - };
> - struct EconomyOptionsWorkerPanel : UI::Box {
> - bool can_act_;
> - TargetWaresDisplay display_;
> - Economy& economy_;
> -
> - EconomyOptionsWorkerPanel(UI::Panel* parent, InteractiveGameBase& igbase, Economy& economy)
> - : UI::Box(parent, 0, 0, UI::Box::Vertical),
> - can_act_(igbase.can_act(economy.owner().player_number())),
> - display_(this, 0, 0, economy.owner().tribe(), Widelands::wwWORKER, can_act_, economy),
> - economy_(economy) {
> - add(&display_, UI::Align::kLeft, true);
> -
> - UI::Box* buttons = new UI::Box(this, 0, 0, UI::Box::Horizontal);
> - add(buttons, UI::Align::kLeft);
> -
> - UI::Button* b = nullptr;
> -#define ADD_WORKER_BUTTON(callback, text, tooltip) \
> - b = new UI::Button(buttons, #callback, 0, 0, 34, 34, \
> - g_gr->images().get("images/ui_basic/but4.png"), text, tooltip); \
> - b->set_enabled(can_act_); \
> - b->sigclicked.connect(boost::bind(&EconomyOptionsWorkerPanel::callback, this)); \
> - buttons->add(b, UI::Align::kHCenter);
> -
> - ADD_WORKER_BUTTON(decrease_target, "-", _("Decrease target"))
> - b->set_repeating(true);
> - ADD_WORKER_BUTTON(increase_target, "+", _("Increase target"))
> - b->set_repeating(true);
> - buttons->add_space(8);
> - ADD_WORKER_BUTTON(reset_target, "R", _("Reset to default"))
> - }
> -
> - void decrease_target() {
> - for (const DescriptionIndex& worker_index : economy_.owner().tribe().workers()) {
> - if (display_.ware_selected(worker_index)) {
> - const Economy::TargetQuantity& tq = economy_.worker_target_quantity(worker_index);
> - if (0 < tq.permanent) {
> - Widelands::Player& player = economy_.owner();
> - Game& game = dynamic_cast<Game&>(player.egbase());
> - game.send_player_command(*new Widelands::CmdSetWorkerTargetQuantity(
> - game.get_gametime(), player.player_number(),
> - player.get_economy_number(&economy_), worker_index, tq.permanent - 1));
> - }
> - }
> - }
> - }
> -
> - void increase_target() {
> - for (const DescriptionIndex& worker_index : economy_.owner().tribe().workers()) {
> - if (display_.ware_selected(worker_index)) {
> - const Economy::TargetQuantity& tq = economy_.worker_target_quantity(worker_index);
> - Widelands::Player& player = economy_.owner();
> - Game& game = dynamic_cast<Game&>(player.egbase());
> + game.get_gametime(), owner_.player_number(), economy_number_, index,
> + tq.permanent + amount));
> + } else {
> game.send_player_command(*new Widelands::CmdSetWorkerTargetQuantity(
> - game.get_gametime(), player.player_number(), player.get_economy_number(&economy_),
> - worker_index, tq.permanent + 1));
> - }
> - }
> - }
> -
> - void reset_target() {
> - for (const DescriptionIndex& worker_index : economy_.owner().tribe().workers()) {
> - if (display_.ware_selected(worker_index)) {
> - Widelands::Player& player = economy_.owner();
> - Game& game = dynamic_cast<Game&>(player.egbase());
> - game.send_player_command(*new Widelands::CmdResetWorkerTargetQuantity(
> - game.get_gametime(), player.player_number(), player.get_economy_number(&economy_),
> - worker_index));
> - }
> - }
> - }
> - };
> -};
> -
> -// TODO(unknown): Neither this function nor the UI Registry should be part
> -// of Economy. Economy should be made an observerable class where
> -// users can register for change updates. The registry should be
> -// moved to InteractivePlayer or some other UI component.
> -void Economy::show_options_window() {
> - if (optionswindow_registry_.window) {
> - optionswindow_registry_.window->move_to_top();
> - } else {
> - new EconomyOptionsWindow(
> - dynamic_cast<InteractiveGameBase&>(*owner().egbase().get_ibase()), *this);
> + game.get_gametime(), owner_.player_number(), economy_number_, index,
> + tq.permanent + amount));
> + }
> + }
> + }
> + }
> +}
> +
> +void EconomyOptionsWindow::EconomyOptionsPanel::reset_target() {
> + Widelands::Game& game = dynamic_cast<Widelands::Game&>(owner_.egbase());
> + const bool is_wares = type_ == Widelands::wwWARE;
> + const auto& items = is_wares ? owner_.tribe().wares() : owner_.tribe().workers();
> + for (const Widelands::DescriptionIndex& index : items) {
> + if (display_.ware_selected(index)) {
> + if (is_wares) {
> + game.send_player_command(*new Widelands::CmdResetWareTargetQuantity(
> + game.get_gametime(), owner_.player_number(), economy_number_, index));
> + } else {
> + game.send_player_command(*new Widelands::CmdResetWorkerTargetQuantity(
> + game.get_gametime(), owner_.player_number(), economy_number_, index));
> + }
> + }
> }
> }
>
> === modified file 'src/wui/fieldaction.cc'
> --- src/wui/fieldaction.cc 2016-10-26 06:54:00 +0000
> +++ src/wui/fieldaction.cc 2016-11-23 09:27:13 +0000
> @@ -589,8 +590,16 @@
> }
>
> void FieldActionWindow::act_configure_economy() {
> - if (upcast(const Widelands::Flag, flag, node_.field->get_immovable()))
> - flag->get_economy()->show_options_window();
> + if (upcast(const Widelands::Flag, flag, node_.field->get_immovable())) {
Can use InteractiveBase::unique_windows() to keep track which economy currently has a window.
> + Widelands::Economy* economy = flag->get_economy();
> + if (!economy->has_window()) {
> + new EconomyOptionsWindow(dynamic_cast<InteractiveGameBase&>(ibase()), *economy);
> + } else {
> + const size_t economy_number = economy->owner().get_economy_number(economy);
> + Notifications::publish(Widelands::NoteEconomyWindow(
> + economy_number, economy_number, Widelands::NoteEconomyWindow::Action::kRefresh));
> + }
> + }
> }
>
> /*
--
https://code.launchpad.net/~widelands-dev/widelands/notifications_economy_window/+merge/311572
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/notifications_economy_window.
References
