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.

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

Add objects filter to the objects dock #1566

Closed
wants to merge 30 commits into from

Conversation

@thabetx
Copy link
Contributor

commented May 10, 2017

Fixes #1467 .

filter06

The filter text box supports two shortcuts

  • Escape clears the filter.
  • Return focuses the first filtered item, pressing return while an object is focused still changes the focus to the properties dock.

Also each map has its own search string so switching between maps will change the filter.

All columns are matched (name, type, id, position) even if they are hidden. Group names are not matched.

Not entirely sure about the above decisions so would like some feedback regarding them.

Some stuff that are still missing

  • Expanded groups that didn't match will collapse after clearing the search, I'm still looking into it.
  • Matching custom properties.
@thabetx

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2017

Now, any group having a matching object will be expanded while filtering, so the above the GIF is not very accurate.

I also updated the behavior of the return key to just focus the tree instead of focusing the first item.

I will fix expanding pre-filter expanded groups that didn't match.

@bjorn bjorn self-requested a review May 15, 2017

@bjorn
Copy link
Owner

left a comment

Hey @thabetx, I've tried it out and I really like it! It's nice that you remember the search string for each map as well as automatically expanding after filtering and restoring the user's expanded groups afterwards. I also like the handling of Return and Escape keys.

On the code side I had some requests, let me know how you feel about them!

src/tiled/objectsdock.cpp Outdated Show resolved Hide resolved

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

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

This comment has been minimized.

Copy link
@bjorn

bjorn May 15, 2017

Owner

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

Typo: Resotre

