ubuntu-touch-coreapps-reviewers team mailing list archive

[Michael Zanetti <michael.zanetti@xxxxxxxxxxxxx>]

Michael Zanetti has proposed merging lp:~mzanetti/reminders-app/two-job-queues into lp:reminders-app with lp:~mzanetti/reminders-app/cleanup-debug as a prerequisite.

Commit message:
Further improve the jobqueue by splitting it up into high and low priority queues and optimizing the reply rate.

Requested reviews:
  Riccardo Padovani (rpadovani)
  Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot): continuous-integration

For more details, see:
https://code.launchpad.net/~mzanetti/reminders-app/two-job-queues/+merge/252175
-- 
Your team Ubuntu Reminders app developers is subscribed to branch lp:reminders-app.

=== modified file 'src/libqtevernote/evernoteconnection.cpp'
--- src/libqtevernote/evernoteconnection.cpp	2015-03-06 21:15:32 +0000
+++ src/libqtevernote/evernoteconnection.cpp	2015-03-06 21:15:32 +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();
@@ -340,11 +346,27 @@
     return false;
 }
 
-EvernoteJob* EvernoteConnection::findDuplicate(EvernoteJob *job)
-{
-    foreach (EvernoteJob *queuedJob, m_jobQueue) {
-        // explicitly use custom operator==()
-        if (job->operator ==(queuedJob)) {
+void EvernoteConnection::attachDuplicate(EvernoteJob *original, EvernoteJob *duplicate)
+{
+    if (duplicate->originatingObject() && duplicate->originatingObject() != original->originatingObject()) {
+        duplicate->attachToDuplicate(m_currentJob);
+    }
+    connect(original, &EvernoteJob::finished, duplicate, &EvernoteJob::deleteLater);
+}
+
+EvernoteJob* EvernoteConnection::findExistingDuplicate(EvernoteJob *job)
+{
+    foreach (EvernoteJob *queuedJob, m_highPriorityJobQueue) {
+        // explicitly use custom operator==()
+        if (job->operator ==(queuedJob)) {
+            qCDebug(dcJobQueue) << "Found duplicate in high priority queue";
+            return queuedJob;
+        }
+    }
+    foreach (EvernoteJob *queuedJob, m_lowPriorityJobQueue) {
+        // explicitly use custom operator==()
+        if (job->operator ==(queuedJob)) {
+            qCDebug(dcJobQueue) << "Found duplicate in low priority queue";
             return queuedJob;
         }
     }
@@ -359,26 +381,38 @@
         job->deleteLater();
         return;
     }
-    EvernoteJob *duplicate = findDuplicate(job);
-    if (duplicate) {
-        job->attachToDuplicate(duplicate);
-        connect(duplicate, &EvernoteJob::finished, job, &EvernoteJob::deleteLater);
+    if (m_currentJob && m_currentJob->operator ==(job)) {
+        qCDebug(dcJobQueue) << "Duplicate of new job request already running:" << job->toString();
+        if (m_currentJob->isFinished()) {
+            job->deleteLater();
+        } else {
+            attachDuplicate(m_currentJob, job);
+        }
+        return;
+    }
+    EvernoteJob *existingJob = findExistingDuplicate(job);
+    if (existingJob) {
+        qCDebug(dcJobQueue) << "Duplicate job already queued:" << job->toString();
+        attachDuplicate(existingJob, job);
         // reprioritze the repeated request
-        qCDebug(dcJobQueue) << "Duplicate job already queued:" << job->toString();
         if (job->jobPriority() == EvernoteJob::JobPriorityHigh) {
             qCDebug(dcJobQueue) << "Reprioritising duplicate job:" << job->toString();
-            duplicate->setJobPriority(job->jobPriority());
-            m_jobQueue.prepend(m_jobQueue.takeAt(m_jobQueue.indexOf(duplicate)));
+            existingJob->setJobPriority(job->jobPriority());
+            if (m_highPriorityJobQueue.contains(existingJob)) {
+                m_highPriorityJobQueue.prepend(m_highPriorityJobQueue.takeAt(m_highPriorityJobQueue.indexOf(existingJob)));
+            } else {
+                m_highPriorityJobQueue.prepend(m_lowPriorityJobQueue.takeAt(m_lowPriorityJobQueue.indexOf(existingJob)));
+            }
         }
     } else {
         connect(job, &EvernoteJob::finished, job, &EvernoteJob::deleteLater);
         connect(job, &EvernoteJob::finished, this, &EvernoteConnection::startNextJob);
         if (job->jobPriority() == EvernoteJob::JobPriorityHigh) {
-            qCDebug(dcJobQueue) << "Prepending high priority job request:" << job->toString();
-            m_jobQueue.prepend(job);
+            qCDebug(dcJobQueue) << "Adding high priority job request:" << job->toString();
+            m_highPriorityJobQueue.prepend(job);
         } else {
-            qCDebug(dcJobQueue) << "Appending low priority job request:" << job->toString();
-            m_jobQueue.append(job);
+            qCDebug(dcJobQueue) << "Adding low priority job request:" << job->toString();
+            m_lowPriorityJobQueue.prepend(job);
         }
         startJobQueue();
     }
@@ -401,15 +435,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();
+    }
+
     qCDebug(dcJobQueue) << "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-06 21:15:32 +0000
@@ -73,6 +73,18 @@
     QString token() const;
     void setToken(const QString &token);
 
+    // This will add the job to the job queue. The job queue will take ownership of the object
+    // and manage it's lifetime.
+    // * If there is an identical job already existing in the queue, the duplicate will be
+    //   attached to original job and not actually fetched a second time from the network in
+    //   order to reduce network traffic.
+    // * If the new job has a higher priority than the existing one, the existing one will
+    //   reprioritized to the higher priorty.
+    // * If the jobs have different originatingObjects, each job will emit the jobDone signal,
+    //   if instead the originatingObject is the same in both jobs, only one of them will emit
+    //   a jobDone signal. This is useful if you want to reschedule a job with higher priority
+    //   without having to track previously queued jobs and avoid invoking the connected slot
+    //   multiple times.
     void enqueue(EvernoteJob *job);
 
     bool isConnected() const;
@@ -104,7 +116,10 @@
     bool connectUserStore();
     bool connectNotesStore();
 
-    EvernoteJob* findDuplicate(EvernoteJob *job);
+    EvernoteJob* findExistingDuplicate(EvernoteJob *job);
+
+    // "duplicate" will be attached to "original"
+    void attachDuplicate(EvernoteJob *original, EvernoteJob *duplicate);
 
     bool m_useSSL;
     bool m_isConnected;
@@ -115,7 +130,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

=== modified file 'src/libqtevernote/jobs/evernotejob.cpp'
--- src/libqtevernote/jobs/evernotejob.cpp	2015-03-06 21:15:32 +0000
+++ src/libqtevernote/jobs/evernotejob.cpp	2015-03-06 21:15:32 +0000
@@ -38,10 +38,11 @@
 using namespace apache::thrift::protocol;
 using namespace apache::thrift::transport;
 
-EvernoteJob::EvernoteJob(QObject *parent, JobPriority jobPriority) :
-    QThread(parent),
+EvernoteJob::EvernoteJob(QObject *originatingObject, JobPriority jobPriority) :
+    QThread(nullptr),
     m_token(EvernoteConnection::instance()->token()),
-    m_jobPriority(jobPriority)
+    m_jobPriority(jobPriority),
+    m_originatingObject(originatingObject)
 {
 }
 
@@ -198,6 +199,11 @@
     return metaObject()->className();
 }
 
+QObject *EvernoteJob::originatingObject() const
+{
+    return m_originatingObject;
+}
+
 QString EvernoteJob::token()
 {
     return m_token;

=== modified file 'src/libqtevernote/jobs/evernotejob.h'
--- src/libqtevernote/jobs/evernotejob.h	2015-02-26 22:47:10 +0000
+++ src/libqtevernote/jobs/evernotejob.h	2015-03-06 21:15:32 +0000
@@ -37,8 +37,7 @@
  *   your job won't be executed but you should instead forward the other's job results.
  *
  * Jobs can be enqueue()d in NotesStore.
- * They will destroy themselves when finished.
- * The jobqueue will take care about starting them.
+ * The jobqueue will take care about starting them and deleting them.
  */
 class EvernoteJob : public QThread
 {
@@ -49,7 +48,7 @@
         JobPriorityLow
     };
 
-    explicit EvernoteJob(QObject *parent = 0, JobPriority jobPriority = JobPriorityHigh);
+    explicit EvernoteJob(QObject *originatingObject = 0, JobPriority jobPriority = JobPriorityHigh);
     virtual ~EvernoteJob();
 
     JobPriority jobPriority() const;
@@ -63,6 +62,8 @@
 
     virtual QString toString() const;
 
+    QObject* originatingObject() const;
+
 signals:
     void connectionLost(const QString &errorMessage);
 
@@ -76,6 +77,7 @@
 private:
     QString m_token;
     JobPriority m_jobPriority;
+    QObject *m_originatingObject;
 
     friend class EvernoteConnection;
 };

=== modified file 'src/libqtevernote/jobs/fetchnotesjob.cpp'
--- src/libqtevernote/jobs/fetchnotesjob.cpp	2015-03-06 21:15:32 +0000
+++ src/libqtevernote/jobs/fetchnotesjob.cpp	2015-03-06 21:15:32 +0000
@@ -41,7 +41,9 @@
         return false;
     }
     return this->m_filterNotebookGuid == otherJob->m_filterNotebookGuid
-            && this->m_searchWords == otherJob->m_searchWords;
+            && this->m_searchWords == otherJob->m_searchWords
+            && this->m_startIndex == otherJob->m_startIndex
+            && this->m_chunkSize == otherJob->m_chunkSize;
 }
 
 void FetchNotesJob::attachToDuplicate(const EvernoteJob *other)

