Testcase for: mitk::DataTreeNodeFactory::SetDefaultCommonProperties() does not respect given properties

Closed, ResolvedPublic

Actions

mitk::DataTreeNodeFactory::SetDefaultCommonProperties(mitk::DataTreeNode::Pointer &node) (line 475):

sets node properties without checking whether these properties are already defined by the reader.

Is there a design policy regarding properties and their (pre-)definition?

Also, there is lots of commented code in mitkDataTreeNodeFactory line 173 pp.

There is no written design policy about this. However I understood the problem after discussion with Markus:

• There is a file reader that generates a BaseData object.
• The reader attaches properties like "color", "name" to the BaseData
• In DataTreeNodeFactory::SetDefaultCommonProperties a "name" is added to the DataTreeNode containing the read BaseData object. "name" is overwritten in this case. Something similar goes for "color"

The PropertyLists on BaseData were meant to store properties that belong to the data and should not change easily, such as "modality" (CT, MR, ..). Properties on DataTreeNode should decorate BaseData object regarding an application context -- properties like "color" are added here. And no, MITK is not at all consistent in this regard yet.

I would suggest the following: the reader adds properties to BaseData describing the data. In this case something like "modality"=="US Doppler"/"US BMode" ... The relevant mappers and/or DataTreeNodeFactory should read out this property and set color/name accordingly.

Regarding setting properties, there are two methods in PropertyList that should behave different:

• ReplaceProperty(key, newprop) adds the given "newprop" object to the list of properties, replacing any previous entry. Especially it uses the given pointer, thus enabling sharing one property object between two nodes
• SetProperty(key, newprop) tries to assign a value and not replace existing objects. The more complicated logic is:
  • if there is no such key, add the given newprop
  • if there already is such a key
    • • compare old and newprop using ==, only continue if they differ
      • check the Assignable(BaseProperty*) method
        • if assigning a value is possible do this by calling operator=
        • if assigning is not possible, refuse further processing unless typeid's of properties are identical
    • as a last resort, if types are identical but operator= is not implemented, replace the pointer

First of all, you (Markus) should check, whether this mechanism is still working as it was meant to be. Then you should discuss with Matthias, if the proposed solution (like "modality" properties) is viable. For actual values of such a "modality" property, I would strongly recommend to use values from the DICOM standard.

After talking to Matthias,
we agreed, that the reader may set all data-related properties.
All rendering-related properties are not to be set (and cannot be set as there is no node yet).

If a name is already set by the reader, the mitkDataTreeNodeFactory uses this one instead of creating a new one using the filename. This is vital when several images are contained in one file that must not have the same name (eg. "color Doppler" and "power Doppler").

In addition to that, the reader can set an "ImageType"-Property (or modality) that defines which kind of data was loaded. A enum of types would be good/necessary.

The imagemapper that sets the DefaultImageProperties could then check this ImageType and set properties specifically.

Further, we'll have to check if more readers set rendering-specific properties, or if there are more ImageTypes that could need specific DefaultProperties (dicom-tags?)

mitkDataTreeNodeFactory now checks whether a name has already been set by the reader (saved in BaseData's PropertyList) and uses that if possible.
If no name has been defined, the filename is used, just as it was before

first step towards porting data-specific properties to the node

Do you please think of adapting the test case, too?

Property discussions have a long history in MITK. See T1332. DataTreeNodeFactory should not set any node properties, this is the job of the mapper classes.

Node properties were only meant to let the application module configure the mappers in an abstract way. All other information that gets stored there points to design flaws in my opinion.

Since Markus is not here for the next two months: Can any of the discussers tell if this is 1.0 release relevant?

I would vote yes. The issue is solved, only a testcase is to be written. Since this relates to core features it should be solved for 1.0

Assigning this bug to me since Markus will not be here before 1.0 release

[SVN revision 21095]
FIX (#2181): added test for DataTreeNodeFactory. Still missing: test for node name property

[SVN revision 21097]
FIX (#2181): added test for node name property in DataTreeNodeFactoryTest