Merge lp:~mzanetti/reminders-app/fix-writeback-issue into lp:reminders-app

[Michael Zanetti]

some improvements in job handling:

* treat write operations with highest priority
* don't dedupe or reorder write operations
* don't update the local note after a write operation. that will break local changes while the write operation is running.

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:467
http://91.189.93.70:8080/job/reminders-app-ci/766/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-utopic/3033
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic/3033/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/reminders-app-utopic-amd64-ci/368
    SUCCESS: http://91.189.93.70:8080/job/reminders-app-vivid-amd64-ci/188

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/reminders-app-ci/766/rebuild

review: Approve (continuous-integration)

[lp:~mzanetti/reminders-app/fix-writeback-issue]

468. By Michael Zanetti on 2015-07-05

more fixes

* handle multiple push notifications at once
* fix lastSyncSequenceNumber
* don't write back every checkbox change immediately, only when exiting

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:468
http://91.189.93.70:8080/job/reminders-app-ci/767/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-utopic/3034
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic/3034/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/reminders-app-utopic-amd64-ci/369
    SUCCESS: http://91.189.93.70:8080/job/reminders-app-vivid-amd64-ci/189

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/reminders-app-ci/767/rebuild

review: Approve (continuous-integration)

[Riccardo Padovani (rpadovani)]

Looks good to me, just a couple of comments inline (clarification, nothing wrong)

review: Needs Information

[Michael Terry (mterry)]

I'm not sure if this helped with my problem (bug 1487920)...

I tried this branch, and while it looked like it was trying to save (which is better than before!), it wasn't super successful:

[0824/120454:WARNING:channel.cc(549)] Failed to send message to ack remove remote endpoint (local ID 1, remote ID 1)
[0824/120454:WARNING:channel.cc(315)] RawChannel write error

[Michael Zanetti (mzanetti)]

interesting... no idea where that message comes from.

[lp:~mzanetti/reminders-app/fix-writeback-issue]

469. By Michael Zanetti on 2015-08-23

revert unwanted change

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:469
http://91.189.93.70:8080/job/reminders-app-ci/778/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/reminders-app-vivid-amd64-ci/200

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/reminders-app-ci/778/rebuild

review: Approve (continuous-integration)

[Riccardo Padovani (rpadovani)]

lgtm, thanks!

review: Approve

=== modified file 'src/app/qml/reminders.qml'
--- src/app/qml/reminders.qml	2015-07-25 03:01:39 +0000
+++ src/app/qml/reminders.qml	2015-09-08 16:22:25 +0000
@@ -360,32 +360,35 @@
         appId: root.applicationName + "_reminders"
 
         onNotificationsChanged: {
-            print("PushClient notification:", notifications)
-            var notification = JSON.parse(notifications)["payload"];
-
-            if (notification["userId"] != UserStore.userId) { // Yes, we want type coercion here.
-                console.warn("user mismatch:", notification["userId"], "!=", UserStore.userId)
-                return;
-            }
-
-            switch(notification["reason"]) {
-            case "update":
-                print("Note updated on server:", notification["guid"])
-                if (NotesStore.note(notification["guid"]) === null) {
+            print("Received PushClient notifications:", notifications.length)
+            for (var i = 0; i < notifications.length; i++) {
+                print("notification", i, ":", notifications[i])
+                var notification = JSON.parse(notifications[i])["payload"];
+
+                if (notification["userId"] != UserStore.userId) { // Yes, we want type coercion here.
+                    console.warn("user mismatch:", notification["userId"], "!=", UserStore.userId)
+                    return;
+                }
+
+                switch(notification["reason"]) {
+                case "update":
+                    print("Note updated on server:", notification["guid"])
+                    if (NotesStore.note(notification["guid"]) === null) {
+                        NotesStore.refreshNotes();
+                    } else {
+                        NotesStore.refreshNoteContent(notification["guid"]);
+                    }
+                    break;
+                case "create":
+                    print("New note appeared on server:", notification["guid"])
                     NotesStore.refreshNotes();
-                } else {
-                    NotesStore.refreshNoteContent(notification["guid"]);
+                    break;
+                case "notebook_update":
+                    NotesStore.refreshNotebooks();
+                    break;
+                default:
+                    console.warn("Unhandled push notification:", notification["reason"])
                 }
-                break;
-            case "create":
-                print("New note appeared on server:", notification["guid"])
-                NotesStore.refreshNotes();
-                break;
-            case "notebook_update":
-                NotesStore.refreshNotebooks();
-                break;
-            default:
-                console.warn("Unhandled push notification:", notification["reason"])
             }
         }
 

=== modified file 'src/app/qml/ui/NoteView.qml'
--- src/app/qml/ui/NoteView.qml	2015-07-25 01:52:22 +0000
+++ src/app/qml/ui/NoteView.qml	2015-09-08 16:22:25 +0000
@@ -39,6 +39,17 @@
         z: 10
     }
 
+    Component.onDestruction: {
+        if (priv.dirty) {
+            NotesStore.saveNote(note.guid);
+        }
+    }
+
+    QtObject {
+        id: priv
+        property bool dirty: false
+    }
+
     WebContext {
         id: webContext
 
@@ -81,7 +92,6 @@
 
         locationBarController {
             height: locationBar.height
-            mode: Oxide.LocationBarController.ModeAuto
         }
 
         property string html: root.note ? note.htmlContent : ""
@@ -111,7 +121,7 @@
                     switch (data.type) {
                     case "checkboxChanged":
                         note.markTodo(data.todoId, data.checked);
-                        NotesStore.saveNote(note.guid);
+                        priv.dirty = true;
                         break;
                     case "attachmentOpened":
                         var filePath = root.note.resource(data.resourceHash).hashedFilePath;

=== modified file 'src/libqtevernote/evernoteconnection.cpp'
--- src/libqtevernote/evernoteconnection.cpp	2015-03-30 19:01:49 +0000
+++ src/libqtevernote/evernoteconnection.cpp	2015-09-08 16:22:25 +0000
@@ -446,6 +446,7 @@
         }
         return;
     }
+
     EvernoteJob *existingJob = findExistingDuplicate(job);
     if (existingJob) {
         qCDebug(dcJobQueue) << "Duplicate job already queued:" << job->toString();
@@ -491,6 +492,20 @@
     }
 }
 
