Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add objects filter to the objects dock #1566

Closed
wants to merge 30 commits into from
Closed

Add objects filter to the objects dock #1566

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
bd138a3
Add the line edit
thabetx May 7, 2017
9b2c9a5
New filter
thabetx May 7, 2017
59d44b4
Initial working version
thabetx May 9, 2017
a821e3d
Fix object selection from map
thabetx May 9, 2017
eb8e1ae
Fix saving and restoring expanded groups
thabetx May 9, 2017
afaf337
Make the models private
thabetx May 9, 2017
6d42d49
Fix style
thabetx May 9, 2017
0553dbb
Update filtering logic
thabetx May 10, 2017
2ffaefd
Add keyboard shortcuts to the objects dock
thabetx May 10, 2017
eeb0dd7
Fix style
thabetx May 10, 2017
e72855d
Save filter string between maps
thabetx May 10, 2017
6ebcdf9
Refactoring
thabetx May 12, 2017
a46173b
Use returnPressed signal instead of handling return key
thabetx May 12, 2017
6b522ae
Expand groups having matching objects
thabetx May 12, 2017
4dc0fdb
Restore the prefilter expansion state when filter is canceled
thabetx May 13, 2017
e425e53
Fix style
thabetx May 14, 2017
12eaea7
Use Filter string from TileStampsDock
thabetx May 16, 2017
5f6f1df
Refactor the switching code
thabetx May 16, 2017
184bff3
Prevent collapsing while filtering
thabetx May 16, 2017
d302c97
Fix spaces
thabetx May 17, 2017
3c0a470
Use nullptr instead of 0 for initializing ObjectsFilterModel
thabetx May 17, 2017
9a51d46
Rename mProxyModel to mReversingProxyModel
thabetx May 17, 2017
e4f3b46
Reverse the order of the proxy models
thabetx May 17, 2017
5a59bd7
Rename proxyIndex to viewIndex
thabetx May 17, 2017
fd400ad
Create mapToViewModel function
thabetx May 17, 2017
1ad9ef2
Refactor getGroupIndex code
thabetx May 18, 2017
a440d16
Merge remote-tracking branch 'upstream/master' into new-filter
thabetx May 18, 2017
b62505e
Filter using objects instead of indices
thabetx May 18, 2017
04cbe51
Rename getGroupIndex to groupIndex
thabetx May 18, 2017
7bdad99
Fixed the logic in ReversingProxyModel::sourceRowsAboutToBeInserted
bjorn May 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
152 changes: 124 additions & 28 deletions src/tiled/objectsdock.cpp
View file
Open in desktop
Expand Up @@ -37,7 +37,9 @@
#include <QBoxLayout>
#include <QContextMenuEvent>
#include <QHeaderView>
#include <QKeyEvent>
#include <QLabel>
#include <QLineEdit>
#include <QMenu>
#include <QSettings>
#include <QToolBar>
Expand All @@ -52,6 +54,7 @@ using namespace Tiled::Internal;

ObjectsDock::ObjectsDock(QWidget *parent)
: QDockWidget(parent)
, mFilterEdit(new QLineEdit(this))
, mObjectsView(new ObjectsView)
, mMapDocument(nullptr)
{
Expand All @@ -64,10 +67,16 @@ ObjectsDock::ObjectsDock(QWidget *parent)

MapDocumentActionHandler *handler = MapDocumentActionHandler::instance();

mFilterWasEmpty[mMapDocument] = true;
mFilterEdit->setClearButtonEnabled(true);
connect(mFilterEdit, &QLineEdit::textChanged, this, &ObjectsDock::filterObjects);
connect(mFilterEdit, &QLineEdit::returnPressed, [&] { mObjectsView->setFocus(); });

QWidget *widget = new QWidget(this);
QVBoxLayout *layout = new QVBoxLayout(widget);
layout->setMargin(0);
layout->setSpacing(0);
layout->addWidget(mFilterEdit);
layout->addWidget(mObjectsView);

mActionNewLayer = new QAction(this);
Expand Down Expand Up @@ -136,7 +145,8 @@ void ObjectsDock::moveObjectsDown()
void ObjectsDock::setMapDocument(MapDocument *mapDoc)
{
if (mMapDocument) {
saveExpandedGroups();
mFilterStrings[mMapDocument] = mFilterEdit->text();
saveExpandedGroups(mExpandedGroups);
mMapDocument->disconnect(this);
}

Expand All @@ -145,7 +155,8 @@ void ObjectsDock::setMapDocument(MapDocument *mapDoc)
mObjectsView->setMapDocument(mapDoc);

if (mMapDocument) {
restoreExpandedGroups();
mFilterEdit->setText(mFilterStrings.take(mMapDocument));
restoreExpandedGroups(mExpandedGroups);
connect(mMapDocument, SIGNAL(selectedObjectsChanged()),
this, SLOT(updateActions()));
}
Expand Down Expand Up @@ -173,6 +184,7 @@ void ObjectsDock::retranslateUi()
mActionObjectProperties->setToolTip(tr("Object Properties"));
mActionMoveUp->setToolTip(tr("Move Objects Up"));
mActionMoveDown->setToolTip(tr("Move Objects Down"));
mFilterEdit->setPlaceholderText(tr("Filter"));
bjorn marked this conversation as resolved.
Show resolved Hide resolved

updateActions();
}
Expand Down Expand Up @@ -221,47 +233,90 @@ void ObjectsDock::objectProperties()
emit mMapDocument->editCurrentObject();
}

