Code review comment for lp:~jonas-drange/ubuntu-settings-components/asyncness

Revision history for this message
Jonas G. Drange (jonas-drange) wrote :

> 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.

« Back to merge proposal