Diff comments:

> 
> === modified file 'src/wui/attack_box.cc'
> --- src/wui/attack_box.cc     2019-02-23 11:00:49 +0000
> +++ src/wui/attack_box.cc     2019-05-07 12:26:33 +0000
> @@ -151,13 +204,28 @@
>       soldiers_slider_ = add_slider(
>          columnbox, 100, 10, 0, max_attackers, max_attackers > 0 ? 1 : 0, 
> _("Number of soldiers"));
>  
> -     
> soldiers_slider_->changed.connect(boost::bind(&AttackBox::update_attack, 
> this));
> +     attacking_soldiers_.reset(new ListOfSoldiers(&mainbox, this, 0, 0, 30, 
> 30,
> +                     _("Remove this soldier from the list of 
> attackers<br>Hold down Ctrl to remove all soldiers from the list")));

Split this into 2 strings to hide the <br> from translators. Same for the 
tooltip below.

> +     remaining_soldiers_.reset(new ListOfSoldiers(&mainbox, this, 0, 0, 30, 
> 30,
> +                     _("Add this soldier to the list of attackers<br>Hold 
> down Ctrl to add all soldiers to the list")));
> +     attacking_soldiers_->set_complement(remaining_soldiers_.get());
> +     remaining_soldiers_->set_complement(attacking_soldiers_.get());
> +
> +     for (const auto& s : all_attackers) {
> +             remaining_soldiers_->add(s);
> +     }
> +
> +     add_text(mainbox, _("Attackers:"));
> +     mainbox.add(attacking_soldiers_.get(), UI::Box::Resizing::kFullSize);
> +     add_text(mainbox, _("Not attacking:"));
> +     mainbox.add(remaining_soldiers_.get(), UI::Box::Resizing::kFullSize);
> +
> +     soldiers_slider_->changed.connect([this]() { update_attack(false); });
>       more_soldiers_ = add_button(linebox, std::to_string(max_attackers),
>                                   &AttackBox::send_more_soldiers, _("Send 
> more soldiers"));
>  
>       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() {
> @@ -169,7 +237,128 @@
>       soldiers_slider_->set_value(soldiers_slider_->get_value() + 1);
>  }
>  
> -uint32_t AttackBox::soldiers() const {
> -     assert(soldiers_slider_.get());
> -     return soldiers_slider_->get_value();
> -}
> +size_t AttackBox::count_soldiers() const {
> +     return attacking_soldiers_->count_soldiers();
> +}
> +
> +std::vector<Widelands::Serial> AttackBox::soldiers() const {
> +     std::vector<Widelands::Serial> result;
> +     for (const auto& s : attacking_soldiers_->get_soldiers()) {
> +             result.push_back(s->serial());
> +     }
> +     return result;
> +}
> +
> +constexpr int kSoldierIconWidth = 30;
> +constexpr int kSoldierIconHeight = 29;
> +
> +AttackBox::ListOfSoldiers::ListOfSoldiers(UI::Panel* const parent,
> +                AttackBox* parent_box,
> +                int32_t const x,
> +                int32_t const y,
> +                int const w,
> +                int const h,
> +               const std::string& tooltip,
> +               int16_t max_size,
> +               bool restrict_rows)
> +   : UI::Panel(parent, x, y, w, h, tooltip),
> +     size_restriction_(max_size),
> +     restricted_row_number_(restrict_rows),
> +     attack_box_(parent_box) {
> +     update_desired_size();
> +}
> +
> +bool AttackBox::ListOfSoldiers::handle_mousepress(uint8_t btn, int32_t x, 
> int32_t y) {
> +     if (btn != SDL_BUTTON_LEFT || !other_) {
> +             return UI::Panel::handle_mousepress(btn, x, y);
> +     }
> +     if (SDL_GetModState() & KMOD_CTRL) {
> +             for (const auto& s : get_soldiers()) {
> +                     remove(s);
> +                     other_->add(s);
> +             }
> +     } else {
> +             const Widelands::Soldier* s = soldier_at(x, y);
> +             if (!s) {
> +                     return UI::Panel::handle_mousepress(btn, x, y);
> +             }
> +             remove(s);
> +             other_->add(s);
> +     }
> +     attack_box_->update_attack(true);
> +     return true;
> +}
> +
> +Widelands::Extent AttackBox::ListOfSoldiers::size() const {
> +     const size_t nr_soldiers = count_soldiers();
> +     if (nr_soldiers == 0) {
> +             // At least one icon
> +             return Widelands::Extent(1, 1);
> +     }
> +     uint32_t rows = nr_soldiers / size_restriction_;
> +     if (rows * size_restriction_ < nr_soldiers) {
> +             ++rows;
> +     }
> +     if (restricted_row_number_) {
> +             return Widelands::Extent(rows, size_restriction_);
> +     } else {
> +             return Widelands::Extent(size_restriction_, rows);
> +     }
> +}
> +
> +void AttackBox::ListOfSoldiers::update_desired_size() {
> +     const Widelands::Extent e = size();
> +     set_desired_size(e.w * kSoldierIconWidth, e.h * kSoldierIconHeight);
> +}
> +
> +const Widelands::Soldier* AttackBox::ListOfSoldiers::soldier_at(int32_t x, 
> int32_t y) const {
> +     if (x < 0 || y < 0 || soldiers_.empty()) {
> +             return nullptr;
> +     }
> +     const int32_t col = x / kSoldierIconWidth;
> +     const int32_t row = y / kSoldierIconHeight;
> +     assert(col >= 0);

Can we assert > here, since we don't reach this when soldiers are empty?

> +     assert(row >= 0);
> +     if ((restricted_row_number_ ? row : col) >= size_restriction_) {
> +             return nullptr;
> +     }
> +     const int index = restricted_row_number_ ? size_restriction_ * col + 
> row : size_restriction_ * row + col;
> +     assert(index >= 0);
> +     return static_cast<unsigned int>(index) < soldiers_.size() ? 
> soldiers_[index] : nullptr;
> +}
> +
> +void AttackBox::ListOfSoldiers::add(const Widelands::Soldier* s) {
> +     soldiers_.push_back(s);
> +     update_desired_size();
> +}
> +
> +void AttackBox::ListOfSoldiers::remove(const Widelands::Soldier* s) {
> +     const auto it = std::find(soldiers_.begin(), soldiers_.end(), s);
> +     assert(it != soldiers_.end());
> +     soldiers_.erase(it);
> +     update_desired_size();
> +}
> +
> +void AttackBox::ListOfSoldiers::draw(RenderTarget& dst) {
> +     const size_t nr_soldiers = soldiers_.size();
> +     int32_t column = 0;
> +     int32_t row = 0;
> +     for (uint32_t i = 0; i < nr_soldiers; ++i) {
> +             Vector2i location(column * kSoldierIconWidth, row * 
> kSoldierIconHeight);
> +             soldiers_[i]->draw_info_icon(location, 1.0f, false, &dst);
> +             if (restricted_row_number_) {
> +                     ++row;
> +                     if (row >= size_restriction_) {
> +                             row = 0;
> +                             ++column;
> +                     }
> +             } else {
> +                     ++column;
> +                     if (column >= size_restriction_) {
> +                             column = 0;
> +                             ++row;
> +                     }
> +             }
> +     }
> +}
> +
> 
> === modified file 'src/wui/attack_box.h'
> --- src/wui/attack_box.h      2019-02-23 11:00:49 +0000
> +++ src/wui/attack_box.h      2019-05-07 12:26:33 +0000
> @@ -88,6 +91,76 @@
>       std::unique_ptr<UI::Button> less_soldiers_;
>       std::unique_ptr<UI::Button> more_soldiers_;
>  
> +     // A SoldierPanel is not applicable here as it's keyed to a building 
> and thinks too much

I haven't looked at the SoldierPanel - is there enough common code to warrant a 
common superclass?

> +     struct ListOfSoldiers : public UI::Panel {
> +             ListOfSoldiers(UI::Panel* const parent,
> +                            AttackBox* parent_box,
> +                            int32_t const x,
> +                            int32_t const y,
> +                            int const w,
> +                            int const h,
> +                        const std::string& tooltip,
> +                            int16_t max_size = 8,
> +                            bool restrict_rows = false);
> +
> +             bool handle_mousepress(uint8_t btn, int32_t x, int32_t y) 
> override;
> +
> +             const Widelands::Soldier* soldier_at(int32_t x, int32_t y) 
> const;
> +             void add(const Widelands::Soldier*);
> +             void remove(const Widelands::Soldier*);
> +             bool contains(const Widelands::Soldier* soldier) const {
> +                     for (const auto& s : soldiers_) {
> +                             if (s == soldier) {
> +                                     return true;
> +                             }
> +                     }
> +                     return false;
> +             }
> +
> +             std::vector<const Widelands::Soldier*> get_soldiers() const {
> +                     return soldiers_;
> +             }
> +             const Widelands::Soldier* get_soldier() const {
> +                     return soldiers_.back();
> +             }
> +
> +             size_t count_soldiers() const {
> +                     return soldiers_.size();
> +             }
> +             Widelands::Extent size() const;
> +             bool row_number_restricted() const {
> +                     return restricted_row_number_;
> +             }
> +             int16_t size_restriction() const {
> +                     return size_restriction_;
> +             }
> +             void set_size_restriction(int16_t r) {
> +                     size_restriction_ = r;
> +             }
> +             void set_row_number_restricted(bool r) {
> +                     restricted_row_number_ = r;
> +             }
> +
> +             void draw(RenderTarget& dst) override;
> +
> +             void set_complement(ListOfSoldiers* o) {
> +                     other_ = o;
> +             }
> +
> +     private:
> +             int16_t size_restriction_; // Highest number of rows or columns
> +             bool restricted_row_number_;
> +             std::vector<const Widelands::Soldier*> soldiers_;
> +
> +             ListOfSoldiers* other_;
> +             AttackBox* attack_box_;
> +
> +             void update_desired_size() override;
> +     };
> +
> +     std::unique_ptr<ListOfSoldiers> attacking_soldiers_;
> +     std::unique_ptr<ListOfSoldiers> remaining_soldiers_;
> +
>       /// The last time the information in this Panel got updated
>       uint32_t lastupdate_;
>  };


-- 
https://code.launchpad.net/~widelands-dev/widelands/choose-attack-soldiers/+merge/367041
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/choose-attack-soldiers 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

Reply via email to