void ObjectsDock::saveExpandedGroups()
QModelIndex ObjectsDock::getGroupIndex(ObjectGroup *og)
{
mExpandedGroups[mMapDocument].clear();

const auto proxyModel = static_cast<QAbstractProxyModel*>(mObjectsView->model());
const QModelIndex sourceIndex = mMapDocument->mapObjectModel()->index(og);
return proxyModel->mapFromSource(mObjectsView->proxyModel()->mapFromSource(sourceIndex));
}

void ObjectsDock::saveExpandedGroups(QMap<MapDocument*, QList<ObjectGroup*> > &expansionState)
{
expansionState[mMapDocument].clear();

const auto &objectGroups = mMapDocument->map()->objectGroups();

for (ObjectGroup *og : objectGroups) {
const QModelIndex sourceIndex = mMapDocument->mapObjectModel()->index(og);
const QModelIndex index = proxyModel->mapFromSource(sourceIndex);
if (mObjectsView->isExpanded(index))
mExpandedGroups[mMapDocument].append(og);
}
for (ObjectGroup *og : objectGroups)
if (mObjectsView->isExpanded(getGroupIndex(og)))
expansionState[mMapDocument].append(og);
}

void ObjectsDock::restoreExpandedGroups()
void ObjectsDock::restoreExpandedGroups(QMap<MapDocument*, QList<ObjectGroup*> > &expansionState)
{
const auto objectGroups = mExpandedGroups.take(mMapDocument);
for (ObjectGroup *og : objectGroups) {
const QModelIndex sourceIndex = mMapDocument->mapObjectModel()->index(og);
const QModelIndex index = static_cast<QAbstractProxyModel*>(mObjectsView->model())->mapFromSource(sourceIndex);
mObjectsView->setExpanded(index, true);
}
const auto objectGroups = expansionState.take(mMapDocument);

for (ObjectGroup *og : objectGroups)
mObjectsView->setExpanded(getGroupIndex(og), true);
}

void ObjectsDock::documentAboutToClose(Document *document)
{
if (MapDocument *mapDocument = qobject_cast<MapDocument*>(document))
if (MapDocument *mapDocument = qobject_cast<MapDocument*>(document)) {
mFilterStrings.remove(mapDocument);
mExpandedGroups.remove(mapDocument);
}
}

