Merge pull request #335 from gpospelov/InsertItemRefactoring

Insert item refactoring

source/libmvvm_model/mvvm/commands/copyitemcommand.cpp

@@ -48,22 +48,17 @@ CopyItemCommand::~CopyItemCommand() = default;
 void CopyItemCommand::undo_command()
 {
     auto parent = itemFromPath(p_impl->item_path);
-    delete parent->takeItem(p_impl->tagrow);
+    parent->takeItem(p_impl->tagrow);
     setResult(nullptr);
 }
 
 void CopyItemCommand::execute_command()
 {
     auto parent = itemFromPath(p_impl->item_path);
-    auto item = p_impl->backup_strategy->restoreItem();
-    if (parent->insertItem(item.get(), p_impl->tagrow)) {
-        auto result = item.release();
-        setResult(result);
-    }
-    else {
-        setResult(nullptr);
-        setObsolete(true);
-    }
+    auto item = parent->insertItem(p_impl->backup_strategy->restoreItem(), p_impl->tagrow);
+    // FIXME revise behaviour in the case of invalid operation. Catch or not here?
+    setResult(item);
+    setObsolete(!item); // command is osbolete if insertion failed
 }
 
 namespace {

source/libmvvm_model/mvvm/commands/insertnewitemcommand.cpp

@@ -46,7 +46,6 @@ void InsertNewItemCommand::undo_command()
     // saving identifier for later redo
     if (p_impl->initial_identifier.empty())
         p_impl->initial_identifier = item->identifier();
-    delete item;
     setResult(nullptr);
 }
 

source/libmvvm_model/mvvm/commands/moveitemcommand.cpp

@@ -68,7 +68,7 @@ void MoveItemCommand::undo_command()
 
     // then make manipulations
     auto taken = current_parent->takeItem(p_impl->target_tagrow);
-    target_parent->insertItem(taken, p_impl->original_tagrow);
+    target_parent->insertItem(std::move(taken), p_impl->original_tagrow);
 
     // adjusting new addresses
     p_impl->target_parent_path = pathFromItem(current_parent);
@@ -87,8 +87,7 @@ void MoveItemCommand::execute_command()
     if (!taken)
         throw std::runtime_error("MoveItemCommand::execute() -> Can't take an item.");
 
-    bool succeeded = target_parent->insertItem(taken, p_impl->target_tagrow);
-    if (!succeeded)
+    if (!target_parent->insertItem(std::move(taken), p_impl->target_tagrow))
         throw std::runtime_error("MoveItemCommand::execute() -> Can't insert item.");
 
     // adjusting new addresses

source/libmvvm_model/mvvm/commands/removeitemcommand.cpp

