widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

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.