{
// Save the prefilter expansion state if this is the first applied filter
if(mFilterWasEmpty[mMapDocument]) {
mFilterWasEmpty[mMapDocument] = false;

This comment has been minimized.

Copy link
@bjorn

bjorn May 15, 2017

Owner

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

This comment has been minimized.

Copy link
@thabetx

thabetx May 16, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@bjorn

bjorn May 16, 2017

Owner

Ah, makes sense, thanks for the explanation!

@@ -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);

This comment has been minimized.

Copy link
@bjorn

bjorn May 15, 2017

Owner

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.

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

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

This comment has been minimized.

Copy link
@bjorn

bjorn May 15, 2017

Owner

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.

This comment has been minimized.

Copy link
@thabetx

thabetx May 17, 2017

Author Contributor

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.

{
for (int i = 0; i < sourceModel()->rowCount(index); ++i) {
QModelIndex childIndex = sourceModel()->index(i, 0, index);
if (childIndex.isValid() && objectContainsFilterString(childIndex))

This comment has been minimized.

Copy link
@bjorn

bjorn May 15, 2017

Owner

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

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()))

This comment has been minimized.

Copy link
@bjorn

bjorn May 15, 2017

Owner

"type" is a confusing variable name here.

QMenu *mMoveToMenu;
QModelIndex getGroupIndex(ObjectGroup *og);

This comment has been minimized.

Copy link
@bjorn

bjorn May 15, 2017

Owner

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.

This comment has been minimized.

Copy link
@thabetx

thabetx May 18, 2017

Author Contributor

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.

Q_OBJECT

public:
ObjectsFilterModel(QObject *parent = 0);

This comment has been minimized.

Copy link
@bjorn

bjorn May 15, 2017

Owner

Please use nullptr instead of 0.


private :
bool groupHasAnyMatchingObjects(const QModelIndex index) const;
bool objectContainsFilterString(const QModelIndex index) const;

This comment has been minimized.

Copy link
@bjorn

bjorn May 15, 2017

Owner

Arguments are missing a &.

@thabetx

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

Alright, thank you for the review, I will be updating the pull request.

I've a question though, suppose we have an expanded group, then we applied a filter and one of the objects of this group matched the search, then we collapsed this group, when cancelling the search it is restored to the initial state(expanded) although we just collapsed it, is this behavior expected?

I saw one application (gitkraken) that prevented changing the expansion state of any group while a filter is applied, this will solve this case but is kind of restrictive, what do you think?

@bjorn

This comment has been minimized.

Copy link
Owner

commented May 15, 2017

I've a question though, suppose we have an expanded group, then we applied a filter and one of the objects of this group matched the search, then we collapsed this group, when cancelling the search it is restored to the initial state(expanded) although we just collapsed it, is this behavior expected?

I guess it could be helpful to keep it collapsed given the user's disinterest in this group. So instead of collapsing everything and restoring from mPrefilterExpandedGroups, you could instead just collapse any group that isn't in it that list.

I saw one application (gitkraken) that prevented changing the expansion state of any group while a filter is applied, this will solve this case but is kind of restrictive, what do you think?

I agree that this is too restrictive.

Btw, it occurred to me that for expanding the tree when the search filter is changed you should just call mObjectsView->expandAll(). The current method of expanding all object groups won't work in case of group layers.

@bjorn

This comment has been minimized.

Copy link
Owner

commented May 15, 2017

Actually on second thought, it occurred to me that allowing the user to collapse the tree is not very useful when on any typed character we will automatically expand the whole thing again. So maybe disabling itemsExpandable on the view while a filter is active isn't a bad idea after all...

@thabetx

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

Ok I will test disabling collapsing. This might also simplify the code as mExpandedGroups and mPrefilterExpandedGroups can be merged into one in that case again.

@thabetx

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2017

Hey @bjorn, thank you for the great review, I've addressed the requests, the small notes were also helpful.

Filtering now works directly on the objects, this made the filtering code simpler and I think it will make filtering by custom properties possible. Right now the filter works on name, type and ID, I don't check the position, I think it's not useful but not sure.

I would like to receive some usability reviews, I think the behavior of the filtering can be improved.

@thabetx

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2017

Things don't work properly when moving objects up and down, will investigate it.

@thabetx

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2017

For some reason reversing the proxy models stopped deleting the objects or moving them up an down from working, I tried restoring it to the previous order and the functions worked fine. This will require adding the missing translation step when filtering, although it works without it.

I thought of introducing two extra QAbsractProxyModel pointers that point to the two proxies, called mFrontEndProxyModel and mBackEndProxyModel, I think this makes the code more readable and changing the order of the proxies is just a matter of changing what those pointers point to, I'm not sure if this is a good practice though.

Another thing to notice when moving objects up and down is that the order of the objects is the same as when the objects are not filtered, so moving-up an object while filtering might not visually move it up in the view because there might be objects above that are hidden, I think this is expected from the code, but don't know what to do about it.

@bjorn

This comment has been minimized.

Copy link
Owner

commented May 20, 2017

I would like to receive some usability reviews, I think the behavior of the filtering can be improved.

Can you elaborate a bit about this?

Things don't work properly when moving objects up and down, will investigate it.

I'll have a look at this as well. Sounds like an annoying thing to debug... Of course the order of the proxy models shouldn't matter at all. :-/

I thought of introducing two extra QAbsractProxyModel pointers that point to the two proxies, called mFrontEndProxyModel and mBackEndProxyModel, I think this makes the code more readable and changing the order of the proxies is just a matter of changing what those pointers point to, I'm not sure if this is a good practice though.

I prefer the more explicit names.

Another thing to notice when moving objects up and down is that the order of the objects is the same as when the objects are not filtered, so moving-up an object while filtering might not visually move it up in the view because there might be objects above that are hidden, I think this is expected from the code, but don't know what to do about it.

I think it is reasonable to not expect those actions to adjust to any active search filter.

@thabetx

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2017

About the improvements, I thought it'd be useful to receive feedback from the community, I don't have much in mind other than keeping groups with selected objects expanded when cancelling the search, adding a shortcut to focus the search bar and currently when filtering, collapsing is disabled, but there is no indication for that, so needed some opinions about it.

Fixed the logic in ReversingProxyModel::sourceRowsAboutToBeInserted
The logic was wrong when more than one row was getting inserted.
@bjorn

This comment has been minimized.

Copy link
Owner

commented Jul 17, 2019

Ohoh, those conflicts in objectsdock.cpp are looking quite severe...

@bjorn bjorn closed this in 7fd47c5 Jul 17, 2019

@bjorn

This comment has been minimized.

Copy link
Owner

commented Jul 17, 2019

After fixing the issue with the ReversingProxyModel by basing it on the QSortFilterProxyModel in change e248604, I decided to reimplement this change instead of trying to resolve the conflicts.

My patch does not remember a search filter for each document, but I'm anyway not sure what is the better behavior and it was easier to leave that functionality out for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.