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 task filter to NameServiceItem #64

Conversation

g-arjones
Copy link
Contributor

This adds support for a task filter in the NameService item

@doudou
Copy link
Member

doudou commented Oct 18, 2017

Could you tell us more about what you're planning to do with this ?

In principle, the filtering could already be done post-model, using standard Qt mechanisms. Why filtering in the name service ?

@g-arjones
Copy link
Contributor Author

g-arjones commented Oct 18, 2017

I am working on something like the rock-display with a text field on top of it to filter tasks by name. Filtering in the nameservice saves memory/cpu and improves reusability. Since the item adds stuff to the model by itself, I thought it made sence to filter here...

@doudou
Copy link
Member

doudou commented Oct 19, 2017

I am working on something like the rock-display with a text field on top of it

You mean ... you're extending the rock-display to add a text field on top of it ? ;-)

In theory (I haven't developed this code), filtering at the name service level should save very little. The implementation of the model will only create port and property listeners if the items are opened. Moreover, as a user, if I have a port opened, filter to find another tasks, and then clear the filter, I expect whatever I had before filtering to stay as it was. Which is what the QSortFilterProxy does.

If you want to save memory and CPU in the task inspector, I can suggest a few things that would have a lot more impact ...

@doudou
Copy link
Member

doudou commented Oct 19, 2017

You mean ... you're extending the rock-display to add a text field on top of it ? ;-)

I forgot to add: what a great idea.

@g-arjones
Copy link
Contributor Author

You mean ... you're extending the rock-display to add a text field on top of it ? ;-)

Yes.. of course! :)

Which is what the QSortFilterProxy does.

That was my first choice but there is code in gui-vizkit that expects the VizkitTreeView model to inherit from QStandardItemModel (it calls methods that are not available in QSortFilterProxyModel). I will have a deeper look to see how much work it is to fix this.

If you want to save memory and CPU in the task inspector, I can suggest a few things that would have a lot more impact ...

Please, do!

@doudou
Copy link
Member

doudou commented Oct 19, 2017

That was my first choice but there is code in gui-vizkit that expects the VizkitTreeView model to inherit from QStandardItemModel (it calls methods that are not available in QSortFilterProxyModel). I will have a deeper look to see how much work it is to fix this.

The main change one usually has to do when using the soft filter proxy model is to map indexes from the filter to the real model with map_from_source/map_to_source.

Please, do!

I've just registered two my pet ideas about this on issues:

rock-core/tools-orocosrb#117

#66

The first one is IMO the lowest-cost/highest-reward. For somebody with your skills, it should be a small-ish time investment, and I believe it should really have a meaningful impact (not measured, so as always might be wrong).

In addition, if what you are targetting is in the end the display within the IDE, a big chunk of the memory/CPU consumption is the chronicle display in the middle. It's basically useless, so I wanted to replace it with something that provides more useful information. It's quite a lot more work though.

Alternatively, I think that we could reduce its CPU usage significantly by replacing it with a QML implementation backed by a Qt model. This would in addition provide us with a migration path to Qt5 - QML is the way to go to interface Qt5 and Ruby (the two things that will be hard to migrate are the task inspector and the Roby displays)

@g-arjones
Copy link
Contributor Author

The main change one usually has to do when using the soft filter proxy model is to map indexes from the filter to the real model with map_from_source/map_to_source

Exactly, I am assessing every call to itemFromIndex() and item() that VizkitTreeView does and replacing it with mapTo/FromSource if the model is a QAbstractProxyModel. The worst part is that there are no unit tests for this stuff so I may overlook something (I am counting on you to review this :)).

The first one is IMO the lowest-cost/highest-reward. For somebody with your skills, it should be a small-ish time investment, and I believe it should really have a meaningful impact (not measured, so as always might be wrong).

I will have a look on the first one when I am done tweaking the task inspector and VizkitTreeView. Regarding the migration to a QML implementation, I think that would be great even if we got zero performance improvements. It would increase flexibility a lot and much of the interface work could be better handled by UX specialists. I am afraid that's too much work for one person though but I can help if you or anyone else decides to lead the way..

@g-arjones
Copy link
Contributor Author

Follow-up: #1

@g-arjones g-arjones closed this Oct 19, 2017
@g-arjones g-arjones deleted the add_task_filter branch October 19, 2017 17:35
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.

2 participants