ubuntu-touch-coreapps-reviewers team mailing list archive

[Michael Zanetti <michael.zanetti@xxxxxxxxxxxxx>]

Thanks for those comments. While I think this branch is ok, I found an issue with the attachToDuplicate() mechanism. I will fix that in another branch, as that issue is not introduced by this branch.

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)));

Only if the newly appended job is of high priority.

For example: the app is started and the content of a few notes is fetched because we want to show the tagline in the notes list. Those jobs are of low priority. Then the user clicks a note while the low priority job to fetch this note's content is still waiting in the waiting queue. The opening of the note will create a high priority job but otherwise everything is the same.

So the job queue finds the (low priority) duplicate, and because the new request is of high priority, the original one is moved to the high priority queue and the new duplicate one attached to it.

So the reprioritizing is indeed what we want I think.

> +            }
>          }
>      } 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);

Yes, we always prepend to the job queues. The reason is that a more recent request is more likely to satisfy the user. If the user flicks through the whole list of notes and stops at the end, we try to deliver the bottom notes first, before going back and slowly fetch the ones that are scrolled outside the view already.

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