Invert Selection #1428
Invert Selection #1428
Conversation
| /*}*/ | ||
| } else if (ObjectGroup *objectGroup = layer->asObjectGroup()) { | ||
| QList<MapObject*> allObjects = objectGroup->objects(); | ||
| QList<MapObject*> selectedObjects = mMapDocument->selectedObjects(); |
There was a problem hiding this comment.
Please use const & here since you're not going to modify these containers.
There was a problem hiding this comment.
Do you mean const QList<MapObject*> allObjects = objectGroup->objects(); ?
There was a problem hiding this comment.
No, I meant:
const QList<MapObject*> &allObjects = objectGroup->objects();though you may also write:
const auto &allObjects = objectGroup->objects();The point is to avoid the implicit copy, and to make sure const iterators are used (avoiding a deep copy).
| QList<MapObject*> notSelectedObjects; | ||
|
|
||
| QList<MapObject*>::iterator i; | ||
| for(i=allObjects.begin(); i != allObjects.end(); ++i) |
There was a problem hiding this comment.
Please use a modern for-loop (and place opening { on the same line):
for (MapObject *mapObject : allObjects) {| selectAll(); | ||
| } else if(mMapDocument->selectedArea() == all) { | ||
| selectNone(); | ||
| } else {*/ |
There was a problem hiding this comment.
These special cases need not to be made, because that's what should be happening automatically, right? Please drop the commented out code.
There was a problem hiding this comment.
Yes exactly, I forgot to remove those comments.
| mMapDocument->undoStack()->push(command); | ||
| } else if (layer->asObjectGroup()) { | ||
| QList<MapObject*> emptyListOfObjects; | ||
| mMapDocument->setSelectedObjects(emptyListOfObjects); |
There was a problem hiding this comment.
Thanks for fixing the unselect for object selection! Btw, you can write:
mMapDocument->setSelectedObjects(QList<MapObject*>());There was a problem hiding this comment.
Yes, I agree sometimes I do this kind of things for the sake of clarity.
| @@ -555,7 +606,19 @@ void MapDocumentActionHandler::updateActions() | |||
| } | |||
|
|
|||
| mActionSelectAll->setEnabled(map); | |||
| mActionSelectNone->setEnabled(!selection.isEmpty()); | |||
|
|
|||
| if(currentLayer) { | |||
There was a problem hiding this comment.
Please put a space between if and (.
| if (currentLayer->asTileLayer()) { | ||
| mActionSelectNone->setEnabled(!selection.isEmpty()); | ||
| } else if (currentLayer->asObjectGroup()) { | ||
| mActionSelectNone->setEnabled(mMapDocument->selectedObjects().count() > 0); |
There was a problem hiding this comment.
Please also use selectedObjectsCount here, because mMapDocument may be null.
|
I made the requested changes |
| QList<MapObject*> selectedObjects = mMapDocument->selectedObjects(); | ||
| QList<MapObject*> notSelectedObjects; | ||
|
|
||
| for (auto& mapObject : allObjects) { |
There was a problem hiding this comment.
Please don't write a & here because you're creating a reference to a pointer. Just auto is enough.
| /*}*/ | ||
| } else if (ObjectGroup *objectGroup = layer->asObjectGroup()) { | ||
| QList<MapObject*> allObjects = objectGroup->objects(); | ||
| QList<MapObject*> selectedObjects = mMapDocument->selectedObjects(); |
There was a problem hiding this comment.
No, I meant:
const QList<MapObject*> &allObjects = objectGroup->objects();though you may also write:
const auto &allObjects = objectGroup->objects();The point is to avoid the implicit copy, and to make sure const iterators are used (avoiding a deep copy).
| QList<MapObject*> notSelectedObjects; | ||
|
|
||
| for (auto& mapObject : allObjects) { | ||
| if(!selectedObjects.contains(mapObject)) { |
There was a problem hiding this comment.
Please put a space between if and (.
|
Looks good, thanks! |
To fix this issue:
#1423