+void EvernoteConnection::enqueueWrite(EvernoteJob *job)
+{
+    if (!isConnected()) {
+        qCWarning(dcJobQueue) << "Not connected to evernote. Can't enqueue job.";
+        job->emitJobDone(ErrorCodeConnectionLost, gettext("Disconnected from Evernote."));
+        job->deleteLater();
+        return;
+    }
+    connect(job, &EvernoteJob::jobFinished, job, &EvernoteJob::deleteLater);
+    connect(job, &EvernoteJob::jobFinished, this, &EvernoteConnection::startNextJob);
+    m_writeJobQueue.append(job);
+    startJobQueue();
+}
+
 bool EvernoteConnection::isConnected() const
 {
     return m_userstoreClient != nullptr &&
@@ -512,7 +527,9 @@
         return;
     }
 
-    if (!m_highPriorityJobQueue.isEmpty()) {
+    if (!m_writeJobQueue.isEmpty()) {
+        m_currentJob = m_writeJobQueue.takeFirst();
+    } else if (!m_highPriorityJobQueue.isEmpty()) {
         m_currentJob = m_highPriorityJobQueue.takeFirst();
     } else if (!m_mediumPriorityJobQueue.isEmpty()){
         m_currentJob = m_mediumPriorityJobQueue.takeFirst();

=== modified file 'src/libqtevernote/evernoteconnection.h'
--- src/libqtevernote/evernoteconnection.h	2015-03-08 21:29:28 +0000
+++ src/libqtevernote/evernoteconnection.h	2015-09-08 16:22:25 +0000
@@ -88,6 +88,9 @@
     //   multiple times.
     void enqueue(EvernoteJob *job);
 
+    // Use this to queue write calls. They won't be deduped and will be ordered
+    void enqueueWrite(EvernoteJob *job);
+
     bool isConnected() const;
 
     QString error() const;
@@ -134,6 +137,7 @@
     QList<EvernoteJob*> m_highPriorityJobQueue;
     QList<EvernoteJob*> m_mediumPriorityJobQueue;
     QList<EvernoteJob*> m_lowPriorityJobQueue;
+    QList<EvernoteJob*> m_writeJobQueue;
     EvernoteJob *m_currentJob;
 
     // Those need to be mutexed

=== modified file 'src/libqtevernote/jobs/savenotejob.cpp'
--- src/libqtevernote/jobs/savenotejob.cpp	2015-03-15 20:00:21 +0000
+++ src/libqtevernote/jobs/savenotejob.cpp	2015-09-08 16:22:25 +0000
@@ -37,7 +37,8 @@
     if (!otherJob) {
         return false;
     }
-    return this->m_note == otherJob->m_note;
+    return this->m_note == otherJob->m_note
+            && this->m_note->updateSequenceNumber() == otherJob->m_note->updateSequenceNumber();
 }
 
 void SaveNoteJob::attachToDuplicate(const EvernoteJob *other)
@@ -46,6 +47,14 @@
     connect(otherJob, &SaveNoteJob::jobDone, this, &SaveNoteJob::jobDone);
 }
 
