Skip to content

Commit

Permalink
GUI2/Tree View Node: store child nodes in a vector of unique_ptrs ins…
Browse files Browse the repository at this point in the history 
…tead of boost::ptr_vector

I don't know if it would be better design to
  • Loading branch information
@Vultraz
Vultraz committed Sep 11, 2017
1 parent 0fef006 commit cba7493
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 53 deletions.
5 changes: 2 additions & 3 deletions src/gui/auxiliary/iterator/walker_tree_node.cpp
View file
Expand Up @@ -15,7 +15,6 @@
#define GETTEXT_DOMAIN "wesnoth-lib"

#include "gui/auxiliary/iterator/walker_tree_node.hpp"
#include "gui/widgets/tree_view_node.hpp"

#include <cassert>

Expand All @@ -25,7 +24,7 @@ namespace gui2
namespace iteration
{

tree_node::tree_node(gui2::tree_view_node& node, boost::ptr_vector<gui2::tree_view_node>& children)
tree_node::tree_node(gui2::tree_view_node& node, tree_view_node::node_children_vector& children)
: children_(children), widget_(&node), itor_(children.begin())
{
}
Expand Down Expand Up @@ -83,7 +82,7 @@ gui2::widget* tree_node::get(const level level)
if(itor_ == children_.end()) {
return nullptr;
} else {
return itor_.operator->();
return itor_.operator->()->get();
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/gui/auxiliary/iterator/walker_tree_node.hpp
View file
Expand Up @@ -16,7 +16,7 @@

#include "gui/auxiliary/iterator/walker.hpp"

#include <boost/ptr_container/ptr_vector.hpp>
#include "gui/widgets/tree_view_node.hpp"

namespace gui2
{
Expand All @@ -36,7 +36,7 @@ class tree_node : public walker_base
* @param node The tree view node which the walker is attached to.
* @param children The node's children.
*/
tree_node(gui2::tree_view_node& node, boost::ptr_vector<gui2::tree_view_node>& children);
tree_node(gui2::tree_view_node& node, tree_view_node::node_children_vector& children);

/** Inherited from @ref gui2::iteration::walker_base. */
virtual state_t next(const level level);
Expand All @@ -49,7 +49,7 @@ class tree_node : public walker_base

private:
/** The children of the node which the walker is attached to. */
boost::ptr_vector<gui2::tree_view_node>& children_;
tree_view_node::node_children_vector& children_;

/**
* The node which the walker is attached to.
Expand All @@ -65,7 +65,7 @@ class tree_node : public walker_base
* This variable is used to track where the @ref
* gui2::iteration::walker_base::child level visiting is.
*/
boost::ptr_vector<gui2::tree_view_node>::iterator itor_;
tree_view_node::node_children_vector::iterator itor_;
};

} // namespace iteration
Expand Down
4 changes: 2 additions & 2 deletions src/gui/dialogs/gamestate_inspector.cpp
View file
Expand Up @@ -330,8 +330,8 @@ class gamestate_inspector::controller
// Furthermore, there's no need to remember that a subnode was open once the parent is closed.
if(!selected->is_root_node()) {
for(auto& node : selected->parent_node().children()) {
if(&node != selected) {
node.fold(true);
if(node.get() != selected) {
node->fold(true);
}
}

Expand Down
5 changes: 4 additions & 1 deletion src/gui/widgets/tree_view.cpp
View file
Expand Up @@ -74,7 +74,10 @@ int tree_view::remove_node(tree_view_node* node)

tree_view_node::node_children_vector& siblings = node->parent_node_->children_;

auto node_itor = std::find(siblings.begin(), siblings.end(), *node);
auto node_itor = std::find_if(siblings.begin(), siblings.end(),
[node](const std::unique_ptr<tree_view_node>& c) { return c.get() == node; }
);

assert(node_itor != siblings.end());

const int position = node_itor - siblings.begin();
Expand Down
76 changes: 41 additions & 35 deletions src/gui/widgets/tree_view_node.cpp
View file
Expand Up @@ -18,6 +18,7 @@

#include "gettext.hpp"
#include "gui/auxiliary/find_widget.hpp"
#include "gui/auxiliary/iterator/walker_tree_node.hpp"
#include "gui/core/log.hpp"
#include "gui/widgets/toggle_button.hpp"
#include "gui/widgets/toggle_panel.hpp"
Expand Down Expand Up @@ -125,7 +126,7 @@ void tree_view_node::clear_before_destruct()
{
tree_view_ = nullptr;
for(auto& child : children_) {
child.clear_before_destruct();
child->clear_before_destruct();
}
}

Expand All @@ -134,28 +135,28 @@ tree_view_node& tree_view_node::add_child(
const std::map<std::string /* widget id */, string_map>& data,
const int index)
{
boost::ptr_vector<tree_view_node>::iterator itor = children_.end();
auto itor = children_.end();

if(static_cast<size_t>(index) < children_.size()) {
itor = children_.begin() + index;
}

itor = children_.insert(itor, new tree_view_node(id, this, get_tree_view(), data));
itor = children_.emplace(itor, new tree_view_node(id, this, get_tree_view(), data));

if(is_folded() /*|| is_root_node()*/) {
return *itor;
return **itor;
}

if(get_tree_view().get_size() == point()) {
return *itor;
return **itor;
}

assert(get_tree_view().content_grid());
const point current_size = get_tree_view().content_grid()->get_size();

// Calculate width modification.
// This increases tree width if the width of the new node is greater than the current width.
point best_size = itor->get_best_size();
point best_size = (*itor)->get_best_size();
best_size.x += get_indentation_level() * get_tree_view().indentation_step_size_;

const int width_modification = best_size.x > current_size.x
Expand All @@ -179,9 +180,9 @@ tree_view_node& tree_view_node::add_child(
assert(height_modification >= 0);

// Request new size.
get_tree_view().resize_content(width_modification, height_modification, -1, itor->calculate_ypos());
get_tree_view().resize_content(width_modification, height_modification, -1, (*itor)->calculate_ypos());

return *itor;
return **itor;
}

unsigned tree_view_node::get_indentation_level() const
Expand Down Expand Up @@ -225,7 +226,7 @@ void tree_view_node::fold(const bool recursive)

if(recursive) {
for(auto& child_node : children_) {
child_node.fold(true);
child_node->fold(true);
}
}
}
Expand All @@ -241,7 +242,7 @@ void tree_view_node::unfold(const bool recursive)

if(recursive) {
for(auto& child_node : children_) {
child_node.unfold(true);
child_node->unfold(true);
}
}
}
Expand Down Expand Up @@ -288,7 +289,7 @@ void tree_view_node::clear()
if(!is_folded()) {
for(const auto & node : children_)
{
height_reduction += node.get_current_size().y;
height_reduction += node->get_current_size().y;
}
}

Expand All @@ -311,7 +312,7 @@ struct tree_view_node_implementation
const bool must_be_active)
{
for(It it = begin; it != end; ++it) {
if(W* widget = it->find_at(coordinate, must_be_active)) {
if(W* widget = (*it)->find_at(coordinate, must_be_active)) {
return widget;
}
}
Expand Down Expand Up @@ -361,8 +362,8 @@ widget* tree_view_node::find(const std::string& id, const bool must_be_active)
return result;
}

for(tree_view_node& child : children_) {
result = child.find(id, must_be_active);
for(auto& child : children_) {
result = child->find(id, must_be_active);
if(result) {
return result;
}
Expand All @@ -383,8 +384,8 @@ const widget* tree_view_node::find(const std::string& id, const bool must_be_act
return result;
}

for(const tree_view_node& child : children_) {
result = child.find(id, must_be_active);
for(const auto& child : children_) {
result = child->find(id, must_be_active);
if(result) {
return result;
}
Expand All @@ -404,7 +405,7 @@ void tree_view_node::impl_populate_dirty_list(window& caller, const std::vector<

for(auto& node : children_) {
std::vector<widget*> child_call_stack = call_stack;
node.impl_populate_dirty_list(caller, child_call_stack);
node->impl_populate_dirty_list(caller, child_call_stack);
}
}

Expand All @@ -430,11 +431,11 @@ point tree_view_node::get_current_size(bool assume_visible) const
}

for(const auto& node : children_) {
if(node.grid_.get_visible() == widget::visibility::invisible) {
if(node->grid_.get_visible() == widget::visibility::invisible) {
continue;
}

point node_size = node.get_current_size();
point node_size = node->get_current_size();

size.y += node_size.y;
size.x = std::max(size.x, node_size.x);
Expand All @@ -460,11 +461,11 @@ point tree_view_node::get_unfolded_size() const
}

for(const auto& node : children_) {
if(node.grid_.get_visible() == widget::visibility::invisible) {
if(node->grid_.get_visible() == widget::visibility::invisible) {
continue;
}

point node_size = node.get_current_size(true);
point node_size = node->get_current_size(true);

size.y += node_size.y;
size.x = std::max(size.x, node_size.x);
Expand All @@ -486,11 +487,11 @@ point tree_view_node::calculate_best_size(const int indentation_level,
DBG_GUI_L << LOG_HEADER << " own grid best size " << best_size << ".\n";

for(const auto& node : children_) {
if(node.grid_.get_visible() == widget::visibility::invisible) {
if(node->grid_.get_visible() == widget::visibility::invisible) {
continue;
}

const point node_size = node.calculate_best_size(indentation_level + 1, indentation_step_size);
const point node_size = node->calculate_best_size(indentation_level + 1, indentation_step_size);

if(!is_folded()) {
best_size.y += node_size.y;
Expand Down Expand Up @@ -548,7 +549,7 @@ unsigned tree_view_node::place(const unsigned indentation_step_size,

DBG_GUI_L << LOG_HEADER << " set children.\n";
for(auto & node : children_) {
origin.y += node.place(indentation_step_size, origin, width);
origin.y += node->place(indentation_step_size, origin, width);
}

// Inherited.
Expand All @@ -570,7 +571,7 @@ void tree_view_node::set_visible_rectangle(const SDL_Rect& rectangle)
}

for(auto & node : children_) {
node.set_visible_rectangle(rectangle);
node->set_visible_rectangle(rectangle);
}
}

Expand All @@ -585,7 +586,7 @@ void tree_view_node::impl_draw_children(surface& frame_buffer,
}

for(auto & node : children_) {
node.impl_draw_children(frame_buffer, x_offset, y_offset);
node->impl_draw_children(frame_buffer, x_offset, y_offset);
}
}

Expand Down Expand Up @@ -685,7 +686,7 @@ const std::string& tree_view_node::get_control_type() const
tree_view_node& tree_view_node::get_child_at(int index)
{
assert(static_cast<size_t>(index) < children_.size());
return children_[index];
return *children_[index];
}

std::vector<int> tree_view_node::describe_path()
Expand All @@ -696,7 +697,7 @@ std::vector<int> tree_view_node::describe_path()

std::vector<int> res = parent_node_->describe_path();
for(size_t i = 0; i < parent_node_->count_children(); ++i) {
if(&parent_node_->children_[i] == this) {
if(parent_node_->children_[i].get() == this) {
res.push_back(i);
return res;
}
Expand All @@ -714,11 +715,11 @@ int tree_view_node::calculate_ypos()

int res = parent_node_->calculate_ypos();
for(const auto& node : parent_node_->children_) {
if(&node == this) {
if(node.get() == this) {
break;
}

res += node.get_current_size(true).y;
res += node->get_current_size(true).y;
}

return res;
Expand All @@ -740,11 +741,11 @@ tree_view_node* tree_view_node::get_node_above()

tree_view_node* cur = nullptr;
for(size_t i = 0; i < parent_node_->count_children(); ++i) {
if(&parent_node_->children_[i] == this) {
if(parent_node_->children_[i].get() == this) {
if(i == 0) {
return parent_node_->is_root_node() ? nullptr : parent_node_;
} else {
cur = &parent_node_->children_[i - 1];
cur = parent_node_->children_[i - 1].get();
break;
}
}
Expand All @@ -769,9 +770,9 @@ tree_view_node* tree_view_node::get_node_below()
tree_view_node& parent = *cur->parent_node_;

for(size_t i = 0; i < parent.count_children(); ++i) {
if(&parent.children_[i] == cur) {
if(parent.children_[i].get() == cur) {
if(i < parent.count_children() - 1) {
return &parent.children_[i + 1];
return parent.children_[i + 1].get();
} else {
cur = &parent;
}
Expand Down Expand Up @@ -833,8 +834,13 @@ void tree_view_node::layout_initialize(const bool full_initialization)

// Clear child caches.
for(auto & child : children_) {
child.layout_initialize(full_initialization);
child->layout_initialize(full_initialization);
}
}

iteration::walker_base* tree_view_node::create_walker()
{
return new gui2::iteration::tree_node(*this, children_);
}

} // namespace gui2
11 changes: 3 additions & 8 deletions src/gui/widgets/tree_view_node.hpp
View file
Expand Up @@ -16,9 +16,7 @@

#include "gui/widgets/widget.hpp"
#include "gui/widgets/grid.hpp"
#include "gui/auxiliary/iterator/walker_tree_node.hpp"

#include <boost/ptr_container/ptr_vector.hpp>
#include <memory>

namespace gui2
{
Expand All @@ -36,7 +34,7 @@ class tree_view_node : public widget
friend class tree_view;

public:
using node_children_vector = boost::ptr_vector<tree_view_node>;
using node_children_vector = std::vector<std::unique_ptr<tree_view_node>>;

bool operator==(const tree_view_node& node)
{
Expand Down Expand Up @@ -140,10 +138,7 @@ class tree_view_node : public widget
*
* @todo Implement properly.
*/
virtual iteration::walker_base* create_walker() override
{
return new gui2::iteration::tree_node(*this, children_);
}
virtual iteration::walker_base* create_walker() override;

node_children_vector& children()
{
Expand Down

7 comments on commit cba7493

@Vultraz
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the commit message I meant to say I don't know if it would be better to just store the nodes directly in the vector instead of as pointers.

@jyrkive
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to store them directly in the vector. A vector of unique_ptrs already owns the nodes. Storing the nodes in the vector wouldn't change the ownership situation, but it would remove a level of indirection. Accessing a node wouldn't require reading a pointer from the vector and then dereferencing it.

@gfgtdf
Copy link
Contributor

@gfgtdf gfgtdf commented on cba7493 Sep 11, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why we store then as pointers is that other code might hold references/pointers to the tree_view_node objects (for example node::parent_node_) and if we stored them directly in the vector those pointers might be corupted if nodes are added/removed.

@jyrkive
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gfgtdf Can't we use std::list, then?

@gfgtdf
Copy link
Contributor

@gfgtdf gfgtdf commented on cba7493 Sep 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what would be the advantage of that? vector is the default way to go simply necause it's faster.

@jyrkive
Copy link
Member

@jyrkive jyrkive commented on cba7493 Sep 11, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vector of unique_ptr is slower than list because of double indirection, and also slightly harder to use.

And if you're asking about the difference to vector, a list doesn't move its elements around in memory. A reference to an object stored in a list remains valid as long as the object isn't removed from the list.

@CelticMinstrel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on the requirements, std::deque may also be an option. As long as you only use push_back, pop_back, push_front, and pop_front, an std::deque does not move its elements in memory, so pointers to elements remain stable until they are removed. An std::deque may also be slightly faster than std::list since it doesn't have indirection between every element.

Please sign in to comment.