Review: Needs Information
I am unsure about this change now. Most UI elements are owned by their parents.
Why did we need the delete before in the first place? More comments inlined.
Diff comments:
>
> === modified file 'src/wui/attack_box.cc'
> --- src/wui/attack_box.cc 2015-03-30 08:45:09 +0000
> +++ src/wui/attack_box.cc 2015-07-27 10:01:25 +0000
> @@ -119,31 +113,43 @@
> return *button;
> }
>
> +/*
> + * Update available soldiers
> + */
> +void AttackBox::think() {
> + int32_t gametime = player_->egbase().get_gametime();
can still be const
> + if ((gametime - lastupdate_) > kUpdateTimeInGametimeMs) {
> + update_attack();
> + lastupdate_ = gametime;
> + }
> +}
> +
> void AttackBox::update_attack() {
> - assert(m_slider_soldiers);
> - assert(m_text_soldiers);
> - assert(m_less_soldiers);
> - assert(m_add_soldiers);
> + assert(soldiers_slider_.get());
> + assert(soldiers_text_.get());
> + assert(less_soldiers_.get());
> + assert(more_soldiers_.get());
>
> int32_t max_attackers = get_max_attackers();
>
> - if (m_slider_soldiers->get_max_value() != max_attackers)
> - m_slider_soldiers->set_max_value(max_attackers);
> + if (soldiers_slider_->get_max_value() != max_attackers) {
> + soldiers_slider_->set_max_value(max_attackers);
> + }
>
> - m_slider_soldiers->set_enabled(max_attackers > 0);
> - m_add_soldiers->set_enabled(max_attackers >
> m_slider_soldiers->get_value());
> - m_less_soldiers ->set_enabled(m_slider_soldiers->get_value() > 0);
> + soldiers_slider_->set_enabled(max_attackers > 0);
> + more_soldiers_->set_enabled(max_attackers >
> soldiers_slider_->get_value());
> + less_soldiers_ ->set_enabled(soldiers_slider_->get_value() > 0);
>
> /** TRANSLATORS: %1% of %2% soldiers. Used in Attack box. */
> - m_text_soldiers->set_text((boost::format(_("%1% / %2%"))
> - %
> m_slider_soldiers->get_value()
> + soldiers_text_->set_text((boost::format(_("%1% / %2%"))
> + %
> soldiers_slider_->get_value()
> %
> max_attackers).str());
>
> - m_add_soldiers->set_title(std::to_string(max_attackers));
> + more_soldiers_->set_title(std::to_string(max_attackers));
> }
>
> void AttackBox::init() {
> - assert(m_node);
> + assert(node_coordinates_);
>
> uint32_t max_attackers = get_max_attackers();
>
> @@ -166,42 +172,42 @@
> const std::string attack_string =
> (boost::format(_("%1% / %2%")) % (max_attackers > 0 ? 1
> : 0) % max_attackers).str();
>
> - m_text_soldiers =
> + soldiers_text_.reset(
on second look that looks weird.... Can you doublecheck that add_text really
passes ownership? Maybe the delete's were wrong before?
If it passes ownership, it should directly return a unique_ptr<>. If that is a
bigger change, add a TODO so that it is at least documented. Right now it seems
to pass a reference - that is very wrong for passing ownership.
> &add_text(columnbox, attack_string, UI::Box::AlignCenter,
> UI::g_fh1->fontset().serif(),
> - UI_FONT_SIZE_ULTRASMALL);
> + UI_FONT_SIZE_ULTRASMALL));
>
> - m_slider_soldiers =
> + soldiers_slider_.reset(
> &add_slider
> (columnbox,
> 100, 10,
> 0, max_attackers, max_attackers > 0 ? 1 : 0,
> "pics/but2.png",
> - _("Number of soldiers"));
> + _("Number of soldiers")));
>
> -
> m_slider_soldiers->changed.connect(boost::bind(&AttackBox::update_attack,
> this));
> - m_add_soldiers =
> +
> soldiers_slider_->changed.connect(boost::bind(&AttackBox::update_attack,
> this));
> + more_soldiers_.reset(
> &add_button
> (linebox,
> std::to_string(max_attackers),
> &AttackBox::send_more_soldiers,
> - _("Send more soldiers"));
> + _("Send more soldiers")));
>
> - m_slider_soldiers->set_enabled(max_attackers > 0);
> - m_add_soldiers ->set_enabled(max_attackers > 0);
> - m_less_soldiers ->set_enabled(max_attackers > 0);
> + soldiers_slider_->set_enabled(max_attackers > 0);
> + more_soldiers_ ->set_enabled(max_attackers > 0);
> + less_soldiers_ ->set_enabled(max_attackers > 0);
> }
>
> void AttackBox::send_less_soldiers() {
> - assert(m_slider_soldiers);
> - m_slider_soldiers->set_value(m_slider_soldiers->get_value() - 1);
> + assert(soldiers_slider_.get());
> + soldiers_slider_->set_value(soldiers_slider_->get_value() - 1);
> }
>
> void AttackBox::send_more_soldiers() {
> - m_slider_soldiers->set_value(m_slider_soldiers->get_value() + 1);
> + soldiers_slider_->set_value(soldiers_slider_->get_value() + 1);
> }
>
> uint32_t AttackBox::soldiers() const {
> - assert(m_slider_soldiers);
> - return m_slider_soldiers->get_value();
> + assert(soldiers_slider_.get());
> + return soldiers_slider_->get_value();
> }
--
https://code.launchpad.net/~widelands-dev/widelands/bug-653308/+merge/265931
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-653308.
_______________________________________________
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
