ubuntu-touch-coreapps-reviewers team mailing list archive

[Riccardo Padovani <riccardo@xxxxxxxxxxxxx>]

Review: Needs Information

Looks good to me, but I left a couple of comments, just to be sure changes you made are wanted :-)

Diff comments:

> === modified file 'src/libqtevernote/evernoteconnection.cpp'
> --- src/libqtevernote/evernoteconnection.cpp	2015-02-27 21:32:29 +0000
> +++ src/libqtevernote/evernoteconnection.cpp	2015-03-04 22:15:09 +0000
> @@ -153,11 +153,17 @@
>          return;
>      }
>  
> -    foreach (EvernoteJob *job, m_jobQueue) {
> -        job->emitJobDone(EvernoteConnection::ErrorCodeConnectionLost, "Disconnected from Evernote");
> -        job->deleteLater();
> -    }
> -    m_jobQueue.clear();
> +    foreach (EvernoteJob *job, m_highPriorityJobQueue) {
> +        job->emitJobDone(EvernoteConnection::ErrorCodeConnectionLost, "Disconnected from Evernote");
> +        job->deleteLater();
> +    }
> +    m_highPriorityJobQueue.clear();
> +
> +    foreach (EvernoteJob *job, m_lowPriorityJobQueue) {
> +        job->emitJobDone(EvernoteConnection::ErrorCodeConnectionLost, "Disconnected from Evernote");
> +        job->deleteLater();
> +    }
> +    m_lowPriorityJobQueue.clear();
>  
>      m_errorMessage.clear();
>      emit errorChanged();
> @@ -342,7 +348,13 @@
>  
>  EvernoteJob* EvernoteConnection::findDuplicate(EvernoteJob *job)
>  {
> -    foreach (EvernoteJob *queuedJob, m_jobQueue) {
> +    foreach (EvernoteJob *queuedJob, m_highPriorityJobQueue) {
> +        // explicitly use custom operator==()
> +        if (job->operator ==(queuedJob)) {
> +            return queuedJob;
> +        }
> +    }
> +    foreach (EvernoteJob *queuedJob, m_lowPriorityJobQueue) {
>          // explicitly use custom operator==()
>          if (job->operator ==(queuedJob)) {
>              return queuedJob;
> @@ -368,17 +380,21 @@
>          if (job->jobPriority() == EvernoteJob::JobPriorityHigh) {
>              qDebug() << "[JobQueue] Reprioritising duplicate job:" << job->toString();
>              duplicate->setJobPriority(job->jobPriority());
> -            m_jobQueue.prepend(m_jobQueue.takeAt(m_jobQueue.indexOf(duplicate)));
> +            if (m_highPriorityJobQueue.contains(job)) {
> +                m_highPriorityJobQueue.prepend(m_highPriorityJobQueue.takeAt(m_highPriorityJobQueue.indexOf(duplicate)));
> +            } else {
> +                m_highPriorityJobQueue.prepend(m_lowPriorityJobQueue.takeAt(m_lowPriorityJobQueue.indexOf(duplicate)));

I'm not sure about this: when a low priority job is appended for the second time you move it to the highPriority queue?

> +            }
>          }
>      } else {
>          connect(job, &EvernoteJob::finished, job, &EvernoteJob::deleteLater);
>          connect(job, &EvernoteJob::finished, this, &EvernoteConnection::startNextJob);
>          if (job->jobPriority() == EvernoteJob::JobPriorityHigh) {
>              qDebug() << "[JobQueue] Prepending high priority job request:" << job->toString();
> -            m_jobQueue.prepend(job);
> +            m_highPriorityJobQueue.prepend(job);
>          } else {
>              qDebug() << "[JobQueue] Appending low priority job request:" << job->toString();
> -            m_jobQueue.append(job);
> +            m_lowPriorityJobQueue.prepend(job);

So new low priority jobs go to the start of the low priority queue? Shouldn't go to the end?

>          }
>          startJobQueue();
>      }
> @@ -401,15 +417,20 @@
>  
>  void EvernoteConnection::startJobQueue()
>  {
> -    if (m_jobQueue.isEmpty()) {
> -        return;
> -    }
> -
>      if (m_currentJob) {
>          return;
>      }
>  
> -    m_currentJob = m_jobQueue.takeFirst();
> +    if (m_highPriorityJobQueue.isEmpty() && m_lowPriorityJobQueue.isEmpty()) {
> +        return;
> +    }
> +
> +    if (!m_highPriorityJobQueue.isEmpty()) {
> +        m_currentJob = m_highPriorityJobQueue.takeFirst();
> +    } else {
> +        m_currentJob = m_lowPriorityJobQueue.takeFirst();
> +    }
> +
>      qDebug() << "[JobQueue] Starting job:" << m_currentJob->toString();
>      m_currentJob->start();
>  }
> 
> === modified file 'src/libqtevernote/evernoteconnection.h'
> --- src/libqtevernote/evernoteconnection.h	2015-02-26 22:47:10 +0000
> +++ src/libqtevernote/evernoteconnection.h	2015-03-04 22:15:09 +0000
> @@ -115,7 +115,8 @@
>  
>      // There must be only one job running at a time
>      // Do not start jobs other than with startJobQueue()
> -    QList<EvernoteJob*> m_jobQueue;
> +    QList<EvernoteJob*> m_highPriorityJobQueue;
> +    QList<EvernoteJob*> m_lowPriorityJobQueue;
>      EvernoteJob *m_currentJob;
>  
>      // Those need to be mutexed
> 


-- 
https://code.launchpad.net/~mzanetti/reminders-app/two-job-queues/+merge/251838
Your team Ubuntu Reminders app developers is subscribed to branch lp:reminders-app.