+QString SaveNoteJob::toString() const
+{
+    return QString("%1, NoteGuid: %2, UpdateSeq: %3")
+            .arg(metaObject()->className())
+            .arg(m_note->guid())
+            .arg(m_note->updateSequenceNumber());
+}
+
 void SaveNoteJob::startJob()
 {
     evernote::edam::Note note;

=== modified file 'src/libqtevernote/jobs/savenotejob.h'
--- src/libqtevernote/jobs/savenotejob.h	2014-10-24 18:45:35 +0000
+++ src/libqtevernote/jobs/savenotejob.h	2015-09-08 16:22:25 +0000
@@ -34,6 +34,8 @@
     virtual bool operator==(const EvernoteJob *other) const override;
     virtual void attachToDuplicate(const EvernoteJob *other) override;
 
+    virtual QString toString() const override;
+
 signals:
     void jobDone(EvernoteConnection::ErrorCode errorCode, const QString &errorMessage, const evernote::edam::Note &note);
 

=== modified file 'src/libqtevernote/notesstore.cpp'
--- src/libqtevernote/notesstore.cpp	2015-06-21 16:28:58 +0000
+++ src/libqtevernote/notesstore.cpp	2015-09-08 16:22:25 +0000
@@ -878,13 +878,17 @@
         qCWarning(dcSync) << "can't find note for this update... ignoring...";
         return;
     }
+    if (note->updateSequenceNumber() > result.updateSequenceNum) {
+        qCWarning(dcSync) << "Local update sequence number higher than remote. Local:" << note->updateSequenceNumber() << "remote:" << result.updateSequenceNum;
+        return;
+    }
 
     QModelIndex noteIndex = index(m_notes.indexOf(note));
     QVector<int> roles;
 
     handleUserError(errorCode);
     if (errorCode != EvernoteConnection::ErrorCodeNoError) {
-        qWarning(dcSync) << "Fetch note job failed:" << errorMessage;
+        qCWarning(dcSync) << "Fetch note job failed:" << errorMessage;
         note->setLoading(false);
         roles << RoleLoading;
         note->setSyncError(true);
@@ -1389,11 +1393,11 @@
             // This note hasn't been created on the server yet... try that first
             CreateNoteJob *job = new CreateNoteJob(note, this);
             connect(job, &CreateNoteJob::jobDone, this, &NotesStore::createNoteJobDone);
-            EvernoteConnection::instance()->enqueue(job);
+            EvernoteConnection::instance()->enqueueWrite(job);
         } else {
             SaveNoteJob *job = new SaveNoteJob(note, this);
             connect(job, &SaveNoteJob::jobDone, this, &NotesStore::saveNoteJobDone);
-            EvernoteConnection::instance()->enqueue(job);
+            EvernoteConnection::instance()->enqueueWrite(job);
         }
     }
 
@@ -1415,24 +1419,19 @@
 
     int idx = m_notes.indexOf(note);
     note->setLoading(false);
+    QModelIndex noteIndex = index(idx);
 
     handleUserError(errorCode);
     if (errorCode != EvernoteConnection::ErrorCodeNoError) {
         qCWarning(dcSync) << "Unhandled error saving note:" << errorCode << "Message:" << errorMessage;
         note->setSyncError(true);
-        emit dataChanged(index(idx), index(idx), QVector<int>() << RoleLoading << RoleSyncError);
+        emit dataChanged(noteIndex, noteIndex, QVector<int>() << RoleLoading << RoleSyncError);
         return;
     }
 
-    note->setUpdateSequenceNumber(result.updateSequenceNum);
     note->setLastSyncedSequenceNumber(result.updateSequenceNum);
-    note->setTitle(QString::fromStdString(result.title));
-    note->setNotebookGuid(QString::fromStdString(result.notebookGuid));
-    note->setUpdated(QDateTime::fromMSecsSinceEpoch(result.updated));
-
     syncToCacheFile(note);
 
-    QModelIndex noteIndex = index(m_notes.indexOf(note));
     emit dataChanged(noteIndex, noteIndex);
     emit noteChanged(note->guid(), note->notebookGuid());
 }