WIP
When finished, should fix T8225
TODO:
Remove the test data and integrate it with sinkDONE- Use the parent / children model in qml, instead of relying on the ".events" attribute (using DelegateModel I think)
cmollekopf |
WIP
When finished, should fix T8225
TODO:
Lint Skipped |
Unit Tests Skipped |
framework/src/domain/eventtreemodel.cpp | ||
---|---|---|
33 | This should probably be a live query, otherwise it won't updated. | |
41 | You will have to react to rowsInserted/rowsRemoved as well. dataChanged only tells you about an individual index changing. | |
43 | 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. | |
framework/src/domain/eventtreemodel.h | ||
2 | Your copyright is missing. | |
57 | 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. |
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 ....
framework/src/domain/eventtreemodel.cpp | ||
---|---|---|
58 | 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 | You have to notify about rowsInserted otherwise the view doesn't pick it up. |
Done:
Todo:
TODO:
Waiting on Michael to discuss the QML side
Great work, I think we can merge this after the nitpicks are fixed.
framework/src/domain/perioddayeventmodel.cpp | ||
---|---|---|
82 ↗ | (On Diff #32261) | const QDate & |
217 ↗ | (On Diff #32261) | Perhaps convert this into: return {{1, "foo"}, {2, "bar"}}; |
233 ↗ | (On Diff #32261) | Pass all non-pod types by const-reference. and then don't move instead. |
256 ↗ | (On Diff #32261) | This std::move doesn't do anything AFAIK. int doesn't have a move constructor. |
framework/src/domain/perioddayeventmodel.h | ||
113 ↗ | (On Diff #32261) | pass by const ref |
120 ↗ | (On Diff #32261) | We use "const QDate &candidate" everywhere else, so let's stick to that. |
framework/src/domain/perioddayeventmodel.cpp | ||
---|---|---|
233 ↗ | (On Diff #32261) | 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: https://youtu.be/xnqTKD8uD64?t=51m6s |
256 ↗ | (On Diff #32261) | Right, I've just seen it. I think it's a leftover from when I wanted a setPeriodEnd instead of setPeriodLength |