Separation of Model and Data (QAbstractListModel)

markugra

Hi there,

I saw a presentation about best practices in Qt and one of the tips was to use the QAbstractListModel as an independent interface between the VisualizationLayer (QML) and the DataLayer. To do this the data is included in the model class only by reference, and append/removed signals from the data layer are connected to slots in the interface that emit beginInsertRows()/endInsertRows() or beginRemoveRows/endRemoveRows() signals necessary for the QML View to deal with changes in the model. The relevant parts of the provided example code are:

class AlarmModel : public QAbstractListModel
{
Q_OBJECT
public:
enum Roles { SeverityRole = Qt::UserRole,
DescriptionRole };
AlarmModel(DataModel& data);
QHash<int, QByteArray> roleNames() const;
int rowCount(const QModelIndex& parent = QModelIndex()) const;
QVariant data(const QModelIndex& index, int role) const;
private slots:
void handleAlarmAppened();
void handleAlarmRemoved(int alarm);
private:
DataModel& m_data;
};

AlarmModel::AlarmModel(DataModel& data) : m_data(data)
{
connect(&data, SINGAL(alarmsAppened()),
this, SLOT(handleAlarmAppened()));
connect(&data, SINGAL(alarmsRemoved(int)),
this, SLOT(handleAlarmRemoved(int)));
}

void AlarmModel::handleAlarmAppened()
{
beginInsertRows(QModelIndex(), rowCount(), rowCount());
endInsertRows();
}
void AlarmModel::handleAlarmRemoved(int alarm)
{
beginRemoveRows(QModelIndex(), alarm, alarm);
endRemoveRows();
}

I thought I'll apply this to my application, but noticed that the view did not update properly when appending an item. The reason for that, I believe, is that the data is modified before the handle function and not in between the the begin and end signals.

To keep the layers separated I want to avoid modifying the data directly from the interface layer. Would I need emit two signals from the data layer to trigger the begin and end signals separately?

I am not so sure which approach would be the cleanest/best or maybe I misunderstood the example?

I appreciate any input. Thanks in advance.

Christian Ehrlicher

I don't think that holding a reference on the data is a good approach (I would not do it - it's looks like it's calling for crashes and undefined behavior).
What do you mean with 'modify' data. I just see additions and removes but no modifies. And you should really do the append/remove in between beginFoo() and endFoo(). When the row count changes before beginFoo() it can happen that the view accesses rowCount() and get a wrong result.

dheerendra

when you append the date, only beginInsertRows() and endInsertRows() should be sufficient. If you are modifying the data, dataChanged() signal may help. I did not see data is actually appended to your m_data object and also not removed from m_data.

markugra

Thanks for your comments.
To clarify by modifying the data I am talking only about appending and removing data from a list. In fact in my implementation the DataModel would be a QList Alarm Objects.
So the datalayer would look something like this:

class DataLayer: public QObject
{
Q_OBJECT
...
public:
appendAlarm(properties);
signals:
alarmsAppended();
alarmsRemoved();
...

QList<Alarm*> m_data;
}


DataLayer::appendAlarm(properties)
{
m_data.append(new Alarm(this,properties));
emit alarmsAppended();
}

Christian Ehrlicher

This can be safely go into the QAbstractListModel - to much uselss work and signals/slots and other stuff which can go wrong just to hold a QList of pointer to Alarm instances ...

VRonin

The problem in your code is the order of calls. You should call appendAlarm only after beginInsertRows.
In handleAlarmAppened rowCount will return already the number of rows after the append happened so you are telling the view to act on a row it doesn't know it exists

markugra

@ Christian: Thank you for your comments. I agree that it would be easy to just not separate data and model and I am considering doing that. However, it is not just this model that I have to consider but a bunch of other data that is stored in the DataLayer for which I created an InterfaceLayer that holds references to the stored data in a form that they are exposed to QML (this is where the model would go). The idea is that the InterfaceLayer could be easily exchanged in case one would like to switch from QML to another frontend. I am not experienced in this approach, though, so if I and up having to duplicate all the signals in the (beginFun(), endFun(),...) it might not be worth it. I thought there might be a cleaner way, that I was missing...

@VRonin: Thanks for your remarks. The first part of the code I provided is from an example that I found online whereas the second part is kind of pseudo code showing how the example code would be used (to answer previous remarks). Sorry for not clarifying this. For it to work I would indeed need to replace the rowCount() with an index that gets send as parameter with the append signal. The first point you mention is the main problem that I have with the example code. It seems like I need to duplicate all signals.

VRonin

@markugra said in Separation of Model and Data (QAbstractListModel):

It seems like I need to duplicate all signals.

I'm afraid this is the case. Slots connected to begin*() signals should be able to rely on the fact that the model is in the exactly the same state as the one before the change.

In your case, you might cheat and still get away with it but you need to adjust the indexes you pass to begin*() to be the indexes pre-change

markugra

Ok, so just to summarize: In order to have the model and data separated I would have the following signals in the datalayer:

void beginAppendAlarm(int index);
void endAppendAlarm();
//same for remove

and the corresponding slots in the interface layer:

InterfaceLayer::beginAppendAlarm(int index){
    beginInsertRows(QModelIndex(),index,index);
}
InterfaceLayer::endAppendAlarm(){
    endInsertRows();
}

Coming back to Christian's comments i.e. "I don't think that holding a reference on the data is a good approach (I would not do it - it's looks like it's calling for crashes and undefined behavior)." and "to much uselss work and signals/slots and other stuff which can go wrong just to hold a QList of pointer to Alarm instances"

Aside from the additional complexity in terms of maintainability and usability introduced by the InterfaceLayer are there any other issues with this approach? Can you come up with scenarios where it actually crashes or things I would need to pay attention to?

I appreciate your input!