widelands-dev team mailing list archive

[Benedikt Straub <benedikt-straub@xxxxxx>]

Implemented or replied to the diff comments.

The ASan error is in trunk and can happen everywhere there are dropdowns. For example, when I´m thrown back to the main menu by an error from a fsmenu screen that has one I already got this crash. It can appear whenever destroying a window that contains a dropdown on a lower level. Happens very rarely though, so I did not report it…

> In the ware statistics, the bottom slider and text are cut off.
> In the economy window, we get wide gaps between the columns and some extra space on the left and right of the button rows.
They probably do not layout properly yet… will look into it

Diff comments:

> 
> === modified file 'src/wui/economy_options_window.cc'
> --- src/wui/economy_options_window.cc	2019-02-23 11:00:49 +0000
> +++ src/wui/economy_options_window.cc	2019-05-16 17:28:26 +0000
> @@ -186,35 +281,402 @@
>  			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(), serial_, index,
> -					   tq.permanent + amount));
> +			// Don't allow negative new amount
> +			const int old_amount = static_cast<int>(tq.permanent);
> +			const int new_amount = std::max(0, old_amount + delta);
> +			if (new_amount == old_amount) {
> +				continue;
> +			}
> +			if (is_wares) {
> +				game.send_player_command(*new Widelands::CmdSetWareTargetQuantity(
> +				   game.get_gametime(), player_->player_number(), serial_, index, new_amount));
> +			} else {
> +				game.send_player_command(*new Widelands::CmdSetWorkerTargetQuantity(
> +				   game.get_gametime(), player_->player_number(), serial_, index, new_amount));
> +			}
> +		}
> +	}
> +}
> +
> +void EconomyOptionsWindow::EconomyOptionsPanel::reset_target() {
> +	Widelands::Game& game = dynamic_cast<Widelands::Game&>(player_->egbase());
> +	const bool is_wares = type_ == Widelands::wwWARE;
> +	const auto& items = is_wares ? player_->tribe().wares() : player_->tribe().workers();
> +	const PredefinedTargets settings = economy_options_window_->get_selected_target();
> +
> +	bool anything_selected = false;
> +	bool second_phase = false;
> +
> +run_second_phase:
> +	for (const Widelands::DescriptionIndex& index : items) {
> +		if (display_.ware_selected(index) || (second_phase && !display_.is_ware_hidden(index))) {
> +			anything_selected = true;
> +			if (is_wares) {
> +				game.send_player_command(*new Widelands::CmdSetWareTargetQuantity(
> +						game.get_gametime(), player_->player_number(), serial_, index, settings.wares.at(index)));
> +			} else {
> +				game.send_player_command(*new Widelands::CmdSetWorkerTargetQuantity(
> +						game.get_gametime(), player_->player_number(), serial_, index, settings.workers.at(index)));
> +			}
> +		}
> +	}
> +
> +	if (!second_phase && !anything_selected) {
> +		// Nothing was selected, now go through the loop again and change everything
> +		second_phase = true;
> +		goto run_second_phase;
> +	}
> +}
> +
> +constexpr unsigned kThinkInterval = 200;
> +
> +void EconomyOptionsWindow::think() {
> +	const uint32_t time = player_->egbase().get_gametime();
> +	if (time - time_last_thought_ < kThinkInterval || !player_->get_economy(serial_)) {
> +		// If our economy has been deleted, die() was already called, no need to do anything
> +		return;
> +	}
> +	time_last_thought_ = time;
> +	update_profiles();
> +}
> +
> +std::string EconomyOptionsWindow::applicable_target() {
> +	const Widelands::Economy* eco = player_->get_economy(serial_);
> +	for (const auto& pair : predefined_targets_) {
> +		bool matches = true;
> +		if (tabpanel_.active() == 0) {
> +			for (const Widelands::DescriptionIndex& index : player_->tribe().wares()) {
> +				const auto it = pair.second.wares.find(index);
> +				if (it != pair.second.wares.end() && eco->ware_target_quantity(index).permanent != it->second) {
> +					matches = false;
> +					break;
> +				}
> +			}
> +		} else {
> +			for (const Widelands::DescriptionIndex& index : player_->tribe().workers()) {
> +				const auto it = pair.second.workers.find(index);
> +				if (it != pair.second.workers.end() && eco->worker_target_quantity(index).permanent != it->second) {
> +					matches = false;
> +					break;
> +				}
> +			}
> +		}
> +		if (matches) {
> +			return pair.first;
> +		}
> +	}
> +	return "";
> +}
> +
> +void EconomyOptionsWindow::update_profiles() {
> +	const std::string current_profile = applicable_target();
> +
> +	for (const auto& pair : predefined_targets_) {
> +		if (last_added_to_dropdown_.count(pair.first) == 0) {
> +			return update_profiles_needed(current_profile);
> +		}
> +	}
> +	for (const auto& string : last_added_to_dropdown_) {
> +		if (!string.empty() && predefined_targets_.find(string) == predefined_targets_.end()) {
> +			return update_profiles_needed(current_profile);
> +		}
> +	}
> +	if (last_added_to_dropdown_.count("") == (current_profile.empty() ? 0 : 1)) {
> +		return update_profiles_needed(current_profile);
> +	}
> +
> +	update_profiles_select(current_profile);
> +}
> +
> +void EconomyOptionsWindow::update_profiles_needed(const std::string& current_profile) {
> +	dropdown_.clear();
> +	last_added_to_dropdown_.clear();
> +	for (const auto& pair : predefined_targets_) {
> +		dropdown_.add(_(pair.first), pair.first);
> +		last_added_to_dropdown_.insert(pair.first);
> +	}
> +	if (current_profile.empty()) {
> +		// Nothing selected
> +		dropdown_.add("", "");

If a player decides to name a profile "–" then, this may cause very weird behaviour, especially since we do not know how it´ll be translated…

> +		last_added_to_dropdown_.insert("");
> +	}
> +	update_profiles_select(current_profile);
> +}
> +
> +void EconomyOptionsWindow::update_profiles_select(const std::string& current_profile) {
> +	if (dropdown_.is_expanded()) {
> +		return;
> +	}
> +	const std::string select = current_profile.empty() ? "" : _(current_profile);

Fixed. I wouldn´t even have needed to bother with the whole _("") confusion...

> +	if (!dropdown_.has_selection() || dropdown_.get_selected() != select) {
> +		dropdown_.select(select);
> +	}
> +	assert(dropdown_.has_selection());
> +}
> +
> +void EconomyOptionsWindow::SaveProfileWindow::update_save_enabled() {
> +	const std::string& text = profile_name_.text();
> +	if (text.empty() || text == kDefaultEconomyProfile) {
> +		save_.set_enabled(false);
> +		save_.set_tooltip(text.empty() ? _("The profile name cannot be empty") :
> +				_("The default profile cannot be overwritten"));
> +	} else {
> +		save_.set_enabled(true);
> +		save_.set_tooltip(_("Save the profile under this name"));
> +	}
> +}
> +
> +void EconomyOptionsWindow::SaveProfileWindow::table_selection_changed() {
> +	if (!table_.has_selection()) {
> +		delete_.set_enabled(false);
> +		delete_.set_tooltip("");
> +		return;
> +	}
> +	const std::string& sel = table_[table_.selection_index()];
> +	if (economy_options_->get_predefined_targets().at(sel).undeletable) {
> +		delete_.set_tooltip(_("The predefined profiles cannot be deleted"));
> +		delete_.set_enabled(false);
> +	} else {
> +		delete_.set_tooltip(_("Delete the selected profiles"));
> +		delete_.set_enabled(true);
> +	}
> +	profile_name_.set_text(sel);
> +	update_save_enabled();
> +}
> +
> +void EconomyOptionsWindow::SaveProfileWindow::update_table() {
> +	table_.clear();
> +	for (const auto& pair : economy_options_->get_predefined_targets()) {
> +		table_.add(pair.first).set_string(0, _(pair.first));
> +	}
> +	layout();
> +}
> +
> +void EconomyOptionsWindow::SaveProfileWindow::save() {
> +	const std::string name = profile_name_.text();
> +	assert(!name.empty());
> +	assert(name != kDefaultEconomyProfile);
> +	for (const auto& pair : economy_options_->get_predefined_targets()) {
> +		if (pair.first == name) {
> +			UI::WLMessageBox m(this, _("Overwrite?"),
> +					_("A profile with this name already exists.\nDo you wish to replace it?"),
> +					UI::WLMessageBox::MBoxType::kOkCancel);
> +			if (m.run<UI::Panel::Returncodes>() != UI::Panel::Returncodes::kOk) {
> +				return;
> +			}
> +			break;
> +		}
> +	}
> +	economy_options_->do_create_target(name);
> +	die();
> +}
> +
> +void EconomyOptionsWindow::SaveProfileWindow::delete_selected() {
> +	assert(table_.has_selection());
> +
> +	auto& map = economy_options_->get_predefined_targets();
> +	const std::string& name = table_[table_.selection_index()];
> +
> +	assert(name != kDefaultEconomyProfile);
> +	assert(!map.at(name).undeletable);
> +	auto it = map.find(name);
> +	assert(it != map.end());
> +	map.erase(it);
> +
> +	economy_options_->save_targets();
> +	economy_options_->update_profiles();
> +	update_table();
> +}
> +
> +void EconomyOptionsWindow::SaveProfileWindow::unset_parent() {
> +	economy_options_ = nullptr;
> +	die();
> +}
> +
> +void EconomyOptionsWindow::SaveProfileWindow::think() {
> +	if (!economy_options_) {
> +		die();
> +	}
> +	UI::Window::think();
> +}
> +
> +EconomyOptionsWindow::SaveProfileWindow::SaveProfileWindow(UI::Panel* parent, EconomyOptionsWindow* eco)
> +   : UI::Window(parent, "save_economy_options_profile", 0, 0, 0, 0, _("Save Profile")),
> +     economy_options_(eco),
> +     main_box_(this, 0, 0, UI::Box::Vertical),
> +     table_box_(&main_box_, 0, 0, UI::Box::Vertical),
> +     table_(&table_box_, 0, 0, 460, 120, UI::PanelStyle::kWui),
> +     buttons_box_(&main_box_, 0, 0, UI::Box::Horizontal),
> +     profile_name_(&buttons_box_, 0, 0, 240, 0, 0, UI::PanelStyle::kWui),
> +     save_(&buttons_box_, "save", 0, 0, 80, 34, UI::ButtonStyle::kWuiPrimary, _("Save")),
> +     cancel_(&buttons_box_, "cancel", 0, 0, 80, 34, UI::ButtonStyle::kWuiSecondary, _("Cancel")),
> +     delete_(&buttons_box_, "delete", 0, 0, 80, 34, UI::ButtonStyle::kWuiSecondary, _("Delete")) {
> +	table_.add_column(200, _("Existing Profiles"));
> +	update_table();
> +
> +	table_.selected.connect([this](uint32_t) { table_selection_changed(); });
> +	profile_name_.changed.connect([this] { update_save_enabled(); });
> +	profile_name_.ok.connect([this] { save(); });
> +	profile_name_.cancel.connect([this] { die(); });
> +	save_.sigclicked.connect([this] { save(); });
> +	cancel_.sigclicked.connect([this] { die(); });
> +	delete_.sigclicked.connect([this] { delete_selected(); });
> +
> +	table_box_.add(&table_, UI::Box::Resizing::kFullSize);
> +	buttons_box_.add(&profile_name_, UI::Box::Resizing::kFullSize);
> +	buttons_box_.add(&save_);
> +	buttons_box_.add(&cancel_);
> +	buttons_box_.add(&delete_);
> +	main_box_.add(&table_box_, UI::Box::Resizing::kFullSize);
> +	main_box_.add(&buttons_box_, UI::Box::Resizing::kFullSize);
> +	set_center_panel(&main_box_);
> +
> +	table_selection_changed();
> +	update_save_enabled();
> +}
> +
> +EconomyOptionsWindow::SaveProfileWindow::~SaveProfileWindow() {
> +	if (economy_options_) {
> +		economy_options_->close_save_profile_window();
> +	}
> +}
> +
> +void EconomyOptionsWindow::create_target() {
> +	if (save_profile_dialog_) {
> +		// Already open
> +		return;
> +	}
> +	save_profile_dialog_ = new SaveProfileWindow(get_parent(), this);
> +}
> +
> +void EconomyOptionsWindow::close_save_profile_window() {
> +	assert(save_profile_dialog_);
> +	save_profile_dialog_ = nullptr;
> +}
> +
> +void EconomyOptionsWindow::do_create_target(const std::string& name) {
> +	assert(!name.empty());
> +	assert(name != kDefaultEconomyProfile);
> +	const Widelands::Tribes& tribes = player_->egbase().tribes();
> +	const Widelands::TribeDescr& tribe = player_->tribe();
> +	Widelands::Economy* economy = player_->get_economy(serial_);
> +	PredefinedTargets t;
> +	for (Widelands::DescriptionIndex di : tribe.wares()) {
> +		if (tribes.get_ware_descr(di)->has_demand_check(tribe.name())) {
> +			t.wares[di] = economy->ware_target_quantity(di).permanent;
> +		}
> +	}
> +	for (Widelands::DescriptionIndex di : tribe.workers()) {
> +		if (tribes.get_worker_descr(di)->has_demand_check()) {
> +			t.workers[di] = economy->worker_target_quantity(di).permanent;
> +		}
> +	}
> +	predefined_targets_[name] = t;
> +
> +	save_targets();
> +	update_profiles();
> +}
> +
> +void EconomyOptionsWindow::save_targets() {
> +	const Widelands::Tribes& tribes = player_->egbase().tribes();
> +	Profile profile;
> +
> +	std::map<std::string, uint32_t> serials;
> +	for (const auto& pair : predefined_targets_) {
> +		if (pair.first != kDefaultEconomyProfile) {
> +			serials.emplace(pair.first, serials.size());
> +		}
> +	}
> +	Section& global_section = profile.create_section(kDefaultEconomyProfile.c_str());
> +	for (const auto& pair : serials) {
> +		global_section.set_string(std::to_string(pair.second).c_str(), pair.first);
> +	}
> +
> +	for (const auto& pair : predefined_targets_) {
> +		if (pair.first == kDefaultEconomyProfile) {
> +			continue;
> +		}
> +		Section& section = profile.create_section(std::to_string(serials.at(pair.first)).c_str());
> +		for (const auto& setting : pair.second.wares) {
> +			section.set_natural(tribes.get_ware_descr(setting.first)->name().c_str(), setting.second);
> +		}
> +		for (const auto& setting : pair.second.workers) {
> +			section.set_natural(tribes.get_worker_descr(setting.first)->name().c_str(), setting.second);
> +		}
> +	}
> +
> +	Section& section = profile.create_section("undeletable");
> +	for (const auto& pair : predefined_targets_) {
> +		if (pair.first != kDefaultEconomyProfile) {
> +			section.set_bool(std::to_string(serials.at(pair.first)).c_str(), pair.second.undeletable);
> +		}
> +	}
> +
> +	g_fs->ensure_directory_exists(kEconomyProfilesDir);
> +	std::string complete_filename = kEconomyProfilesDir + g_fs->file_separator() + player_->tribe().name();
> +	profile.write(complete_filename.c_str(), false);
> +
> +	// Inform the windows of other economies of new and deleted profiles
> +	Notifications::publish(NoteEconomyProfile(serial_));
> +}
> +
> +void EconomyOptionsWindow::read_targets() {
> +	predefined_targets_.clear();
> +	const Widelands::Tribes& tribes = player_->egbase().tribes();
> +	const Widelands::TribeDescr& tribe = player_->tribe();
> +
> +	{
> +		PredefinedTargets t;
> +		t.undeletable = true;
> +		for (Widelands::DescriptionIndex di : tribe.wares()) {
> +			const Widelands::WareDescr* descr = tribes.get_ware_descr(di);
> +			if (descr->has_demand_check(tribe.name())) {
> +				t.wares.emplace(di, descr->default_target_quantity(tribe.name()));
> +			}
> +		}
> +		for (Widelands::DescriptionIndex di : tribe.workers()) {
> +			const Widelands::WorkerDescr* descr = tribes.get_worker_descr(di);
> +			if (descr->has_demand_check()) {
> +				t.workers.emplace(di, descr->default_target_quantity());
> +			}
> +		}
> +		predefined_targets_.emplace(kDefaultEconomyProfile, t);
> +	}
> +
> +	std::string complete_filename = kEconomyProfilesDir + g_fs->file_separator() + player_->tribe().name();
> +	Profile profile;
> +	profile.read(complete_filename.c_str());
> +
> +	Section* global_section = profile.get_section(kDefaultEconomyProfile);
> +	if (global_section) {
> +		std::map<std::string, std::string> serials;
> +		while (Section::Value* v = global_section->get_next_val()) {
> +			serials.emplace(std::string(v->get_name()), v->get_string());
> +		}
> +
> +		for (const auto& pair : serials) {
> +			Section* section = profile.get_section(pair.first);
> +			PredefinedTargets t;
> +			while (Section::Value* v = section->get_next_val()) {
> +				const std::string name = std::string(v->get_name());
> +				Widelands::DescriptionIndex di = tribes.ware_index(name);
> +				if (di == Widelands::INVALID_INDEX) {
> +					di = tribes.worker_index(name);
> +					assert(di != Widelands::INVALID_INDEX);
> +					t.workers.emplace(di, v->get_natural());
>  				} else {
> -					game.send_player_command(*new Widelands::CmdSetWorkerTargetQuantity(
> -					   game.get_gametime(), player_->player_number(), serial_, index,
> -					   tq.permanent + amount));
> +					t.wares.emplace(di, v->get_natural());
>  				}
>  			}
> +			predefined_targets_.emplace(pair.second, t);
>  		}
> -	}
> -}
>  
> -void EconomyOptionsWindow::EconomyOptionsPanel::reset_target() {
> -	Widelands::Game& game = dynamic_cast<Widelands::Game&>(player_->egbase());
> -	const bool is_wares = type_ == Widelands::wwWARE;
> -	const auto& items = is_wares ? player_->tribe().wares() : player_->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(), player_->player_number(), serial_, index));
> -			} else {
> -				game.send_player_command(*new Widelands::CmdResetWorkerTargetQuantity(
> -				   game.get_gametime(), player_->player_number(), serial_, index));
> +		if (Section* section = profile.get_section("undeletable")) {
> +			while (Section::Value* v = section->get_next_val()) {
> +				predefined_targets_.at(serials.at(std::string(v->get_name()))).undeletable = v->get_bool();
>  			}
>  		}
>  	}
> +
> +	update_profiles();
>  }


-- 
https://code.launchpad.net/~widelands-dev/widelands/economy-target-profiles/+merge/366987
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/economy-target-profiles.