void ObjectsDock::filterObjects()
{
// Save the prefilter expansion state if this is the first applied filter
if(mFilterWasEmpty[mMapDocument]) {
mFilterWasEmpty[mMapDocument] = false;
Copy link
Member

Choose a reason for hiding this comment

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

Could we check the state of the model instead? I think mObjectsView->objectsFilterModel()->filterRegExp().isEmpty() should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried doing that but couldn't get it to work properly.

The filter is shared between documents, so suppose we switched from a non-filtered map to a filtered map, the above condition will be satisfied and the state of the new map will be saved overwriting the original saved state, I couldn't figure a clean way to prevent this from happening.

I changed the mFilterWasEmpty map to a single bool variable and got it work properly but had to introduce a small hack for this particular case.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, makes sense, thanks for the explanation!

saveExpandedGroups(mPrefilterExpandedGroups);
}

mObjectsView->objectsFilterModel()->setFilterFixedString(mFilterEdit->text());

if(mFilterEdit->text().isEmpty()) { // Resotre the prefilter expansion state
Copy link
Member

Choose a reason for hiding this comment

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

Coding style: please put a space after if (also fix other three cases)

Typo: Resotre

mFilterWasEmpty[mMapDocument] = true;
const auto &objectGroups = mMapDocument->map()->objectGroups();

// Collapse all groups so expansion is restored to the original state
for (ObjectGroup *og : objectGroups)
mObjectsView->setExpanded(getGroupIndex(og), false);

restoreExpandedGroups(mPrefilterExpandedGroups);
} else { // Expand the filtered Groups
const auto &objectGroups = mMapDocument->map()->objectGroups();

for (ObjectGroup *og : objectGroups)
mObjectsView->setExpanded(getGroupIndex(og), true);
}
}

void ObjectsDock::keyPressEvent(QKeyEvent *event)
{
if (event->key() == Qt::Key_Escape)
mFilterEdit->clear();
else
QDockWidget::keyPressEvent(event);
}

///// ///// ///// ///// /////

ObjectsView::ObjectsView(QWidget *parent)
: QTreeView(parent)
, mMapDocument(nullptr)
, mObjectsFilterModel(new ObjectsFilterModel(this))
, mProxyModel(new ReversingProxyModel(this))
, mSynching(false)
{
setUniformRowHeights(true);
setModel(mProxyModel);

mObjectsFilterModel->setFilterCaseSensitivity(Qt::CaseInsensitive);
mObjectsFilterModel->setSourceModel(mProxyModel);
setModel(mObjectsFilterModel);

setItemDelegate(new EyeVisibilityDelegate(this));

setSelectionBehavior(QAbstractItemView::SelectRows);
Expand Down Expand Up @@ -317,7 +372,7 @@ MapObjectModel *ObjectsView::mapObjectModel() const

void ObjectsView::onPressed(const QModelIndex &proxyIndex)
{
const QModelIndex index = mProxyModel->mapToSource(proxyIndex);
const QModelIndex index = mObjectsFilterModel->mapToSource(proxyIndex);
Copy link
Member

Choose a reason for hiding this comment

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

While this seems to work, it is missing a translation step. The returned index is an index into the mProxyModel, which is the source model for mObjectsFilterModel. So this should be:

const QModelIndex index = mProxyModel->mapToSource(mObjectsFilterModel->mapToSource(proxyIndex));

There are more functions below missing this.

Since there are a lot of places where you have to make this double translation, I would suggest to introduce two helper functions, for example mapFromViewModel and mapToViewModel. Also, since there are two proxies now, a variable like proxyIndex should probably be renamed to viewIndex for clarity.


if (MapObject *mapObject = mapObjectModel()->toMapObject(index))
mMapDocument->setCurrentObject(mapObject);
Expand All @@ -327,7 +382,7 @@ void ObjectsView::onPressed(const QModelIndex &proxyIndex)

void ObjectsView::onActivated(const QModelIndex &proxyIndex)
{
const QModelIndex index = mProxyModel->mapToSource(proxyIndex);
const QModelIndex index = mObjectsFilterModel->mapToSource(proxyIndex);

if (MapObject *mapObject = mapObjectModel()->toMapObject(index)) {
mMapDocument->setCurrentObject(mapObject);
Expand Down Expand Up @@ -357,7 +412,7 @@ void ObjectsView::selectionChanged(const QItemSelection &selected,

QList<MapObject*> selectedObjects;
for (const QModelIndex &proxyIndex : selectedProxyRows) {
const QModelIndex index = mProxyModel->mapToSource(proxyIndex);
const QModelIndex index = mObjectsFilterModel->mapToSource(proxyIndex);

if (MapObject *o = mapObjectModel()->toMapObject(index))
selectedObjects.append(o);
Expand Down Expand Up @@ -385,7 +440,7 @@ void ObjectsView::selectedObjectsChanged()
const QList<MapObject *> &selectedObjects = mMapDocument->selectedObjects();
if (selectedObjects.count() == 1) {
MapObject *o = selectedObjects.first();
scrollTo(mProxyModel->mapFromSource(mapObjectModel()->index(o)));
scrollTo(mObjectsFilterModel->mapFromSource(mProxyModel->mapFromSource(mapObjectModel()->index(o))));
}
}

Expand All @@ -400,7 +455,7 @@ void ObjectsView::setColumnVisibility(bool visible)

QSettings *settings = Preferences::instance()->settings();
QVariantList visibleSections;
for (int i = 0; i < mProxyModel->columnCount(); i++) {
for (int i = 0; i < mObjectsFilterModel->columnCount(); i++) {
if (!header()->isSectionHidden(i))
visibleSections.append(i);
}
Expand All @@ -411,7 +466,7 @@ void ObjectsView::showCustomMenu(const QPoint &point)
{
Q_UNUSED(point)
QMenu contextMenu(this);
QAbstractItemModel *model = mProxyModel->sourceModel();
QAbstractItemModel *model = mObjectsFilterModel->sourceModel();
for (int i = 0; i < model->columnCount(); i++) {
if (i == MapObjectModel::Name)
continue;
Expand All @@ -430,7 +485,7 @@ void ObjectsView::restoreVisibleSections()
QSettings *settings = Preferences::instance()->settings();
QVariantList visibleSections = settings->value(QLatin1String(VISIBLE_SECTIONS_KEY),
QVariantList() << MapObjectModel::Name << MapObjectModel::Type).toList();
for (int i = 0; i < mProxyModel->columnCount(); i++) {
for (int i = 0; i < mObjectsFilterModel->columnCount(); i++) {
header()->setSectionHidden(i, !visibleSections.contains(i));
}
}
Expand All @@ -443,7 +498,7 @@ void ObjectsView::synchronizeSelectedItems()
QItemSelection itemSelection;

for (MapObject *o : mMapDocument->selectedObjects()) {
QModelIndex index = mProxyModel->mapFromSource(mapObjectModel()->index(o));
QModelIndex index = mObjectsFilterModel->mapFromSource(mProxyModel->mapFromSource(mapObjectModel()->index(o)));
itemSelection.select(index, index);
}

Expand All @@ -454,3 +509,44 @@ void ObjectsView::synchronizeSelectedItems()
QItemSelectionModel::Clear);
mSynching = false;
}

ObjectsFilterModel::ObjectsFilterModel(QObject *parent)
: QSortFilterProxyModel(parent)
{
}

bool ObjectsFilterModel::filterAcceptsRow(int sourceRow, const QModelIndex &sourceParent) const
{
// Show all if filter string is empty. Prevents hiding empty groups
if(filterRegExp().isEmpty())
return true;

QModelIndex index = sourceModel()->index(sourceRow, 0, sourceParent);

// sourceParent of a group has no model
if(!sourceParent.model())
Copy link
Member

Choose a reason for hiding this comment

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

This is a somewhat interesting and not entirely correct check. There is no model, because the parent index is invalid, so a more sensible check may be if (!sourceParent.isValid()). However, either check won't work since layers now form a hierarchy and an object layer within a group layer will have a valid parent, which would lead those object layers to be filtered as if they were an object.

Of course, group layers are a problem of their own as well, since whether they should be filtered or not would depend on whether any of their child layers are object groups that contain any non-filtered objects.

Fortunately, we know we're dealing with a MapObjectModel and I would suggest working through its interface directly instead of requesting data through Qt::DisplayRole. That way, you can use toGroupLayer, toObjectGroup and toMapObject. And to avoid having to convert between ReversingProxyModel indexes here, I would change the order in which the two proxies are applied so that the filtering happens before the reversing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree on working directly with the objects, I think it will be better(using toMapObject), I will try doing that and will refactor the proxies code.

return groupHasAnyMatchingObjects(index);
else
return objectContainsFilterString(index);
}

bool ObjectsFilterModel::groupHasAnyMatchingObjects(const QModelIndex index) const
{
for (int i = 0; i < sourceModel()->rowCount(index); ++i) {
Copy link
Member

Choose a reason for hiding this comment

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

In my previous comment I suggested to replace this code, but just a note here: since sourceModel()->rowCount(index) is relatively slow the result should really be saved in a local variable for use in the loop.

QModelIndex childIndex = sourceModel()->index(i, 0, index);
if (childIndex.isValid() && objectContainsFilterString(childIndex))
Copy link
Member

Choose a reason for hiding this comment

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

The childIndex.isValid() should not be necessary since you're only requesting data from valid indexes.

return true;
}
return false;
}

bool ObjectsFilterModel::objectContainsFilterString(const QModelIndex index) const
{
for (int i = 0; i < sourceModel()->columnCount(index); ++i) {
QModelIndex objectIndex = sourceModel()->index(index.row(), i, index.parent());
QString type = sourceModel()->data(objectIndex, Qt::DisplayRole).toString();
if (type.contains(filterRegExp()))
Copy link
Member

Choose a reason for hiding this comment

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

"type" is a confusing variable name here.

return true;
}
return false;
}
44 changes: 41 additions & 3 deletions src/tiled/objectsdock.h
View file
Open in desktop
Expand Up @@ -22,9 +22,9 @@

#include <QDockWidget>
#include <QTreeView>
#include <QSortFilterProxyModel>

class QAbstractProxyModel;
class QTreeView;

namespace Tiled {

Expand All @@ -48,6 +48,7 @@ class ObjectsDock : public QDockWidget

protected:
void changeEvent(QEvent *e) override;
void keyPressEvent(QKeyEvent *event) override;

private slots:
void updateActions();
Expand All @@ -57,23 +58,32 @@ private slots:
void documentAboutToClose(Document *document);
void moveObjectsUp();
void moveObjectsDown();
void filterObjects();

private:
void retranslateUi();

void saveExpandedGroups();
void restoreExpandedGroups();
void saveExpandedGroups(QMap<MapDocument*, QList<ObjectGroup*> > &expansionState);
void restoreExpandedGroups(QMap<MapDocument*, QList<ObjectGroup*> > &expansionState);

QAction *mActionNewLayer;
QAction *mActionObjectProperties;
QAction *mActionMoveToGroup;
QAction *mActionMoveUp;
QAction *mActionMoveDown;

QLineEdit *mFilterEdit;
ObjectsView *mObjectsView;
MapDocument *mMapDocument;

// These maps store the state of the objects dock for each opened map document
QMap<MapDocument*, QList<ObjectGroup*> > mExpandedGroups;
QMap<MapDocument*, QList<ObjectGroup*> > mPrefilterExpandedGroups;
QMap<MapDocument*, QString> mFilterStrings;
QMap<MapDocument*, bool> mFilterWasEmpty;

QMenu *mMoveToMenu;
QModelIndex getGroupIndex(ObjectGroup *og);
Copy link
Member

Choose a reason for hiding this comment

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

Would be called groupIndex according to naming style, but I think it should instead be a convenience override to the previously suggested mapToViewModel helper function. Should be marked const.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you mean by convenience override.

But here is what I did so far, I moved the getGroupIndex function from the objects dock to the objects view (it uses mapToViewModel for conversion), then made it private and made public helper functions in the objects view(isGroupExpanded, setGroupExpanded), so that the objectsDock can use them without worrying about the conversion.

};

class ObjectsView : public QTreeView
Expand All @@ -88,6 +98,8 @@ class ObjectsView : public QTreeView
void setMapDocument(MapDocument *mapDoc);

MapObjectModel *mapObjectModel() const;
QSortFilterProxyModel *objectsFilterModel() const;
QAbstractProxyModel *proxyModel() const;

protected:
void selectionChanged(const QItemSelection &selected,
Expand All @@ -107,9 +119,35 @@ private slots:
void synchronizeSelectedItems();

MapDocument *mMapDocument;
QSortFilterProxyModel *mObjectsFilterModel;
QAbstractProxyModel *mProxyModel;
bool mSynching;
};

inline QSortFilterProxyModel *ObjectsView::objectsFilterModel() const
{
return mObjectsFilterModel;
}

inline QAbstractProxyModel *ObjectsView::proxyModel() const
{
return mProxyModel;
}

class ObjectsFilterModel : public QSortFilterProxyModel
{
Q_OBJECT

public:
ObjectsFilterModel(QObject *parent = 0);
Copy link
Member

Choose a reason for hiding this comment

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

Please use nullptr instead of 0.


protected:
bool filterAcceptsRow(int sourceRow, const QModelIndex &sourceParent) const override;

private :
bool groupHasAnyMatchingObjects(const QModelIndex index) const;
bool objectContainsFilterString(const QModelIndex index) const;
Copy link
Member

Choose a reason for hiding this comment

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

Arguments are missing a &.

};

} // namespace Internal
} // namespace Tiled