@@ -43,19 +43,16 @@ RemoveItemCommand::~RemoveItemCommand() = default;
 void RemoveItemCommand::undo_command()
 {
     auto parent = itemFromPath(p_impl->item_path);
-    auto reco_item = p_impl->backup_strategy->restoreItem();
-    parent->insertItem(reco_item.release(), p_impl->tagrow);
+    parent->insertItem(p_impl->backup_strategy->restoreItem(), p_impl->tagrow);
 }
 
 void RemoveItemCommand::execute_command()
 {
     auto parent = itemFromPath(p_impl->item_path);
     if (auto child = parent->takeItem(p_impl->tagrow); child) {
-        p_impl->backup_strategy->saveItem(child);
-        delete child;
+        p_impl->backup_strategy->saveItem(child.get());
         setResult(true);
-    }
-    else {
+    } else {
         setResult(false);
         setObsolete(true);
     }

source/libmvvm_model/mvvm/model/compounditem.h

@@ -24,9 +24,10 @@ class MVVM_MODEL_EXPORT CompoundItem : public SessionItem {
 public:
     CompoundItem(const std::string& modelType = Constants::CompoundItemType);
 
-    //! Adds property item of given type.
+    //! Adds property item of given type and register it under the given 'name'.
     template <typename T = PropertyItem> T* addProperty(const std::string& name);
 
+    //! Adds PropertyItem and sets its value to 'value'.
     template <typename V> PropertyItem* addProperty(const std::string& name, const V& value);
 
     //! Register char property. Special case to turn it into std::string.
@@ -37,30 +38,27 @@ class MVVM_MODEL_EXPORT CompoundItem : public SessionItem {
 
 template <typename T> T* CompoundItem::addProperty(const std::string& name)
 {
-    T* property = new T;
-    registerTag(TagInfo::propertyTag(name, property->modelType()));
-    property->setDisplayName(name);
-    insertItem(property, {name, 0});
-    return property;
+    registerTag(TagInfo::propertyTag(name, T().modelType()));
+    auto result = insertItem<T>({name, 0});
+    result->setDisplayName(name);
+    return result;
 }
 
 inline PropertyItem* CompoundItem::addProperty(const std::string& name, const char* value)
 {
+    // Consider merging with the method ::addProperty(const std::string& name, const V& value).
+    // Currently it is not possible because of QVariant dependency. It converts 'const char*'
+    // to QString, and we want std::string.
     return addProperty(name, std::string(value));
 }
 
 template <typename V>
 PropertyItem* CompoundItem::addProperty(const std::string& name, const V& value)
 {
-    auto property = new PropertyItem;
-    registerTag(TagInfo::propertyTag(name, property->modelType()));
-    property->setDisplayName(name);
+    auto property = addProperty<PropertyItem>(name);
     property->setData(value);
-
     if constexpr (std::is_floating_point_v<V>)
         property->setData(RealLimits::limitless(), ItemDataRole::LIMITS);
-
-    insertItem(property, {name, 0});
     return property;
 }
 

source/libmvvm_model/mvvm/model/groupitem.cpp

@@ -18,7 +18,7 @@ using namespace ModelView;
 namespace {
 
 //! Returns vector of model types for given vector of items.
-std::vector<std::string> modelTypes(const std::vector<SessionItem*> items)
+std::vector<std::string> modelTypes(const std::vector<SessionItem*>& items)
 {
     std::vector<std::string> result;
     std::transform(items.begin(), items.end(), std::back_inserter(result),
@@ -29,9 +29,7 @@ std::vector<std::string> modelTypes(const std::vector<SessionItem*> items)
 
 GroupItem::~GroupItem() = default;
 
-GroupItem::GroupItem(model_type modelType)
-    : SessionItem(std::move(modelType))
-    , m_index_to_select(0)
+GroupItem::GroupItem(model_type modelType) : SessionItem(std::move(modelType)), m_index_to_select(0)
 {
     registerTag(TagInfo::universalTag(T_GROUP_ITEMS), /*set_as_default*/ true);
     setData(ComboProperty());
@@ -46,7 +44,7 @@ int GroupItem::currentIndex() const
 
 const SessionItem* GroupItem::currentItem() const
 {
-    return isValidIndex() ? getItem("", currentIndex()) : nullptr;
+    return currentIndex() != -1 ? getItem("", currentIndex()) : nullptr;
 }
 
 SessionItem* GroupItem::currentItem()
@@ -67,7 +65,6 @@ void GroupItem::setCurrentType(const std::string& model_type)
     if (index == -1)
         throw std::runtime_error("GroupItem::setCurrentType() -> Model type '" + model_type
                                  + "' doesn't belong to the group");
-
     setCurrentIndex(index);
 }
 
@@ -78,11 +75,6 @@ void GroupItem::setCurrentIndex(int index)
     setData(combo, ItemDataRole::DATA);
 }
 
-bool GroupItem::isValidIndex() const
-{
-    return currentIndex() != -1;
-}
-
 //! Updates internal data representing selection of items, and current selection.
 //! To be called during GroupItem's construction.
 

source/libmvvm_model/mvvm/model/groupitem.h

@@ -35,7 +35,6 @@ class MVVM_MODEL_EXPORT GroupItem : public SessionItem {
 protected:
     GroupItem(model_type modelType);
     void setCurrentIndex(int index);
-    bool isValidIndex() const;
     template <typename T> void addToGroup(const std::string& text = {}, bool make_selected = false);
     void updateCombo();
 
@@ -48,13 +47,10 @@ class MVVM_MODEL_EXPORT GroupItem : public SessionItem {
 //! @param make_selected defines whether the item should be selected by default.
 template <typename T> void GroupItem::addToGroup(const std::string& text, bool make_selected)
 {
-    auto item = std::make_unique<T>();
-    std::string item_text = text.empty() ? item->modelType() : text;
-    m_item_text.push_back(item_text);
-    insertItem(item.release(), TagRow::append(T_GROUP_ITEMS));
+    m_item_text.push_back(text.empty() ? T().modelType() : text);
+    insertItem<T>(TagRow::append(T_GROUP_ITEMS));
     if (make_selected)
         m_index_to_select = m_item_text.size() - 1;
-
     updateCombo();
 }
 

source/libmvvm_model/mvvm/model/sessionitem.cpp

@@ -205,12 +205,23 @@ SessionItemTags* SessionItem::itemTags()
     return const_cast<SessionItemTags*>(static_cast<const SessionItem*>(this)->itemTags());
 }
 
-//! Insert item into given tag under the given row.
+//! Inserts the item into the given tag under the given row.
+//! Returns 'true' in the case of success, take ownership over the item.
+//! If an item can't be inserted for a given TagRow (i.e. when the container is full, or not
+//! intended for items of a given type) will return false and will not take ownership.
 
 bool SessionItem::insertItem(SessionItem* item, const TagRow& tagrow)
 {
-    // think of passing unique_ptr directly
+    if (!p_impl->m_tags->canInsertItem(item, tagrow))
+        return false;
+    return insertItem(std::unique_ptr<SessionItem>(item), tagrow) != nullptr;
+}
+
+//! Insert item into given tag under the given row. Will take ownership of inserted item.
+//! Returns back a pointer to the same item for convenience.
 
+SessionItem* SessionItem::insertItem(std::unique_ptr<SessionItem> item, const TagRow& tagrow)
+{
     if (!item)
         throw std::runtime_error("SessionItem::insertItem() -> Invalid item.");
 
@@ -220,27 +231,31 @@ bool SessionItem::insertItem(SessionItem* item, const TagRow& tagrow)
     if (item->model())
         throw std::runtime_error("SessionItem::insertItem() -> Existing model.");
 
-    auto result = p_impl->m_tags->insertItem(item, tagrow);
-    if (result) {
-        item->setParent(this);
-        item->setModel(model());
+    if (!p_impl->m_tags->canInsertItem(item.get(), tagrow))
+        throw std::runtime_error("SessionItem::insertItem() -> Can't insert item.");
 
-        if (p_impl->m_model) {
-            // FIXME think of actual_tagrow removal if input tag,row will be always valid
-            auto actual_tagrow = tagRowOfItem(item);
-            p_impl->m_model->mapper()->callOnItemInserted(this, actual_tagrow);
-        }
+    auto result = item.release();
+    p_impl->m_tags->insertItem(result, tagrow);
+    result->setParent(this);
+    result->setModel(model());
+
+    if (p_impl->m_model) {
+        // FIXME think of actual_tagrow removal if input tag,row will be always valid
+        auto actual_tagrow = tagRowOfItem(result);
+        p_impl->m_model->mapper()->callOnItemInserted(this, actual_tagrow);
     }
 
     return result;
 }
 
+
 //! Removes item from given row from given tag, returns it to the caller.
+//! Ownership is granted to the caller.
 
-SessionItem* SessionItem::takeItem(const TagRow& tagrow)
+std::unique_ptr<SessionItem> SessionItem::takeItem(const TagRow& tagrow)
 {
     if (!p_impl->m_tags->canTakeItem(tagrow))
-        return nullptr;
+        return {};
 
     if (p_impl->m_model)
         p_impl->m_model->mapper()->callOnItemAboutToBeRemoved(this, tagrow);
@@ -252,7 +267,7 @@ SessionItem* SessionItem::takeItem(const TagRow& tagrow)
     if (p_impl->m_model)
         p_impl->m_model->mapper()->callOnItemRemoved(this, tagrow);
 
-    return result;
+    return std::unique_ptr<SessionItem>(result);
 }
 
 //! Returns true if this item has `editable` flag set.

source/libmvvm_model/mvvm/model/sessionitem.h

@@ -90,7 +90,10 @@ class MVVM_MODEL_EXPORT SessionItem {
 
     bool insertItem(SessionItem* item, const TagRow& tagrow);
 
-    SessionItem* takeItem(const TagRow& tagrow);
+    SessionItem* insertItem(std::unique_ptr<SessionItem> item, const TagRow& tagrow);
+    template <typename T = SessionItem> T* insertItem(const TagRow& tagrow);
+
+    std::unique_ptr<SessionItem> takeItem(const TagRow& tagrow);
 
     // more convenience methods
 
@@ -173,6 +176,14 @@ template <typename T> std::vector<T*> SessionItem::items(const std::string& tag)
     return result;
 }
 
+//! Creates a new item and insert it into given tag under the given row.
+//! Returns pointer to inserted item to the user.
+
+template <typename T> inline T* SessionItem::insertItem(const TagRow& tagrow)
+{
+    return static_cast<T*>(insertItem(std::make_unique<T>(), tagrow));
+}
+
 //! Returns data stored in property item.
 //! Property is single item registered under certain tag via CompoundItem::addProperty method.
 

source/libmvvm_model/mvvm/model/sessionitemtags.cpp

@@ -59,28 +59,39 @@ int SessionItemTags::itemCount(const std::string& tag_name) const
     return container(tag_name)->itemCount();
 }
 
+//! Returns true if item can be inserted.
+
+bool SessionItemTags::canInsertItem(const SessionItem* item, const TagRow &tagrow) const
+{
+    auto tag_container = container(tagrow.tag);
+    // negative row means appending to the vector
+    auto row = tagrow.row < 0 ? tag_container->itemCount() : tagrow.row;
+    return container(tagrow.tag)->canInsertItem(item, row);
+}
+
 //! Inserts item in container with given tag name and at given row.
 //! Returns true in the case of success. If tag name is empty, default tag will be used.
 
 bool SessionItemTags::insertItem(SessionItem* item, const TagRow& tagrow)
 {
     auto tag_container = container(tagrow.tag);
+    // negative row means appending to the vector
     auto row = tagrow.row < 0 ? tag_container->itemCount() : tagrow.row;
     return container(tagrow.tag)->insertItem(item, row);
 }
 
-//! Removes item at given row and for given tag, returns it to the user.
+//! Returns true if item can be taken.
 
-SessionItem* SessionItemTags::takeItem(const TagRow& tagrow)
+bool SessionItemTags::canTakeItem(const TagRow& tagrow) const
 {
-    return container(tagrow.tag)->takeItem(tagrow.row);
+    return container(tagrow.tag)->canTakeItem(tagrow.row);
 }
 
-//! Returns true if item can be taken.
+//! Removes item at given row and for given tag, returns it to the user.
 
-bool SessionItemTags::canTakeItem(const TagRow& tagrow) const
+SessionItem* SessionItemTags::takeItem(const TagRow& tagrow)
 {
-    return container(tagrow.tag)->canTakeItem(tagrow.row);
+    return container(tagrow.tag)->takeItem(tagrow.row);
 }
 
 //! Returns item at given row of given tag.

source/libmvvm_model/mvvm/model/sessionitemtags.h

@@ -47,12 +47,14 @@ class MVVM_MODEL_EXPORT SessionItemTags {
 
     // adding and removal
 
-    bool insertItem(SessionItem* item, const TagRow& tagrow);
+    bool canInsertItem(const SessionItem *item, const TagRow& tagrow) const;
 
-    SessionItem* takeItem(const TagRow& tagrow);
+    bool insertItem(SessionItem* item, const TagRow& tagrow);
 
     bool canTakeItem(const TagRow& tagrow) const;
 
+    SessionItem* takeItem(const TagRow& tagrow);
+
     // item access
     SessionItem* getItem(const TagRow& tagrow) const;
 

source/libmvvm_model/mvvm/model/tagrow.h

@@ -16,6 +16,7 @@
 namespace ModelView {
 
 //! Aggregate to hold (tag, row) information for SessionModel.
+//! row=-1 is a special value for appending to the end of the in the SessionIteTags context.
 
 class MVVM_MODEL_EXPORT TagRow {
 public:

source/libmvvm_model/mvvm/serialization/jsonmodelconverter.cpp

@@ -80,8 +80,7 @@ void JsonModelConverter::from_json(const QJsonObject& json, SessionModel& model)
 
     auto rebuild_root = [&json, &itemConverter](auto parent) {
         for (const auto ref : json[JsonItemFormatAssistant::itemsKey].toArray()) {
-            auto item = itemConverter->from_json(ref.toObject());
-            parent->insertItem(item.release(), TagRow::append());
+            parent->insertItem(itemConverter->from_json(ref.toObject()), TagRow::append());
         }
     };
     model.clear(rebuild_root);