Implement EventTreeModel

Authored by rnicole on Apr 10 2018, 2:24 PM.




When finished, should fix T8225


  • Remove the test data and integrate it with sink DONE
  • Use the parent / children model in qml, instead of relying on the ".events" attribute (using DelegateModel I think)

Diff Detail

R162 Kube
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
rnicole requested review of this revision.Apr 10 2018, 2:24 PM
rnicole created this revision.
cmollekopf requested changes to this revision.Apr 11 2018, 8:34 AM
cmollekopf added inline comments.
33 ↗(On Diff #31820)

This should probably be a live query, otherwise it won't updated.

41 ↗(On Diff #31820)

You will have to react to rowsInserted/rowsRemoved as well. dataChanged only tells you about an individual index changing.

43 ↗(On Diff #31820)

Synchronizing in the constructor is never a good idea, and synchronizing everything even less so.

Synchronization should (almost) always be triggered by user interaction, see sinkfabric.cpp for examples.

2 ↗(On Diff #31820)

Your copyright is missing.

57 ↗(On Diff #31820)

Maybe this should rather be a DayEventModel (or something alike?)

The toplevel nodes are days after all, so that seems like a defining property of the model.

This revision now requires changes to proceed.Apr 11 2018, 8:34 AM

In general we'll rather want a model that can display an unlimited number of days, not fixed 7.
The model should be configured with a date-filter, that we'd ideally forward to the sink query, but for now we'll just filter inside the model.
That date-filter will then define how many rows the model has.

That way we will be able to reuse the same model for a day-view, 5-day view, month-view ....

58 ↗(On Diff #31820)

You seem to assume that you only get events from a single week. Otherwise you will end up with e.g. all events that occur on a monday ever in the monday slot.

65 ↗(On Diff #31820)

You have to notify about rowsInserted otherwise the view doesn't pick it up.

rnicole updated this revision to Diff 31899.EditedApr 11 2018, 3:48 PM
rnicole marked 5 inline comments as done.
rnicole edited the summary of this revision. (Show Details)


  • Fix copyright
  • Switch to live query
  • React to layoutChanged, rowsInserted / Moved / Removed, and modelReset signals from sink's model
  • Remove debug manual synchronization of sink store
  • Notify views with the modelReset signal
  • Get data from the store instead of test data


  • Maybe rename to DayEventModel? DONE (but to PeriodDayEventModel)
  • Filter according to the view's date, currently we get every events in the week view DONE
  • Use the parent / children model in qml, instead of relying on the ".events" attribute
rnicole updated this revision to Diff 32261.Apr 16 2018, 9:09 AM
  • Add period filtering (with start and length properties)
  • Rename EventTreeModel to PeriodDayEventModel (because it does the partitioning + filter according to a period)


  • Get the start and length from the view side (currently hardcoded for testing)
  • Use the parent / children model in QML, instead of relying on the .events attribute
  • Fix the time scale in the view (currently, events are not aligned with the y axis)

Waiting on Michael to discuss the QML side

rnicole marked 2 inline comments as done.Apr 16 2018, 9:09 AM
rnicole added inline comments.
41 ↗(On Diff #31820)

I'm updating it to react to layoutChanged, rowsInserted / Moved / Removed as well, but from a quick look at sink's modelresult.cpp, the model doesn't seem to emit any of them, only dataChanged

43 ↗(On Diff #31820)

Right, that was only a temporary thing to test my assumptions

Great work, I think we can merge this after the nitpicks are fixed.


const QDate &


Perhaps convert this into:

return {{1, "foo"},
              {2, "bar"}};

Pass all non-pod types by const-reference. and then don't move instead.
Or is there a particular reason why this is better?


This std::move doesn't do anything AFAIK. int doesn't have a move constructor.


pass by const ref


We use "const QDate &candidate" everywhere else, so let's stick to that.

rnicole updated this revision to Diff 32379.Apr 17 2018, 11:52 AM
rnicole marked 5 inline comments as done.

Fix nitpicks

rnicole marked an inline comment as done.Apr 17 2018, 11:53 AM
rnicole added inline comments.

I guess you're right. The really optimized version would be to write a setPeriodStart(const QDate &) plus a setPeriodStart(QDate &&) where move only happens in the second one. Plus, it also depends on how Qt pass the parameters from the view. I guess the C++98 default is the best in our case (simple and fairly optimized).

More info here:


Right, I've just seen it. I think it's a leftover from when I wanted a setPeriodEnd instead of setPeriodLength

rnicole marked 2 inline comments as done.Apr 17 2018, 11:53 AM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 17 2018, 12:33 PM
Closed by commit R162:b8d76329bced: Implement PeriodDayEventModel (authored by rnicole, committed by cmollekopf). · Explain Why
This revision was automatically updated to reflect the committed changes.