[Merge] lp:~jonas-drange/ubuntu-settings-components/asyncness into lp:~phablet-team/ubuntu-settings-components/printer-components
Jonas G. Drange
jonas.drange at canonical.com
Thu Feb 16 10:59:40 UTC 2017
> Is there any reason you can't show the printerPageLoaded to start the loading? and then show the ActivityIndicator as an overlay while printer.isLoaded is false?
Yes, it's to stay the warnings. It also will load the printer page a lot quicker. But it's also just an example :)
> QUESTION: Hah! is this to start the lazy load? wonder if there is a better way todo this?
Would love a suggestion. :)
> NOTE: can you check the following and remove if it does?
>>> Cups will notify us (I think.)
It will, removed.
> QUESTION: how costly is this? maybe there could be two private methods:
>> getDest(printerName, instance) and getPpdFile(printerName, instance)
Good idea! Done.
> what is this used or?
>> if (m_printerName.isEmpty()) {
>> throw std::invalid_argument("Trying to refresh unnamed printer.");
It's used to catch bugs. If it throws, then we got in a situation where a printer that's expected to work, has no name, which is an exception and can't be handled properly by that piece of code.
> QUESTION: could the above not be the following to make it a 1 liner?
Done
> FIXME: can the values be set in the member initializer list?
> const QStringList m_knownQualityOptions = QStringList({
Done
>FIXME: can the -1 be set in the member initializer list?
> int m_cupsSubscriptionId = -1;
Done
> NOTE: maybe instead move this inside an else case for the above if
> to make it more clear this is the !accept route and then the comment can
> be removed
Done
> QUESTION: should we call it dest? maybe printerName is better?
Done
> QUESTION: is owner used anymore? is this now user?
> OwnerRole,
It was the owning printer, but now the printer does not own a JobModel anymore (just a proxy), so I've removed it.
> QUESTION: I think the 1 should be 'from'? So it should read like the following
>> !beginMoveRows(QModelIndex(), from, from, QModelIndex(), to)
> As in the examples on http://doc.qt.io/qt-5/qabstractitemmodel.html#beginMoveRows
> they have beginMoveRows(parent, 2, 2, parent, 0); to move one row?
Yup, done
> QUESTION: i think we said that this should be the default case?
> so if anything other than the above cases is hit, lazy load starts
You're right, done.
--
https://code.launchpad.net/~jonas-drange/ubuntu-settings-components/asyncness/+merge/316786
Your team Ubuntu Phablet Team is subscribed to branch lp:~phablet-team/ubuntu-settings-components/printer-components.
More information about the Ubuntu-reviews
mailing list