Code review comment for lp:~unity-team/libusermetrics/end-to-end

Revision history for this message
MichaƂ Sawicz (saviq) wrote :

Hey, this is looking good, I'll try and skim through this for a code review, but I've a few comments looking at the API:

- there's no reason for the separate infographics property, just make InfographicsList a QAbstractListModel (any reason for using *ItemModel instead, btw?) with just a "source" role

- as discussed on IRC, the filesUpdated() signal feels like a dirty API, I'd rather see timestamped file names and dataChanged on the "source" role when applicable; ideally both files would be available at the same time for a while (old files periodically garbage collected, basically?), in case we're still loading the old file while a new one got added

- the timeout logic feels unnecessary, we'll only be loading a single file (two at most during transitions); throttling should probably happen on the visualizer side, though?

- should we be (are we?) running the visualizers whenever data changes, or only when greeter is shown? on one hand that would mean they're ready when greeter is shown, on the other they might never be shown at all (in case user never switches between images)

BTW, what's the plan for i18n? Are we ok with the SVGs only updating the language on next update, or should we maybe kick the visualisers on locale change?

review: Needs Information

« Back to merge proposal