Agreed. This is rather important especially considering extension development is used more and more. Having a simple and well tested API become more and more critical.

@Andras: When you have a chance, check that it works for you and close the issue. Thanks.

For reference: http://slicer-devel.65872.n3.nabble.com/mrml-references-redesign-tp4027889p4028039.html

The current version (file revision 21810) of vtkMRMLTransformableNode.h contains member variables with unprecedented type in VTK classes, such as

static const std::string TRANSFORM_NODE_REFERENCE_ROLE;
static const std::string TRANSFORM_NODE_REFERENCE_MRML_ATTRIBUTE_NAME;
and
std::string TransformNodeID;

This change caused some build errors in the SlicerRT extension, but more importantly it does not comply with the VTK convention, according to which these should be char* variables using vtkGet/Set macros.

There is an issue with vtkMRMLNodeReference: it has public members variables, moreover these are non-wrappable (STL string).

EDIT (jc): VTK methods dealing with std::string can now be wrapped in VTK. See http://www.vtk.org/Wiki/VTK/Wrapper_Update_2010#Wrapping_vtkStdString

class VTK_MRML_EXPORT vtkMRMLNodeReference : public vtkObject
{
...
public:
std::string ReferenceRole;
std::string ReferencedNodeID;
...
};

To be conform with other parts of the Slicer core the obvious solution is to:

• change std::string to char* (and update constructors, destructor, etc. accordingly)
• move all private members and provide access through vtk Get/Set string macros

Another important modification needed:

Currently vtkMRMLNodeReference has a vtkObject parent, but vtkMRMLNodeReference also has a public constructor, destructor, copy constructor, and operator=.

Two possible solutions:

Option A: The vtkMRMLNodeReference class is converted to a simple struct that is only used privately inside vtkMRMLNode class.

Option B: The vtkMRMLNodeReference class is used publicly, but then it has to be converted to a proper VTK class (no public constructor, destructor, fully wrappable public interface, etc.).

converted vtkMRMLNodeReference class into a proper VTK class. Converted std::string's into char *. vtkMRMLNodeReference is now protected class inside vtkMRMLNode class.

http://viewvc.na-mic.org/viewvc.cgi/Slicer4?view=revision&revision=21850

Right now, the reference role and reference mrml attribute name definitions seem a bit too verbose especially because there is no need to change the strings:
.h
char TransformNodeReferenceRole;
char TransformNodeReferenceMRMLAttributeName;

vtkSetStringMacro(TransformNodeReferenceRole);
vtkGetStringMacro(TransformNodeReferenceRole);

vtkSetStringMacro(TransformNodeReferenceMRMLAttributeName);
vtkGetStringMacro(TransformNodeReferenceMRMLAttributeName);

.cxx
in constructor:
this->TransformNodeReferenceRole = 0;
this->TransformNodeReferenceMRMLAttributeName = 0;

this->SetTransformNodeReferenceRole("transform");
this->SetTransformNodeReferenceMRMLAttributeName("transformNodeRef");

this->AddNodeReferenceRole(this->GetTransformNodeReferenceRole(),
this->GetTransformNodeReferenceMRMLAttributeName());

I suggest the following instead:
.h
static char TransformNodeReferenceRole[];
static char TransformNodeReferenceMRMLAttributeName[];

vtkGetStringMacro(TransformNodeReferenceRole);
vtkGetStringMacro(TransformNodeReferenceMRMLAttributeName);

.cxx
char vtkMRMLSelectionNode::TransformNodeReferenceRole[] = "transform";
char vtkMRMLSelectionNode::TransformNodeReferenceMRMLAttributeName[] = "transformNodeRef";

in constructor:
this->AddNodeReferenceRole(this->GetTransformNodeReferenceRole(),
this->GetTransformNodeReferenceMRMLAttributeName());

Do you all approve that change ?

Make sense. That would simplify the API.

I agree. Would be somewhat nicer with const.

If the role or attribute name does not have to be accessed from other classes (it shouldn't), then I would simply do:

.h
(nothing)

.cxx

static const char TRANSFORM_NODE_REFERENCE_ROLE[] = "transform";
static const char TRANSFORM_NODE_REFERENCE_MRML_ATTRIBUTE_NAME[] = "transformNodeRef";

in constructor:
this->AddNodeReferenceRole(TRANSFORM_NODE_REFERENCE_ROLE,
TRANSFORM_NODE_REFERENCE_MRML_ATTRIBUTE_NAME);

Const is not possible when using vtkGetStringMacro(), it expects a char not const char :-(

Concerning the access from other classes, it should actually be possible to access those strings from subclasses I believe.

OK, keep it as a member variable then.

The non-const vtkGetStringMacro is unfortunate. We could add a simple get function with a const return value, but it would require a few extra lines of code, so not a perfect solution either.

Closing resolved issues that have not been updated in more than 3 months