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)
Details
Details
- Reviewers
cmollekopf - Maniphest Tasks
- T8225: Implement event models
- Commits
- R162:b8d76329bced: Implement PeriodDayEventModel
Diff Detail
Diff Detail
- Repository
- R162 Kube
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
| framework/src/domain/eventtreemodel.cpp | ||
|---|---|---|
| 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. |
| framework/src/domain/eventtreemodel.h | ||
| 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. |
Comment Actions
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 ↗ | (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. |
Comment Actions
Done:
- 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
Todo:
Maybe rename to DayEventModel?DONE (but to PeriodDayEventModel)Filter according to the view's date, currently we get every events in the week viewDONE- Use the parent / children model in qml, instead of relying on the ".events" attribute
Comment Actions
- Add period filtering (with start and length properties)
- Rename EventTreeModel to PeriodDayEventModel (because it does the partitioning + filter according to a period)
TODO:
- 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
| framework/src/domain/eventtreemodel.cpp | ||
|---|---|---|
| 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 |
Comment Actions
Great work, I think we can merge this after the nitpicks are fixed.
| framework/src/domain/perioddayeventmodel.cpp | ||
|---|---|---|
| 83 | const QDate & | |
| 218 | Perhaps convert this into: return {{1, "foo"},
{2, "bar"}}; | |
| 234 | Pass all non-pod types by const-reference. and then don't move instead. | |
| 257 | This std::move doesn't do anything AFAIK. int doesn't have a move constructor. | |
| framework/src/domain/perioddayeventmodel.h | ||
| 114 | pass by const ref | |
| 121 | We use "const QDate &candidate" everywhere else, so let's stick to that. | |
| framework/src/domain/perioddayeventmodel.cpp | ||
|---|---|---|
| 234 | 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 | |
| 257 | Right, I've just seen it. I think it's a leftover from when I wanted a setPeriodEnd instead of setPeriodLength | |