Refactor signaling of value changes (less generic, more explicit)
Goal is to simplify SessionItem
/SessionModel
structure.
If a value is changed (let's say the height of a cylinder), this is notified by the height-containing sub-item, and its name ("height") will be delivered for identification. This can't be followed by any tool (because of the textual definition of the property - same as for read/write access).
This shall be changed to a) less signals and b) named signals.
Example:
Before a recent refactoring (in the context of RealDataItem
), the "name"-property of an instrument signaled its change itself, overhanding its property name, which is "name". To listen to this change, a potential receiver had to step down in the instrument's sub-item hierarchy, find the child property "name", and connect to this signal. Or it can connect to a higher level item, listen on child signals, check whether the child is "name", then react.
All this shows that it is very complicated to ever know who is listening, and whether this listener expects a certain hierarchy and name. By searching for a listener who checks for "name", many false hits will be given, since "name" is used everywhere, not only in instruments.
For the given example, this has been refactored to:
InstrumentModel
{
// ...
signals:
void instrumentNameChanged(const InstrumentItem* instrument);
}
By this,
a) every listener can be localized very easily. The knowledge of any hierarchy or property naming is obsolete. Compilers can track usage.
b) signals are clearly named and understandable
c) the amount of signals is reduced, since not every potential signal is used at all.
This has to be done for
-
Material items -
Sample items -
Instrument items -
Job items -
Real data items
Tips for refactoring
Find all SessionItem based signaling of the item in question. This can be done e.g. by making SessionItem::mapper()
private and check the compiler errors. Or the item can be derived as private from SessionItem
, again check the compiler errors. Or just find where a call to the mapper()
takes place.
After finding the location, find out what shall be achieved by the signal. Then find a way to implement it in a different way. Either by Qt signals or by rewriting the code to eliminate the need for signaling at all.
A small refactoring of this kind you can see in !682 (merged). There are not too many changes, so the modifications are quite visible.
Example 1:
JobItem::JobItem()
{
// ...
mapper()->setOnPropertyChange([this](const QString& name) {
if (SessionItem::isItemNamePropertyName(name))
updateIntensityDataFileName();
});
}
The aim of this code: If the job name changes, the filename of contained intensity data has to be updated.
Replacement: This filename is only used when saving the project document. Therefore it would be sufficient to call
updateIntensityDataFileName();
when saving a project. No signal is necessary at all for this.
Example 2:
JobItem::JobItem()
{
// ...
mapper()->setOnChildPropertyChange([this](SessionItem* item, const QString& name) {
if (item->parent() == this && dynamic_cast<DataItem*>(item)
&& DataItem::isAxesUnitsPropertyName(name))
dynamic_cast<DataItem*>(item)->updateCoords(instrumentItem());
});
}
The aim of this code: React to changes in a DataItem's axis. It is very likely that this kind of change can only be done by some UI. Find the relevant UI, and from there trigger the call to 'updateCoords()'.
Example 3:
m_item->mapper()->setOnItemDestroy([this]() { m_item = nullptr; }, this);
Calls like this check for the lifetime of other elements. Usually this can be eliminated by an object dependency or object ownership redesign.