=== modified file 'src/libqtevernote/note.cpp'
--- src/libqtevernote/note.cpp	2015-03-06 21:15:32 +0000
+++ src/libqtevernote/note.cpp	2015-03-06 21:15:32 +0000
@@ -243,7 +243,7 @@
 
 QString Note::enmlContent() const
 {
-    load();
+    load(true);
     return m_content.enml();
 }
 
@@ -263,13 +263,13 @@
 
 QString Note::htmlContent() const
 {
-    load();
+    load(true);
     return m_content.toHtml(m_guid);
 }
 
 QString Note::richTextContent() const
 {
-    load();
+    load(true);
     return m_content.toRichText(m_guid);
 }
 
@@ -625,12 +625,14 @@
     if (m_loading != loading) {
         m_loading = loading;
         emit loadingChanged();
+    }
 
-        if (!m_loading) {
-            m_loadingHighPriority = false;
-        } else {
-            m_loadingHighPriority = highPriority;
+    if (m_loading) {
+        if (!m_loadingHighPriority && highPriority) {
+            m_loadingHighPriority = true;
         }
+    } else {
+        m_loadingHighPriority = false;
     }
 }
 

=== modified file 'src/libqtevernote/note.h'
--- src/libqtevernote/note.h	2015-03-06 21:15:32 +0000
+++ src/libqtevernote/note.h	2015-03-06 21:15:32 +0000
@@ -212,7 +212,7 @@
     void setConflicting(bool conflicting);
 
     // const because we want to load on demand in getters. Keep this private!
