Skip to content

Conversation

@MaxGavr
Copy link
Contributor

@MaxGavr MaxGavr commented Aug 29, 2017

Изменения:

Close #423.

IvanKosik and others added 30 commits May 13, 2013 03:53
fixed some build errors in scs plugin
removed dynamic_cast
fixed code style
Added asynchronous SCs parser.
Updated SCs errors analyzer and autocompletion.
Fixed: set focus on SCs editor when creating a new file or load file.
Fixed some small errors.
Code refactoring. Add utf8 encoding

foreach (SCgObject* object, objectList)
if (!object || (object->type() != objectList[0]->type()))
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

for (size_t i = 1; i < objectsList.size(); ++i)
{
    SCgObject const * obj = objectList[i];
    Q_ASSERT(obj); // we should not have nullptr in a list
    if (obj->type() != objectList[0]->type())
        return false;
}

bool SCgObject::areObjectsOfEqualType(const SCgObject::SCgObjectList& objectList)
{
Q_ASSERT_X(!objectList.isEmpty(), "SCgObject::areObjectsOfEqualType",
"Ambiguous case: could not determine type equality for empty list of objects");
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this assert

QList<QGraphicsItem*> itemList = selectedItems();
SCgObject::SCgObjectList objectList;

foreach (QGraphicsItem* item, itemList)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use foreach
use C++11 instead

for (QGraphicsItem* item : itemList)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also there we doesn't change item, so it should be QGraphicsItem const * item

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quote from Qt Docs:

An alternative to Qt's foreach loop is the range-based for that is part of C++ 11 and newer. However, keep in mind that the range-based for might force a Qt container to detach, whereas foreach would not. But using foreach always copies the container, which is usually not cheap for STL containers. If in doubt, prefer foreach for Qt containers, and range based for for STL ones.

Detaching a container causes its deep copy. Therefore, foreach is preferable for Qt containers, that are used throughout the project (along with foreach).
Speaking about c++11, it is not yet included to the project. This should be done shortly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add support of C++ 11 and use for (auto it : container)

objectList.append(static_cast<SCgObject*>(item));

return objectList;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

rewrite

SCgBaseCommand* SCgScene::deleteSelObjectsCommand(SCgBaseCommand* parentCmd, bool addToStack)

using this function inside

SCgBaseCommand* cmd = new SCgBaseCommand(this, 0, parentCmd);
cmd->setText(QObject::tr("Change type of multiple objects"));

foreach (SCgObject* object, objList)
Copy link
Contributor

Choose a reason for hiding this comment

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

for (SCgObject* object : objList)

if (parentLayout)
parentLayout->addLayout(buttonLayout);
else
return;
Copy link
Contributor

@deniskoronchik deniskoronchik Aug 30, 2017

Choose a reason for hiding this comment

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

Strange logic. There should be exception error in this case or Q_ASSERT(parentLayout). I mean remove else return; and replace with check


SCgScene* SCgView::getSCgScene() const
{
QGraphicsScene* scene = QGraphicsView::scene();
Copy link
Contributor

Choose a reason for hiding this comment

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

return static_cast<SCgScene*>(scene());

(*it)->setSelected(true);
}

bool SCgView::typeCanBeChanged() const
Copy link
Contributor

@deniskoronchik deniskoronchik Aug 30, 2017

Choose a reason for hiding this comment

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

bool canChangeTypesOfSelection(const SCgObject::SCgObjectList & objectList)

{
if (!mContextObject)
return;
SCgObject::SCgObjectList objectList = getSCgScene()->getSelectedObjects();
Copy link
Contributor

Choose a reason for hiding this comment

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

SCgObject::SCgObjectList const objectList = getSCgScene()->getSelectedObjects();
Q_ASSERT(canChangeTypes(objectList));

if (objectList.length() == 1)
changeType(objectList.first(), typeDialog.getChosenType());
else
changeType(objectList, typeDialog.getChosenType());
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace to one command:

changeType(objectList, typeDialog.getChosenType());

Doesn't need to process one item separately.

Copy link
Contributor Author

@MaxGavr MaxGavr Sep 2, 2017

Choose a reason for hiding this comment

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

I agree, that functions interface should be reduced to just

SCgScene::changeObjectTypeCommand(const SCgObject::SCgObjectList& objectList, ...)

But inside it, single-object case should be separated, as it is handled differently (without creating parent QUndoCommand) and is widely used by a user.

Copy link
Contributor

Choose a reason for hiding this comment

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

That not a problem, you should think, that change type of one element - is a command to change type of N elements, where N == 1.
Main goal to make code more suitable for support. This is not critical code for a memory and performance. So minimize number of code there

Q_ASSERT(mContextObject);
if (object)
getSCgScene()->changeObjectTypeCommand(object, newType);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this function

}


SCgBaseCommand* SCgScene::changeObjectTypeCommand(SCgObject *object, const QString &type, SCgBaseCommand* parentCmd, bool addToStack)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants