Refactor dynamic hierarchy in SessionItems (less generic, more explicit)
Goal is to simplify SessionItem
/SessionModel
structure and then remove dependendy from SessionItem
/SessionModel
.
Dynamic hierarchy with the disadvantage of unclear behaviour change (or even type change) should be removed.
Example:
Right now, a RealDataItem
(which stores 1D or 2D data from real experiments) can change from 1D to 2D just be exchanging a child item. This leads e.g. to strange behavior when loading files, and it needs a lot of type checking. Loading of files is not arbitrary, but the user defines whether he wants to load 1D or 2D. Therefore it would be possible to have a dedicated class RealDataItem1D
and a second class RealDataItem2D
. With this, the dynamic hierarchy change is obsolete, methods using objects of RealDataItems do not have to check for strings to find out the type of the actual object, and so on. Instead, the normal C++ mechanisms for polymorphism can be used to handle the data.
Also refactorings like in the following (simplified) example would be possible, which makes it much easier to understand a class:
Now:
class CylinderItem: public SessionItem
{
}
CylinderItem::CylinderItem()
{
Cylinder.addProperty(HEIGHT)
}
Refactored:
class CylinderItem
{
double m_height;
}
This has to be done for
-
Material items -
Sample items -
Instrument items -
Job items -
Real data items
Hints for refactoring
SessionItem
based signaling
Step 1 - Replace First all SessionItem
based signaling of the item in question has to be replaced by other means. For this, please see #88 (closed). That is necessary, because by removing the base class SessionItem
, the related signaling is not available any more.
SessionItem
exposure
Step 2 - reduce Example commit: 2ddf9cf5
After this, it is a good idea to reduce SessionItem
exposure on the item's API. E.g. instead of returning an item name by SessionItem::itemName()
, add a dedicated method for this and then forward to the itemName(). By this, the migration can be done in a more controllable manner, because after this step, the code still compiles and runs. In a later step, the internals of course will be removed and redirected to own members.
Locating the relevant spots can be achieved e.g. by private deriving from SessionItem (instead of public) and checking the compiler errors.
Step 3 - Use descriptors on an item's API
Example commit: 377b3dcc
Descriptors (e.g. DoubleDescriptor
) provide access to a value and a few properties of this value. For example the form factor item BoxItem
has a length, and this length has limits (>0), a unit (mm), a label (length) and a few more. Right now, with SessionItem
in place, this is created and stored by a child element of type SessionItem, which has itself properties to keep the data. This is done by SessionItem::addProperty()
.
By using descriptors for accessing such a member, the internals of the refactored item stay the same, but the outer world does not have to rely on the implementation of such a property. Therefore after this step, the code is again compilable and working correctly.
Why descriptors? Because they are an abstraction to the actual implementation of a property. By this, the migration can be done without breaking the code. A descriptor can work on a property added with SessionItem::addProperty()
and can also work on any arbitrary implementation using a member like double m_length
. Later in this migration we will use DoubleProperty m_length
, which directly supports the descriptors and is not dependend on any hierarchy or similar.
Example for using descriptors on an item's API
Code before refactoring
class EllipseItem : public MaskItem {
static constexpr auto P_ANGLE{"Angle"};
public:
// ...
double angle() const { return getItemValue(P_ANGLE).toDouble(); }
Code after refactoring
class EllipseItem : public MaskItem {
static constexpr auto P_ANGLE{"Angle"};
public:
// ...
DoubleDescriptor angle() const { return DoubleDescriptor(getItem(P_ANGLE), Unit::degree); }
SessionItem
as base class
Step 4 - Remove Example commit: 6ffbaaee
This step is usually a big refactoring and has to be finished for all items of a model, before the code is compilable and working again.
By removing this base class, SessionItem::addProperty()
is gone, serialization is gone, item creation is gone. Therefore the item creation, the properties and the serialization has to be re-implemented. Since also the hierarchical structure is gone, there may be places where a containing element has to be passed e.g. in a child item's constructor instead of being retrieved by crawling through the former SessionItem
hierarchy.
Step 4.1 - Replace trivial properties
Example commit: 8f4d375d
Properties very often can be replaced by members of type DoubleProperty
, UIntProperty
and SelectionProperty
. Please refer also to the documentation of these classes. The benefit when using these classes is that they directly support the descriptors we introduced in step 3 and are easy to serialize.
For the initialization of the newly introduced members, you need a title. This has to be taken from the related P_XXX
- it defined the title until now. Other initialization has to be taken from the replaced call to addProperty()
. More information about the initial values can be found in the addProperty()
implementation (e.g. which are the defaults for limits, in case they should be set explicitly).
Example for using DoubleProperty instead of SessionItem-property
Code before refactoring
class EllipseItem : public MaskItem {
static constexpr auto P_ANGLE{"Angle"};
public:
// ...
EllipseItem()
{
addProperty(P_ANGLE, 0.0)->setLimits(RealLimits::limitless());
}
double angle() const { return getItemValue(P_ANGLE).toDouble(); }
Code after refactoring (note the additional serialization!)
class EllipseItem : public MaskItem {
public:
EllipseItem()
{
m_angle.init("Angle", "", 0.0, Unit::degree, RealLimits::limitless(), "angle");
}
DoubleDescriptor angle() const { return m_angle; }
double setAngle(double d) const { m_angle.set(d); }
void serialize(Serializer& s)
{
// ...
s.rw(m_angle);
}
private:
DoubleProperty m_angle;
The last one can be simplified by using the macro DOUBLE_PROPERTY, which defines getter,setter and member:
class EllipseItem : public MaskItem {
public:
EllipseItem()
{
m_angle.init("Angle", "", 0.0, Unit::degree, RealLimits::limitless(), "angle");
}
DOUBLE_DESCRIPTOR(angle,Angle)
void serialize(Serializer& s)
{
// ...
s.rw(m_angle);
}
Step 4.2 Replace parent-child hierarchies of non-trivial items
Example commit: 78cf2eba
In SessionModel
, all elements have a parent-child relationship. In the above step, we have changed this for properties. Now they are not children any more, but just normal members. We have done this for trivial items which keep double, int or similar. For non-trivial items like the BeamItem in an InstrumentItem this is different.
If the item just has one child of a certain type, think in normal C++: make it a member, or make it a member-ptr. The decision depends on the creation/usage.
Examples:
class InstrumentItem {
// ....
std::unique_ptr<BeamItem> m_beam;
};
class JobItem {
// ....
MultiLayerItem m_multiLayerItem;
};
There are child elements which can be one out of a selection. For example the resolution function of a DetectorItem can be of type ResolutionFunctionNoneItem
or ResolutionFunction2DGaussianItem
. In this case, you could just add a ptr to their common base class ResolutionFunctionItem
and create according to the needs. But for such a selection, there exists a ready-to-use property, which handles also the related descriptor, the creation and the serialization of the element.
Example:
class DetectorItem {
// ...
DetectorItem()
{
m_resolutionFunction.initWithInitializer<ResolutionFunctionItemCatalog>(
"Resolution function", "", "resolutionFunction");
}
SelectionDescriptor<ResolutionFunctionItem*> resolutionFunctionSelection() const
{
return m_resolutionFunction;
}
void serialize(Serializer& s)
{
// ...
s.rw<ResolutionFunctionItemCatalog>(m_resolutionFunction);
}
}
private:
SelectionProperty<ResolutionFunctionItem*> m_resolutionFunction;
};
For further information, please refer to the SelectionProperty
documentation.
Step 4.3 - cleanup inside class
Remove all the P_XXX
constants and the M_TYPE
. Some functions and includes may have become obsolet. Check for this.
SessionModel
Step 5 - Remove Example commit: 3c172a1d
Now none of the SessionModel
functionality is necessary any more. Remove it as a base class, rename the base class so it does not give a wrong idea of what it is, and use the class as a member directly in ProjectDocument
.
Example:
InstrumentModel
-> IntrumentItems
class InstrumentItems { // NOT derived from SessionModel
public:
~InstrumentItems();
template <typename T> T* addInstrument()
void removeInstrument(InstrumentItem* instrument);
void clear();
void serialize(Serializer& s);
InstrumentItem* findInstrumentById(const QString& instrumentId) const;
bool instrumentExists(const QString& instrumentId) const;
QString suggestInstrumentName(const QString& baseName) const;
QStringList instrumentNames() const;
private:
QVector<InstrumentItem*> m_instruments;
};
Note that this class is very small, not much more than a list with additional accessors. Now put this class as a member in ProjectDocument
, remove it from ApplicationModels
and rectify the getter:
class ProjectDocument : public QObject {
// ...
InstrumentItems& instrumentItems()
{
return m_instrumentItems;
}
private:
InstrumentItems m_instrumentItems;
};