-    void load(bool priorityHigh = true) const;
+    void load(bool highPriority = false) const;
     void loadFromCacheFile() const;
 
 private:

=== modified file 'src/libqtevernote/notesstore.cpp'
--- src/libqtevernote/notesstore.cpp	2015-03-06 21:15:32 +0000
+++ src/libqtevernote/notesstore.cpp	2015-03-06 21:15:32 +0000
@@ -797,6 +797,11 @@
         qCWarning(dcSync) << "RefreshNoteContent: Can't refresn note content. Note guid not found:" << guid;
         return;
     }
+    qCDebug(dcSync) << "should start another one?" << note->loading() << note->m_loadingHighPriority;
+    if (note->loading() && (priority != EvernoteJob::JobPriorityHigh || note->m_loadingHighPriority)) {
+        qCDebug(dcSync) << "Load already loading with high priorty. Not starting again";
+        return;
+    }
     if (EvernoteConnection::instance()->isConnected()) {
         qCDebug(dcNotesStore) << "Fetching note content from network for note" << guid << (what == FetchNoteJob::LoadContent ? "content" : "image");
         FetchNoteJob *job = new FetchNoteJob(guid, what, this);
@@ -821,13 +826,9 @@
     QModelIndex noteIndex = index(m_notes.indexOf(note));
     QVector<int> roles;
 
-    note->setLoading(false);
-    roles << RoleLoading;
-
     switch (errorCode) {
     case EvernoteConnection::ErrorCodeNoError:
         // All is well
-        emit dataChanged(noteIndex, noteIndex, roles);
         break;
     case EvernoteConnection::ErrorCodeUserException:
         qCWarning(dcSync) << "FetchNoteJobDone: EDAMUserException:" << errorMessage;
@@ -915,6 +916,10 @@
         note->setReminderDoneTime(reminderDoneTime);
         roles << RoleReminderDone << RoleReminderDoneTime;
     }
+
+    note->setLoading(false);
+    roles << RoleLoading;
+
     emit noteChanged(note->guid(), note->notebookGuid());
 
     emit dataChanged(noteIndex, noteIndex, roles);