Julien - could you have a look at this code? It's from the Slicer RT project, but could be useful for many applications.

Screenshot of the interface:

https://www.assembla.com/spaces/slicerrt/documents/dKjtn6Ijyr4BT-acwqjQWU/download/dKjtn6Ijyr4BT-acwqjQWU

Commit diff:

https://github.com/cpinter/Slicer/commit/9edb32c18ac9a71462a8ac745bd92a575b5ab689

Looks very nice !
It's a great addition.
I commented on your github commit diff.

2 important points summarized here:

1)

a) A lot of logic is added to the Data module.
b) The Attribute table widget can be reused somewhere else.
-> I would extract all the code from data module and creat ea qMRMLNodeAttributeTableWidget instead.
With the following API:
class qMRMLNodeAttributeTableWidget
: public qMRMLWidget // takes care of the setMRMLScene() for you
{
qMRMLNodeAttributeTableWidget::setMRMLNode(...)
... all the methods you already have...
}

My preference (not 100% mandatory):
Ideally I could also see a qMRMLNodeAttributeTableView that is just the table and all utility functions. And qMRMLNodeAttributeTableWidget that is the table and some buttons such as add attribute, remove attribute...
This is what is done for the Annotation table if I'm not mistaken.

2) I would make the use of QTimer optional.
Using a timer has the drawback of having the state of the widget being inconsistent for a slight moment which can be a problem. Moreover the timing of the repopulating can fall at a bad moment (maybe the node deleted already ? ...)
Populating the tree widget is a quick action. I wouldn't bother with a Timer. The only optimization I can see is to block updates as long as the widget is not visible to the user and refresh it when it gets visible.

I made the changes you suggested in these commits:
https://github.com/cpinter/Slicer/commit/090b4b9ad3ce00ab9bfbc5143641ad2301f13c53
https://github.com/cpinter/Slicer/commit/af8703aaa0c4de8359093da3772a88432270897f

It seems to work fine, although I don't really like the automatic column resize setting of the table. It resizes itself to the actually visible content, and when I scroll to a view that contains longer attribute values, it remains small. Maybe we can come up with something else.

Please take a look at the code and let me know if anything needs to be changed.

Looks good (haven't compiled nor run it though). Please see my few comments.
Consider also adding unit test for the 2 Qt widgets. See qMRMLCheckableNodeComboBoxTest.cxx for an example.
After that, feel free to push.
Thanks for your contribution !

I made the changes you requested, thanks for reviewing the code!
https://github.com/cpinter/Slicer/commit/d27dc435c121aa327582e62f940f81ecb3b3b1d2

I added the tests for the widgets:
https://github.com/cpinter/Slicer/commit/e4e5a867aeea103d4a93f57a7450467aaf1551da
I have questions though. How should I check the results without exposing some functions only for the test? See commented rows in the test cxx's.

I'm going on holiday tomorrow, so I might react slowly.
Thanks!

Writting unit test is a good technique to ensure your API is good and usable.
Here, it probably means you need to expose more functions in your API.

I finished the tests (https://github.com/cpinter/Slicer/commit/b52c52a21c6b274b10eb174c937f12612d2d9b1b).
What would be the best way to integrate it?

I wrote some minor comments.
To integrate, if you have svn access to the repo, it's
git svn dcommit
Otherwise you need someone with rights to commit for you.
Unfortunately, svn doesn't make a difference between committer (the one who integrates into svn) and author (the one who wrote the code).

• So you either get svn write access, but we don't give them to sporadic commiters anymore (we limit write access).
• or you ask someone with write access to integrate your work (git pull request on its Slicer github fork). That person will get the credits so he/she should mention you in the commit message.
  I would suggest to have Andras to integrate your work. I can also do it if you prefer.
  However, because all those commits are semantically only 1, maybe you could squash them in 1 commit to commit.

I addressed most of what you wrote:
https://github.com/cpinter/Slicer/commit/f0d2ef77bf86da31350e38be3d0deed7761827d3
If you agree with what I wrote on line 350 9https://github.com/cpinter/Slicer/commit/b52c52a21c6b274b10eb174c937f12612d2d9b1b#commitcomment-1316698), then I start integrating.

Sounds good.
Before integrating, please squash all your commits in 1 commit.

I commited my changes in the SVN repo as revision 20089.
Thank you for your kind help!