[Merge] lp:~ahayzen/ubuntu-ui-extras/job-model-split-update into lp:~phablet-team/ubuntu-ui-extras/printer-staging
Jonas G. Drange
jonas.drange at canonical.com
Tue Mar 14 12:13:58 UTC 2017
Review: Needs Information
Looks good! A couple of requests for information.
Diff comments:
>
> === modified file 'modules/Ubuntu/Components/Extras/Printers/backend/backend_cups.h'
> --- modules/Ubuntu/Components/Extras/Printers/backend/backend_cups.h 2017-03-08 11:30:48 +0000
> +++ modules/Ubuntu/Components/Extras/Printers/backend/backend_cups.h 2017-03-13 15:02:28 +0000
> @@ -141,8 +147,11 @@
> mutable QMap<QString, cups_dest_t*> m_dests; // Printer name, dest.
> mutable QMap<QString, ppd_file_t*> m_ppds; // Printer name, ppd.
> QSet<QString> m_activeRequests;
I wonder if you could rename this to m_activePrinterRequests.
> + QSet<QPair<QString, int>> m_activeJobRequests;
>
> private Q_SLOTS:
> + void onJobLoaded(QSharedPointer<PrinterJob> oldJob,
> + QSharedPointer<PrinterJob> newJob);
> void onPrinterLoaded(QSharedPointer<Printer> printer);
> };
>
>
> === modified file 'modules/Ubuntu/Components/Extras/Printers/cups/ippclient.cpp'
> --- modules/Ubuntu/Components/Extras/Printers/cups/ippclient.cpp 2017-03-08 21:05:19 +0000
> +++ modules/Ubuntu/Components/Extras/Printers/cups/ippclient.cpp 2017-03-13 15:02:28 +0000
> @@ -455,6 +455,8 @@
> QMap<QString, QVariant> IppClient::printerGetJobAttributes(const QString &printerName,
> const int jobId)
> {
> + m_thread_lock.lock();
Could you inform me what will happen when a thread is locked out? Will it keep trying until it can get a lock? What's the behavior?
> +
> ipp_t *request;
> QMap<QString, QVariant> map;
>
>
> === modified file 'modules/Ubuntu/Components/Extras/Printers/models/jobmodel.cpp'
> --- modules/Ubuntu/Components/Extras/Printers/models/jobmodel.cpp 2017-03-06 13:19:08 +0000
> +++ modules/Ubuntu/Components/Extras/Printers/models/jobmodel.cpp 2017-03-13 15:02:28 +0000
> @@ -31,113 +31,151 @@
> : QAbstractListModel(parent)
> , m_backend(backend)
> {
> - update();
> -
> QObject::connect(m_backend, &PrinterBackend::jobCreated,
> - this, &JobModel::jobSignalCatchAll);
> + this, &JobModel::jobCreated);
> QObject::connect(m_backend, &PrinterBackend::jobState,
> - this, &JobModel::jobSignalCatchAll);
> + this, &JobModel::jobState);
> QObject::connect(m_backend, &PrinterBackend::jobCompleted,
> - this, &JobModel::jobSignalCatchAll);
> + this, &JobModel::jobCompleted);
> +
> + connect(m_backend, SIGNAL(jobLoaded(QSharedPointer<PrinterJob>, QSharedPointer<PrinterJob>)),
> + this, SLOT(updateJob(QSharedPointer<PrinterJob>, QSharedPointer<PrinterJob>)));
> +
> + // Add already existing jobs
> + // FIXME: even this should probably be in a background thread?
Maybe. For printer's we have this proxytype. Maybe you could introduce the same semantics?
> + // so that it doesn't block startup?
> + Q_FOREACH(auto job, m_backend->printerGetJobs()) {
> + addJob(job);
> + }
> }
>
> JobModel::~JobModel()
> {
> }
>
> -void JobModel::jobSignalCatchAll(
> - const QString &text, const QString &printer_uri,
> - const QString &printer_name, uint printer_state,
> - const QString &printer_state_reasons, bool printer_is_accepting_jobs,
> - uint job_id, uint job_state, const QString &job_state_reasons,
> - const QString &job_name, uint job_impressions_completed)
> -{
> - Q_UNUSED(text);
> - Q_UNUSED(printer_uri);
> - Q_UNUSED(printer_name);
> - Q_UNUSED(printer_state);
> - Q_UNUSED(printer_state_reasons);
> - Q_UNUSED(printer_is_accepting_jobs);
> - Q_UNUSED(job_id);
> +void JobModel::jobCreated(
> + const QString &text, const QString &printer_uri,
> + const QString &printer_name, uint printer_state,
> + const QString &printer_state_reasons, bool printer_is_accepting_jobs,
> + uint job_id, uint job_state, const QString &job_state_reasons,
> + const QString &job_name, uint job_impressions_completed)
> +{
> + Q_UNUSED(text); // "Job Created"
> + Q_UNUSED(printer_uri);
> + Q_UNUSED(printer_state);
> + Q_UNUSED(printer_state_reasons);
> + Q_UNUSED(printer_is_accepting_jobs);
> + Q_UNUSED(job_state_reasons);
> +
> + QSharedPointer<PrinterJob> job = QSharedPointer<PrinterJob>(
> + new PrinterJob(printer_name, m_backend, job_id)
> + );
> + job->setImpressionsCompleted(job_impressions_completed);
> + job->setState(static_cast<PrinterEnum::JobState>(job_state));
> + job->setTitle(job_name);
> +
> + // Printers listens to rowInserted and spawns a JobLoader to set the
> + // printer of the job, which triggers the extended attributes to be loaded
> + // once complete this triggers JobModel::updateJob
> + addJob(job);
> +}
> +
> +void JobModel::jobState(
> + const QString &text, const QString &printer_uri,
> + const QString &printer_name, uint printer_state,
> + const QString &printer_state_reasons, bool printer_is_accepting_jobs,
> + uint job_id, uint job_state, const QString &job_state_reasons,
> + const QString &job_name, uint job_impressions_completed)
> +{
> + Q_UNUSED(text);
> + Q_UNUSED(printer_uri);
> + Q_UNUSED(printer_state);
> + Q_UNUSED(printer_state_reasons);
> + Q_UNUSED(printer_is_accepting_jobs);
> + Q_UNUSED(job_state_reasons);
> + Q_UNUSED(job_name);
> +
> + QSharedPointer<PrinterJob> job = getJob(printer_name, job_id);
> +
> + if (job) {
> + job->setImpressionsCompleted(job_impressions_completed);
> + job->setState(static_cast<PrinterEnum::JobState>(job_state));
> +
> + updateJob(job);
> + } else {
> + qWarning() << "JobModel::jobState for unknown job: " << job_name << " ("
> + << job_id << ") for " << printer_name;
> + }
> +}
> +
> +void JobModel::jobCompleted(
> + const QString &text, const QString &printer_uri,
> + const QString &printer_name, uint printer_state,
> + const QString &printer_state_reasons, bool printer_is_accepting_jobs,
> + uint job_id, uint job_state, const QString &job_state_reasons,
> + const QString &job_name, uint job_impressions_completed)
> +{
> + Q_UNUSED(text);
> + Q_UNUSED(printer_uri);
> + Q_UNUSED(printer_state);
> + Q_UNUSED(printer_state_reasons);
> + Q_UNUSED(printer_is_accepting_jobs);
> Q_UNUSED(job_state);
> Q_UNUSED(job_state_reasons);
> Q_UNUSED(job_name);
> -
> - auto job = getJobById(job_id);
> - if (job)
> - job->setImpressionsCompleted(job_impressions_completed);
> -
> - update();
> -}
> -
> -void JobModel::update()
> -{
> - // Store the old count and get the new printers
> - int oldCount = m_jobs.size();
> - QList<QSharedPointer<PrinterJob>> newJobs = m_backend->printerGetJobs();
> -
> - // Go through the old model
> - for (int i=0; i < m_jobs.count(); i++) {
> - // Determine if the old printer exists in the new model
> - bool exists = false;
> -
> - Q_FOREACH(QSharedPointer<PrinterJob> p, newJobs) {
> - if (p->jobId() == m_jobs.at(i)->jobId()) {
> - exists = true;
> -
> - // Ensure the other properties of the job are up to date
> - if (!m_jobs.at(i)->deepCompare(p)) {
> - m_jobs.at(i)->updateFrom(p);
> -
> - Q_EMIT dataChanged(index(i), index(i));
> - }
> -
> - break;
> - }
> - }
> -
> - // If it doesn't exist then remove it from the old model
> - if (!exists) {
> - beginRemoveRows(QModelIndex(), i, i);
> - QSharedPointer<PrinterJob> p = m_jobs.takeAt(i);
> - endRemoveRows();
> -
> - i--; // as we have removed an item decrement
> - }
> - }
> -
> - // Go through the new model
> - for (int i=0; i < newJobs.count(); i++) {
> - // Determine if the new printer exists in the old model
> - bool exists = false;
> - int j;
> -
> - for (j=0; j < m_jobs.count(); j++) {
> - if (m_jobs.at(j)->jobId() == newJobs.at(i)->jobId()) {
> - exists = true;
> - break;
> - }
> - }
> -
> - if (exists) {
> - if (j == i) { // New printer exists and in correct position
> - continue;
> - } else {
> - // New printer does exist but needs to be moved in old model
> - beginMoveRows(QModelIndex(), j, 1, QModelIndex(), i);
> - m_jobs.move(j, i);
> - endMoveRows();
> - }
> - } else {
> - // New printer does not exist insert into model
> - beginInsertRows(QModelIndex(), i, i);
> - m_jobs.insert(i, newJobs.at(i));
> - endInsertRows();
> - }
> - }
> -
> - if (oldCount != m_jobs.size()) {
> - Q_EMIT countChanged();
> + Q_UNUSED(job_impressions_completed);
> +
> + auto job = getJob(printer_name, job_id);
> + if (job) {
> + removeJob(job);
> + } else {
> + qWarning() << "JobModel::jobCompleted for unknown job: " << job_name << " ("
> + << job_id << ") for " << printer_name;
> + }
> +}
> +
> +void JobModel::addJob(QSharedPointer<PrinterJob> job)
> +{
> + int i = m_jobs.size();
> +
> + beginInsertRows(QModelIndex(), i, i);
> + m_jobs.append(job);
> + endInsertRows();
> +
> + Q_EMIT countChanged();
> +}
> +
> +void JobModel::removeJob(QSharedPointer<PrinterJob> job)
> +{
> + int i = m_jobs.indexOf(job);
> +
> + beginRemoveRows(QModelIndex(), i, i);
> + m_jobs.removeAt(i);
> + endRemoveRows();
> +
> + Q_EMIT countChanged();
> +}
> +
> +// This is used by JobModel::jobState as it has modified an existing job
> +void JobModel::updateJob(QSharedPointer<PrinterJob> job)
> +{
> + int i = m_jobs.indexOf(job);
> + QModelIndex idx = index(i);
> + Q_EMIT dataChanged(idx, idx);
> +}
> +
> +// This is used by JobLoader as it creates a new job to prevent threading issues
Not entirely sure I understand this comment. Could you clarify?
> +void JobModel::updateJob(QSharedPointer<PrinterJob> oldJob,
> + QSharedPointer<PrinterJob> newJob)
> +{
> + int i = m_jobs.indexOf(oldJob);
> + QModelIndex idx = index(i);
> +
> + if (i > -1) {
> + oldJob->updateFrom(newJob);
> + Q_EMIT dataChanged(idx, idx);
> + } else {
> + qWarning() << "Tried to updateJob which doesn't exist:" << newJob->printerName() << newJob->jobId();
> }
> }
>
>
> === modified file 'modules/Ubuntu/Components/Extras/Printers/printer/printer.cpp'
> --- modules/Ubuntu/Components/Extras/Printers/printer/printer.cpp 2017-03-09 15:50:41 +0000
> +++ modules/Ubuntu/Components/Extras/Printers/printer/printer.cpp 2017-03-13 15:02:28 +0000
> @@ -379,10 +379,20 @@
> void Printer::updateFrom(QSharedPointer<Printer> other)
> {
> PrinterBackend *tmp = m_backend;
> +
> + // Copy values from other printer which has been loaded in another thread
Good. I wonder why it wasn't like this in the first place.
> + // Note: do not use loadAttributes otherwise can cause UI block
> + m_acceptJobs = other->m_acceptJobs;
> m_backend = other->m_backend;
> + m_defaultColorModel = other->m_defaultColorModel;
> + m_defaultPrintQuality = other->m_defaultPrintQuality;
> + m_deviceUri = other->m_deviceUri;
> + m_shared = other->m_shared;
> + m_stateMessage = other->m_stateMessage;
> + m_supportedColorModels = other->m_supportedColorModels;
> + m_supportedPrintQualities = other->m_supportedPrintQualities;
> +
> other->m_backend = tmp;
> -
> - loadAttributes();
> }
>
> void Printer::onPrinterStateChanged(
--
https://code.launchpad.net/~ahayzen/ubuntu-ui-extras/job-model-split-update/+merge/319676
Your team Ubuntu Phablet Team is subscribed to branch lp:~phablet-team/ubuntu-ui-extras/printer-staging.
More information about the Ubuntu-reviews
mailing list