Merge lp:~mzanetti/reminders-app/rework-error-handling into lp:reminders-app

[Michael Zanetti]

rework error handling and job queue processing.

This reworks how jobs report their results. EvernoteJob.cpp does now all the error handling and tells the job what result code to emit. Creating a new threaded API call now is a matter of copying an existing job and adjusting 2 lines of code.

Along with that I simplified the usage of the job queue. Just create a new job, call enqueue(job) and connect to the jobDone() signal for obtaining results. No need for taking care about processing the queue any more.

Last but not least the simplified error handling made it easy to add a workaround for the connection drops. EvernoteJob.cpp now gives it a second try in case the connection dropped by reopening the connection and starting the job again. In my tests that successfully recovered from 100% of the drops.

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

PASSED: Continuous integration, rev:14
http://91.189.93.70:8080/job/reminders-app-ci/29/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/309
    SUCCESS: http://91.189.93.70:8080/job/reminders-app-saucy-amd64-ci/29
    SUCCESS: http://91.189.93.70:8080/job/reminders-app-trusty-amd64-ci/29

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

review: Approve (continuous-integration)

[Victor Thompson (vthompson)]

1. In notes.h, I believe you'll want to include QQmlParserStatus in a Q_INTERFACES macro.
2. I see you're reworking some of the connection error exception handling. I get the following core dump every so often, I wonder if it's related to an error that isn't being handled.

terminate called after throwing an instance of 'apache::thrift::transport::TSSLException'
  what(): SSL_write: errno = 110
Aborted (core dumped)

3. Overall the changes to job handling and error handling look good--is there any particular error path that's being fixed that I might be able to try to reproduce? It's a bit hard to try to verify an overall rework of exception handling.

review: Needs Information

[Michael Zanetti (mzanetti)]

> 1. In notes.h, I believe you'll want to include QQmlParserStatus in a
> Q_INTERFACES macro.

Hmm... Why is that?

> 2. I see you're reworking some of the connection error exception handling. I
> get the following core dump every so often, I wonder if it's related to an
> error that isn't being handled.
>
> terminate called after throwing an instance of
> 'apache::thrift::transport::TSSLException'
> what(): SSL_write: errno = 110
> Aborted (core dumped)

Interesting. I intentionally only caught the exceptions I could see myself in order to learn about libthrift's behavior. I haven't ever gotten this one. I'll add catching that one and also all the others that can happen in theory as I've seen enough of libthrift by now.

Still, if you could tell me how to reproduce this I could check if there is something else I'm doing wrong and try to prevent it from happening instead of just catching and ignoring.

>
> 3. Overall the changes to job handling and error handling look good--is there
> any particular error path that's being fixed that I might be able to try to
> reproduce? It's a bit hard to try to verify an overall rework of exception
> handling.

Nope, not really. The main reason for this commit is that I didn't want to add the try {} catch() block around every single call to the evernote SDK. So this now offers one place to catch them all and still pass the error code back to the job in a Qt style manner (As Qt intentionally does not use exceptions by design).

[David Planella (dpm)]

>> 1. In notes.h, I believe you'll want to include QQmlParserStatus in a
>> Q_INTERFACES macro.

> Hmm... Why is that?

I'm not familiar with this, so take this as a pointer rather than an suggestion, but my guess would be as it's recommended at http://qt-project.org/doc/qt-5.0/qtqml/qqmlparserstatus.html

In any case, I don't see this affecting this MP.

[David Planella (dpm)]

I cannot reproduce Victor's core dump after several run tests. Otherwise, LGTM.

In any case, Victor, if you could tell us how to reproduce, we can come back to it.

review: Approve

[Michael Zanetti (mzanetti)]

> >> 1. In notes.h, I believe you'll want to include QQmlParserStatus in a
> >> Q_INTERFACES macro.
>
> > Hmm... Why is that?
>
> I'm not familiar with this, so take this as a pointer rather than an
> suggestion, but my guess would be as it's recommended at http://qt-
> project.org/doc/qt-5.0/qtqml/qqmlparserstatus.html
>
> In any case, I don't see this affecting this MP.

Oh... now I get it... Yeah. in this merge notes.h still inherits QQmlParserStatus. This has been changed in a newer one. If you want me I can add it in here for completeness, but it would be removed again with the next merge. Sorry for the confusion.

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

FAILED: Autolanding.
More details in the following jenkins job:
http://91.189.93.70:8080/job/reminders-app-autolanding/9/
Executed test runs:
    FAILURE: http://91.189.93.70:8080/job/generic-mediumtests-trusty/501/console
    SUCCESS: http://91.189.93.70:8080/job/reminders-app-saucy-amd64-autolanding/9
    SUCCESS: http://91.189.93.70:8080/job/reminders-app-trusty-amd64-autolanding/9

review: Needs Fixing (continuous-integration)

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

review: Approve (continuous-integration)

[Victor Thompson (vthompson)]

I haven't been able to reproduce. To me it seems like the core happened after letting the app idle while I was reviewing the code. So maybe it was related to the connection timing out.

review: Approve

[Michael Zanetti (mzanetti)]

Thanks Victor.

I'll pay extra attention to this.

=== modified file 'src/plugin/Evernote/jobs/createnotejob.cpp'
--- src/plugin/Evernote/jobs/createnotejob.cpp	2013-11-26 17:18:33 +0000
+++ src/plugin/Evernote/jobs/createnotejob.cpp	2013-11-27 21:48:53 +0000
@@ -22,36 +22,30 @@
 
 #include <QDebug>
 
-CreateNoteJob::CreateNoteJob(Note *note, QObject *parent) :
+CreateNoteJob::CreateNoteJob(const QString &title, const QString &notebookGuid, const QString &content, QObject *parent) :
     EvernoteJob(parent),
-    m_note(note)
-{
-}
-
-void CreateNoteJob::run()
-{
-    NotesStore::ErrorCode errorCode = NotesStore::ErrorCodeNoError;
-
-    try {
-        evernote::edam::Note input;
-        input.title = m_note->title().toStdString();
-        input.__isset.title = true;
-        input.notebookGuid = m_note->notebookGuid().toStdString();
-        input.__isset.notebookGuid = true;
-        input.content = m_note->content().toStdString();
-        input.__isset.content = true;
-        input.contentLength = m_note->content().length();
-        input.__isset.contentLength = true;
-
-        evernote::edam::Note result;
-        client()->createNote(result, token().toStdString(), input);
-
-        m_note->setGuid(QString::fromStdString(result.guid));
-
-    } catch(evernote::edam::EDAMUserException e) {
-        errorCode = NotesStore::ErrorCodeUserException;
-    } catch(evernote::edam::EDAMSystemException) {
-        errorCode = NotesStore::ErrorCodeSystemException;
-    }
-    emit resultReady(errorCode, m_note);
+    m_title(title),
+    m_notebookGuid(notebookGuid),
+    m_content(content)
+{
+}
+
+void CreateNoteJob::startJob()
+{
+    evernote::edam::Note input;
+    input.title = m_title.toStdString();
+    input.__isset.title = true;
+    input.notebookGuid = m_notebookGuid.toStdString();
+    input.__isset.notebookGuid = true;
+    input.content = m_content.toStdString();
+    input.__isset.content = true;
+    input.contentLength = m_content.length();
+    input.__isset.contentLength = true;
+
+    client()->createNote(m_resultNote, token().toStdString(), input);
+}
+
+void CreateNoteJob::emitJobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage)
+{
+    emit jobDone(errorCode, errorMessage, m_resultNote);
 }
 
=== modified file 'src/plugin/Evernote/jobs/createnotejob.h'
--- src/plugin/Evernote/jobs/createnotejob.h	2013-11-25 00:49:48 +0000
+++ src/plugin/Evernote/jobs/createnotejob.h	2013-11-27 21:48:53 +0000
@@ -8,15 +8,21 @@
 {
     Q_OBJECT
 public:
-    explicit CreateNoteJob(Note *note, QObject *parent = 0);
-
-    void run();
+    explicit CreateNoteJob(const QString &title, const QString &notebookGuid, const QString &content, QObject *parent = 0);
 
 signals:
-    void resultReady(NotesStore::ErrorCode errorCode, Note *note);
+    void jobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage, evernote::edam::Note note);
+
+protected:
+    void startJob();
+    void emitJobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage);
 
 private:
-    Note *m_note;
+    QString m_title;
+    QString m_notebookGuid;
+    QString m_content;
+
+    evernote::edam::Note m_resultNote;
 };
 
 #endif // CREATENOTEJOB_H
 
=== modified file 'src/plugin/Evernote/jobs/deletenotejob.cpp'
--- src/plugin/Evernote/jobs/deletenotejob.cpp	2013-11-26 17:18:33 +0000
+++ src/plugin/Evernote/jobs/deletenotejob.cpp	2013-11-27 21:48:53 +0000
@@ -26,13 +26,12 @@
 {
 }
 
-void DeleteNoteJob::run()
-{
-    NotesStore::ErrorCode errorCode = NotesStore::ErrorCodeNoError;
-    try {
-        client()->deleteNote(token().toStdString(), m_guid.toStdString());
-    } catch(...) {
-        catchTransportException();
-    }
-    emit resultReady(errorCode, m_guid);
+void DeleteNoteJob::startJob()
+{
+    client()->deleteNote(token().toStdString(), m_guid.toStdString());
+}
+
+void DeleteNoteJob::emitJobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage)
+{
+    emit jobDone(errorCode, errorMessage, m_guid);
 }
 
=== modified file 'src/plugin/Evernote/jobs/deletenotejob.h'
--- src/plugin/Evernote/jobs/deletenotejob.h	2013-11-25 00:49:48 +0000
+++ src/plugin/Evernote/jobs/deletenotejob.h	2013-11-27 21:48:53 +0000
@@ -9,10 +9,12 @@
 public:
     DeleteNoteJob(const QString &guid, QObject *parent = 0);
 
-    void run();
-
 signals:
-    void resultReady(NotesStore::ErrorCode errorCode, const QString &guid);
+    void jobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage, const QString &guid);
+
+protected:
+    void startJob();
+    void emitJobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage);
 
 private:
     QString m_guid;
 
=== modified file 'src/plugin/Evernote/jobs/evernotejob.cpp'
--- src/plugin/Evernote/jobs/evernotejob.cpp	2013-11-26 17:18:33 +0000
+++ src/plugin/Evernote/jobs/evernotejob.cpp	2013-11-27 21:48:53 +0000
@@ -38,11 +38,55 @@
     m_token(NotesStore::instance()->token())
 {
     connect(this, &EvernoteJob::finished, this, &EvernoteJob::deleteLater);
+    connect(this, &EvernoteJob::finished, NotesStore::instance(), &NotesStore::startNextJob);
 }
 
 EvernoteJob::~EvernoteJob()
 {
-
+}
+
+void EvernoteJob::run()
+{
+    if (m_token.isEmpty()) {
+        qWarning() << "No token set. Cannot execute job.";
+        emitJobDone(NotesStore::ErrorCodeUserException, QStringLiteral("No token set."));
+        return;
+    }
+
+    try {
+        startJob();
+
+    } catch (const TTransportException & e) {
+
+        // The connection broke down. libthrift + evernote servers seem to be quite flaky
+        // so lets try to start the job once more.
+        qWarning() << "Got a transport exception:" << e.what() << ". Trying to reestablish connection...";
+        try {
+            NotesStore::instance()->m_httpClient->close();
+            NotesStore::instance()->m_httpClient->open();
+
+            startJob();
+        } catch (const TTransportException &e) {
+            // Giving up... the connection seems to be down for real.
+            qWarning() << "Cannot reestablish connection:" << e.what();
+            emitJobDone(NotesStore::ErrorCodeConnectionLost, e.what());
+        } catch (const evernote::edam::EDAMUserException &e) {
+            emitJobDone(NotesStore::ErrorCodeUserException, e.what());
+        } catch (const evernote::edam::EDAMSystemException &e) {
+            emitJobDone(NotesStore::ErrorCodeSystemException, e.what());
+        } catch (const evernote::edam::EDAMNotFoundException &e) {
+            emitJobDone(NotesStore::ErrorCodeNotFoundExcpetion, e.what());
+        }
+
+    } catch (const evernote::edam::EDAMUserException &e) {
+        emitJobDone(NotesStore::ErrorCodeUserException, e.what());
+    } catch (const evernote::edam::EDAMSystemException &e) {
+        emitJobDone(NotesStore::ErrorCodeSystemException, e.what());
+    } catch (const evernote::edam::EDAMNotFoundException &e) {
+        emitJobDone(NotesStore::ErrorCodeNotFoundExcpetion, e.what());
+    }
+
+    emitJobDone(NotesStore::ErrorCodeNoError, QString());
 }
 
 evernote::edam::NoteStoreClient *EvernoteJob::client()
@@ -54,16 +98,3 @@
 {
     return m_token;
 }
-
-void EvernoteJob::catchTransportException()
-{
-    try {
-        // this function is meant to be called from a catch block
-        // rethrow the exception to catch it again
-        throw;
-    } catch (const TTransportException & e) {
-        qDebug() <<  e.what();
-    } catch (const TException & e) {
-        qDebug() <<  e.what();
-    }
-}
 
=== modified file 'src/plugin/Evernote/jobs/evernotejob.h'
--- src/plugin/Evernote/jobs/evernotejob.h	2013-11-25 00:49:48 +0000
+++ src/plugin/Evernote/jobs/evernotejob.h	2013-11-27 21:48:53 +0000
@@ -10,6 +10,19 @@
 
 #include <QThread>
 
+/* How to create a new Job type:
+ * - Subclass EvernoteJob
+ * - Implement startJob() in which you do the call to evernote.
+ *   - No need to catch exceptions, EvernoteJob will deal with those.
+ * - Define a jobDone() signal with the result parameters you need.
+ *   - Keep the convention of jobDone(NotesStore::ErrorCode errorCode, const QString &message [, ...])
+ * - Emit jobDone() in your implementation of emitJobDone().
+ *   - NOTE: emitJobDone() might be called with an error even before startJob() is triggered.
+ *
+ * Jobs can be enqueue()d in NotesStore.
+ * They will destroy themselves when finished.
+ * The jobqueue will take care about starting them.
+ */
 class EvernoteJob : public QThread
 {
     Q_OBJECT
@@ -17,12 +30,18 @@
     explicit EvernoteJob(QObject *parent = 0);
     virtual ~EvernoteJob();
 
+    void run() final;
+
+signals:
+    void connectionLost(const QString &errorMessage);
+
 protected:
+    virtual void startJob() = 0;
+    virtual void emitJobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage) = 0;
+
     evernote::edam::NoteStoreClient* client();
     QString token();
 
-    void catchTransportException();
-
 private:
     QString m_token;
 };
 
=== modified file 'src/plugin/Evernote/jobs/fetchnotebooksjob.cpp'
--- src/plugin/Evernote/jobs/fetchnotebooksjob.cpp	2013-11-26 17:18:33 +0000
+++ src/plugin/Evernote/jobs/fetchnotebooksjob.cpp	2013-11-27 21:48:53 +0000
@@ -28,16 +28,12 @@
 }
 
 
-void FetchNotebooksJob::run()
-{
-    NotesStore::ErrorCode errorCode = NotesStore::ErrorCodeNoError;
-    std::vector<evernote::edam::Notebook> results;
-    try {
-        client()->listNotebooks(results, token().toStdString());
-    } catch(evernote::edam::EDAMUserException) {
-        errorCode = NotesStore::ErrorCodeUserException;
-    } catch(evernote::edam::EDAMSystemException) {
-        errorCode = NotesStore::ErrorCodeSystemException;
-    }
-    emit resultReady(errorCode, results);
+void FetchNotebooksJob::startJob()
+{
+    client()->listNotebooks(m_results, token().toStdString());
+}
+
+void FetchNotebooksJob::emitJobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage)
+{
+    emit jobDone(errorCode, errorMessage, m_results);
 }
 
=== modified file 'src/plugin/Evernote/jobs/fetchnotebooksjob.h'
--- src/plugin/Evernote/jobs/fetchnotebooksjob.h	2013-11-25 00:49:48 +0000
+++ src/plugin/Evernote/jobs/fetchnotebooksjob.h	2013-11-27 21:48:53 +0000
@@ -9,10 +9,15 @@
 public:
     explicit FetchNotebooksJob(QObject *parent = 0);
 
-    void run();
-
 signals:
-    void resultReady(NotesStore::ErrorCode errorCode, const std::vector<evernote::edam::Notebook> &results);
+    void jobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage, const std::vector<evernote::edam::Notebook> &results);
+
+protected:
+    void startJob();
+    void emitJobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage);
+
+private:
+    std::vector<evernote::edam::Notebook> m_results;
 };
 
 #endif // FETCHNOTEBOOKSJOB_H
 
=== modified file 'src/plugin/Evernote/jobs/fetchnotejob.cpp'
--- src/plugin/Evernote/jobs/fetchnotejob.cpp	2013-11-26 17:18:33 +0000
+++ src/plugin/Evernote/jobs/fetchnotejob.cpp	2013-11-27 21:48:53 +0000
@@ -26,22 +26,12 @@
 {
 }
 
-void FetchNoteJob::run()
+void FetchNoteJob::startJob()
 {
-    NotesStore::ErrorCode errorCode = NotesStore::ErrorCodeNoError;
-    evernote::edam::Note result;
-    try {
-        client()->getNote(result, token().toStdString(), m_guid.toStdString(), true, true, false, false);
-    } catch (evernote::edam::EDAMUserException) {
-        errorCode = NotesStore::ErrorCodeUserException;
-    } catch (evernote::edam::EDAMSystemException) {
-        errorCode = NotesStore::ErrorCodeSystemException;
-    } catch (evernote::edam::EDAMNotFoundException) {
-        errorCode = NotesStore::ErrorCodeNotFoundExcpetion;
-    } catch (...) {
-        catchTransportException();
-        errorCode = NotesStore::ErrorCodeConnectionLost;
-    }
+    client()->getNote(m_result, token().toStdString(), m_guid.toStdString(), true, true, false, false);
+}
 
-    emit resultReady(errorCode, result);
+void FetchNoteJob::emitJobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage)
+{
+    emit resultReady(errorCode, errorMessage, m_result);
 }
 
=== modified file 'src/plugin/Evernote/jobs/fetchnotejob.h'
--- src/plugin/Evernote/jobs/fetchnotejob.h	2013-11-25 00:49:48 +0000
+++ src/plugin/Evernote/jobs/fetchnotejob.h	2013-11-27 21:48:53 +0000
@@ -9,14 +9,20 @@
 public:
     explicit FetchNoteJob(const QString &guid, QObject *parent = 0);
 
-    void run();
 signals:
-    void resultReady(NotesStore::ErrorCode error, const evernote::edam::Note &note);
+    void resultReady(NotesStore::ErrorCode error, const QString &errorMessage, const evernote::edam::Note &note);
+
+protected:
+    void startJob();
+    void emitJobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage);
 
 private:
     evernote::edam::NoteStoreClient *m_client;
     QString m_token;
     QString m_guid;
+
+    evernote::edam::Note m_result;
+
 };
 
 #endif // FETCHNOTEJOB_H
 
=== modified file 'src/plugin/Evernote/jobs/fetchnotesjob.cpp'
--- src/plugin/Evernote/jobs/fetchnotesjob.cpp	2013-11-26 17:18:33 +0000
+++ src/plugin/Evernote/jobs/fetchnotesjob.cpp	2013-11-27 21:48:53 +0000
@@ -30,7 +30,7 @@
 {
 }
 
-void FetchNotesJob::run()
+void FetchNotesJob::startJob()
 {
     // TODO: fix start/end (use smaller chunks and continue fetching if there are more notes available)
     int32_t start = 0;
@@ -48,17 +48,10 @@
     resultSpec.includeTitle = true;
     resultSpec.__isset.includeTitle = true;
 
-    NotesStore::ErrorCode errorCode = NotesStore::ErrorCodeNoError;
-    evernote::edam::NotesMetadataList results;
+    client()->findNotesMetadata(m_results, token().toStdString(), filter, start, end, resultSpec);
+}
 
-    try {
-        client()->findNotesMetadata(results, token().toStdString(), filter, start, end, resultSpec);
-    } catch(evernote::edam::EDAMUserException) {
-        errorCode = NotesStore::ErrorCodeUserException;
-    } catch(evernote::edam::EDAMSystemException) {
-        errorCode = NotesStore::ErrorCodeSystemException;
-    } catch(evernote::edam::EDAMNotFoundException) {
-        errorCode = NotesStore::ErrorCodeNotFoundExcpetion;
-    }
-    emit resultReady(errorCode, results);
+void FetchNotesJob::emitJobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage)
+{
+    emit jobDone(errorCode, errorMessage, m_results);
 }
 
=== modified file 'src/plugin/Evernote/jobs/fetchnotesjob.h'
--- src/plugin/Evernote/jobs/fetchnotesjob.h	2013-11-25 00:49:48 +0000
+++ src/plugin/Evernote/jobs/fetchnotesjob.h	2013-11-27 21:48:53 +0000
@@ -9,13 +9,16 @@
 public:
     explicit FetchNotesJob(const QString &filterNotebookGuid, QObject *parent = 0);
 
-    void run();
-
 signals:
-    void resultReady(NotesStore::ErrorCode errorCode, const evernote::edam::NotesMetadataList &results);
+    void jobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage, const evernote::edam::NotesMetadataList &results);
+
+protected:
+    void startJob();
+    void emitJobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage);
 
 private:
     QString m_filterNotebookGuid;
+    evernote::edam::NotesMetadataList m_results;
 };
 
 #endif // FETCHNOTESJOB_H
 
=== modified file 'src/plugin/Evernote/jobs/savenotejob.cpp'
--- src/plugin/Evernote/jobs/savenotejob.cpp	2013-11-26 17:18:33 +0000
+++ src/plugin/Evernote/jobs/savenotejob.cpp	2013-11-27 21:48:53 +0000
@@ -25,36 +25,30 @@
 
 SaveNoteJob::SaveNoteJob(Note *note, QObject *parent) :
     EvernoteJob(parent),
-    m_note(note)
-{
-}
-
-void SaveNoteJob::run()
-{
-    NotesStore::ErrorCode errorCode = NotesStore::ErrorCodeNoError;
-    try {
-        evernote::edam::Note note;
-        note.guid = m_note->guid().toStdString();
-        note.__isset.guid = true;
-        note.title = m_note->title().toStdString();
-        note.__isset.title = true;
-        note.notebookGuid = m_note->notebookGuid().toStdString();
-        note.__isset.notebookGuid = true;
-        note.content = m_note->content().toStdString();
-        note.__isset.content = true;
-        note.contentLength = m_note->content().length();
-
-        client()->updateNote(note, token().toStdString(), note);
-
-    } catch (evernote::edam::EDAMUserException e) {
-        errorCode = NotesStore::ErrorCodeUserException;
-        qDebug() << QString::fromStdString(e.parameter);
-    } catch (evernote::edam::EDAMSystemException) {
-        errorCode = NotesStore::ErrorCodeSystemException;
-    } catch (...) {
-        catchTransportException();
-        errorCode = NotesStore::ErrorCodeConnectionLost;
-    }
-
-    emit resultReady(errorCode, m_note);
+    m_guid(note->guid()),
+    m_title(note->title()),
+    m_notebookGuid(note->notebookGuid()),
+    m_content(note->content())
+{
+}
+
+void SaveNoteJob::startJob()
+{
+    evernote::edam::Note note;
+    note.guid = m_guid.toStdString();
+    note.__isset.guid = true;
+    note.title = m_title.toStdString();
+    note.__isset.title = true;
+    note.notebookGuid = m_notebookGuid.toStdString();
+    note.__isset.notebookGuid = true;
+    note.content = m_content.toStdString();
+    note.__isset.content = true;
+    note.contentLength = m_content.length();
+
+    client()->updateNote(m_note, token().toStdString(), note);
+}
+
+void SaveNoteJob::emitJobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage)
+{
+    emit jobDone(errorCode, errorMessage, m_note);
 }
 
=== modified file 'src/plugin/Evernote/jobs/savenotejob.h'
--- src/plugin/Evernote/jobs/savenotejob.h	2013-11-25 00:49:48 +0000
+++ src/plugin/Evernote/jobs/savenotejob.h	2013-11-27 21:48:53 +0000
@@ -9,12 +9,20 @@
 public:
     explicit SaveNoteJob(Note *note, QObject *parent = 0);
 
-    void run();
 signals:
-    void resultReady(NotesStore::ErrorCode errorCode, Note *note);
+    void jobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage, const evernote::edam::Note &note);
+
+protected:
+    void startJob();
+    void emitJobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage);
 
 private:
-    Note *m_note;
+    QString m_guid;
+    QString m_title;
+    QString m_notebookGuid;
+    QString m_content;
+
+    evernote::edam::Note m_note;
 };
 
 #endif // SAVENOTEJOB_H
 
=== modified file 'src/plugin/Evernote/note.cpp'
--- src/plugin/Evernote/note.cpp	2013-11-26 17:18:33 +0000
+++ src/plugin/Evernote/note.cpp	2013-11-27 21:48:53 +0000
@@ -35,7 +35,7 @@
 
 void Note::setGuid(const QString &guid)
 {
-    if (m_guid == guid) {
+    if (m_guid != guid) {
         m_guid = guid;
         emit guidChanged();
     }
 
=== modified file 'src/plugin/Evernote/notesstore.cpp'
--- src/plugin/Evernote/notesstore.cpp	2013-11-27 21:48:53 +0000
+++ src/plugin/Evernote/notesstore.cpp	2013-11-27 21:48:53 +0000
@@ -77,12 +77,12 @@
         }
 
         boost::shared_ptr<TBufferedTransport> bufferedTransport(new TBufferedTransport(socket));
-        boost::shared_ptr<THttpClient> userStoreHttpClient (new THttpClient(bufferedTransport,
+        m_httpClient = boost::shared_ptr<THttpClient>(new THttpClient(bufferedTransport,
                                                                             EVERNOTE_HOST.toStdString(),
                                                                             EDAM_USER_STORE_PATH.toStdString()));
-        userStoreHttpClient->open();
+        m_httpClient->open();
 
-        boost::shared_ptr<TProtocol> iprot(new TBinaryProtocol(userStoreHttpClient));
+        boost::shared_ptr<TProtocol> iprot(new TBinaryProtocol(m_httpClient));
         m_client = new evernote::edam::NoteStoreClient(iprot);
 
         qDebug() << "NoteStore client created.";
@@ -153,27 +153,6 @@
     return m_notes.value(guid);
 }
 
-void NotesStore::saveNote(const QString &guid)
-{
-    Note *note = m_notes.value(guid);
-
-    QString enml = Html2EnmlConverter::html2enml(note->content());
-    note->setContent(enml);
-
-    SaveNoteJob *job = new SaveNoteJob(note, this);
-    connect(job, &SaveNoteJob::resultReady, this, &NotesStore::saveNoteJobDone);
-    m_jobQueue.append(job);
-    startJobQueue();
-}
-
-void NotesStore::deleteNote(const QString &guid)
-{
-    DeleteNoteJob *job = new DeleteNoteJob(guid, this);
-    connect(job, &DeleteNoteJob::resultReady, this, &NotesStore::deleteNoteJobDone);
-    m_jobQueue.append(job);
-    startJobQueue();
-}
-
 QList<Notebook *> NotesStore::notebooks() const
 {
     return m_notebooks.values();
@@ -184,6 +163,12 @@
     return m_notebooks.value(guid);
 }
 
+void NotesStore::enqueue(EvernoteJob *job)
+{
+    m_jobQueue.append(job);
+    startJobQueue();
+}
+
 void NotesStore::startJobQueue()
 {
     if (m_jobQueue.isEmpty()) {
@@ -205,23 +190,15 @@
 
 void NotesStore::refreshNotes(const QString &filterNotebookGuid)
 {
-    if (m_token.isEmpty()) {
-        qWarning() << "No token set. Cannot fetch notes.";
-        return;
-    }
-
     FetchNotesJob *job = new FetchNotesJob(filterNotebookGuid);
-    connect(job, &FetchNotesJob::resultReady, this, &NotesStore::fetchNotesJobDone);
-
-    m_jobQueue.append(job);
-    startJobQueue();
+    connect(job, &FetchNotesJob::jobDone, this, &NotesStore::fetchNotesJobDone);
+    enqueue(job);
 }
 
-void NotesStore::fetchNotesJobDone(ErrorCode errorCode, const evernote::edam::NotesMetadataList &results)
+void NotesStore::fetchNotesJobDone(ErrorCode errorCode, const QString &errorMessage, const evernote::edam::NotesMetadataList &results)
 {
     if (errorCode != ErrorCodeNoError) {
-        qWarning() << "Failed to fetch notes list:" << errorCodeToString(errorCode);
-        startNextJob();
+        qWarning() << "Failed to fetch notes list:" << errorMessage;
         return;
     }
 
@@ -240,29 +217,19 @@
             emit noteAdded(note->guid());
         }
     }
-
-    startNextJob();
 }
 
 void NotesStore::refreshNoteContent(const QString &guid)
 {
-    if (m_token.isEmpty()) {
-        qWarning() << "No token set. Cannot fetch note.";
-        return;
-    }
-
     FetchNoteJob *job = new FetchNoteJob(guid, this);
     connect(job, &FetchNoteJob::resultReady, this, &NotesStore::fetchNoteJobDone);
-    m_jobQueue.append(job);
-
-    startJobQueue();
+    enqueue(job);
 }
 
-void NotesStore::fetchNoteJobDone(ErrorCode errorCode, const evernote::edam::Note &result)
+void NotesStore::fetchNoteJobDone(ErrorCode errorCode, const QString &errorMessage, const evernote::edam::Note &result)
 {
     if (errorCode != ErrorCodeNoError) {
-        qWarning() << "Error fetching note:" << errorCode;
-        startNextJob();
+        qWarning() << "Error fetching note:" << errorMessage;
         return;
     }
 
@@ -271,29 +238,19 @@
     note->setTitle(QString::fromStdString(result.title));
     note->setContent(QString::fromStdString(result.content));
     emit noteChanged(note->guid());
-
-    startNextJob();
 }
 
 void NotesStore::refreshNotebooks()
 {
-    if (m_token.isEmpty()) {
-        qWarning() << "No token set. Cannot refresh notebooks.";
-        return;
-    }
-
     FetchNotebooksJob *job = new FetchNotebooksJob();
-    connect(job, &FetchNotebooksJob::resultReady, this, &NotesStore::fetchNotebooksJobDone);
-
-    m_jobQueue.append(job);
-    startJobQueue();
+    connect(job, &FetchNotebooksJob::jobDone, this, &NotesStore::fetchNotebooksJobDone);
+    enqueue(job);
 }
 
-void NotesStore::fetchNotebooksJobDone(ErrorCode errorCode, const std::vector<evernote::edam::Notebook> &results)
+void NotesStore::fetchNotebooksJobDone(ErrorCode errorCode, const QString &errorMessage, const std::vector<evernote::edam::Notebook> &results)
 {
     if (errorCode != ErrorCodeNoError) {
-        qWarning() << "Error fetching notebooks:" << errorCodeToString(errorCode);
-        startNextJob();
+        qWarning() << "Error fetching notebooks:" << errorMessage;
         return;
     }
 
@@ -312,51 +269,72 @@
             qDebug() << "got new notebook" << notebook->guid();
         }
     }
-
-    startNextJob();
 }
 
 void NotesStore::createNote(const QString &title, const QString &notebookGuid, const QString &content)
 {
-    Note *note = new Note();
-    note->setTitle(title);
-    note->setNotebookGuid(notebookGuid);
-    note->setContent(content);
-    CreateNoteJob *job = new CreateNoteJob(note);
-    connect(job, &CreateNoteJob::resultReady, this, &NotesStore::createNoteJobDone);
-
-    m_jobQueue.append(job);
-    startJobQueue();
+    CreateNoteJob *job = new CreateNoteJob(title, notebookGuid, content);
+    connect(job, &CreateNoteJob::jobDone, this, &NotesStore::createNoteJobDone);
+    enqueue(job);
 }
 
-void NotesStore::createNoteJobDone(NotesStore::ErrorCode errorCode, Note *note)
+void NotesStore::createNoteJobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage, const evernote::edam::Note &result)
 {
     if (errorCode != ErrorCodeNoError) {
-        qWarning() << "Error creating note:" << errorCodeToString(errorCode);
-        delete note;
-        startNextJob();
+        qWarning() << "Error creating note:" << errorMessage;
         return;
     }
 
+    Note *note = new Note(QString::fromStdString(result.guid));
+    note->setNotebookGuid(QString::fromStdString(result.notebookGuid));
+    note->setTitle(QString::fromStdString(result.title));
+    note->setContent(QString::fromStdString(result.content));
+
     m_notes.insert(note->guid(), note);
     noteAdded(note->guid());
-    startNextJob();
-}
-
-void NotesStore::saveNoteJobDone(NotesStore::ErrorCode errorCode, Note *note)
-{
-    startNextJob();
-}
-
-void NotesStore::deleteNoteJobDone(NotesStore::ErrorCode errorCode, const QString &guid)
-{
-    if (errorCode != ErrorCodeNoError) {
-        qWarning() << "Cannot delete note:" << errorCodeToString(errorCode);
-        startNextJob();
+}
+
+void NotesStore::saveNote(const QString &guid)
+{
+    Note *note = m_notes.value(guid);
+
+    QString enml = Html2EnmlConverter::html2enml(note->content());
+    note->setContent(enml);
+
+    SaveNoteJob *job = new SaveNoteJob(note, this);
+    connect(job, &SaveNoteJob::jobDone, this, &NotesStore::saveNoteJobDone);
+    enqueue(job);
+}
+
+void NotesStore::saveNoteJobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage, const evernote::edam::Note &result)
+{
+    if (errorCode != ErrorCodeNoError) {
+        qWarning() << "error saving note" << errorMessage;
+        return;
+    }
+
+    Note *note = m_notes.value(QString::fromStdString(result.guid));
+    if (note) {
+        note->setTitle(QString::fromStdString(result.title));
+        note->setNotebookGuid(QString::fromStdString(result.notebookGuid));
+
+        emit noteChanged(note->guid());
+    }
+}
+
+void NotesStore::deleteNote(const QString &guid)
+{
+    DeleteNoteJob *job = new DeleteNoteJob(guid, this);
+    connect(job, &DeleteNoteJob::jobDone, this, &NotesStore::deleteNoteJobDone);
+    enqueue(job);
+}
+
+void NotesStore::deleteNoteJobDone(NotesStore::ErrorCode errorCode, const QString &errorMessage, const QString &guid)
+{
+    if (errorCode != ErrorCodeNoError) {
+        qWarning() << "Cannot delete note:" << errorMessage;
         return;
     }
     emit noteRemoved(guid);
     m_notes.take(guid)->deleteLater();
-    startNextJob();
 }
-
 
=== modified file 'src/plugin/Evernote/notesstore.h'
--- src/plugin/Evernote/notesstore.h	2013-11-25 00:49:48 +0000
+++ src/plugin/Evernote/notesstore.h	2013-11-27 21:48:53 +0000
@@ -1,6 +1,9 @@
 #ifndef NOTESSTORE_H
 #define NOTESSTORE_H
 
+// Thrift
+#include <transport/THttpClient.h>
+
 // Evernote SDK
 #include <NoteStore.h>
 #include <NoteStore_constants.h>
@@ -14,6 +17,8 @@
 class Notebook;
 class Note;
 
+using namespace apache::thrift::transport;
+
 class NotesStore : public QObject
 {
     Q_OBJECT
@@ -63,14 +68,19 @@
     void notebookChanged(const QString &guid);
 
 private slots:
-    void fetchNotesJobDone(ErrorCode errorCode, const evernote::edam::NotesMetadataList &results);
-    void fetchNotebooksJobDone(ErrorCode errorCode, const std::vector<evernote::edam::Notebook> &results);
-    void fetchNoteJobDone(ErrorCode errorCode, const evernote::edam::Note &result);
-    void createNoteJobDone(ErrorCode errorCode, Note *note);
-    void saveNoteJobDone(ErrorCode errorCode, Note *note);
-    void deleteNoteJobDone(ErrorCode errorCode, const QString &guid);
+    void fetchNotesJobDone(ErrorCode errorCode, const QString &errorMessage, const evernote::edam::NotesMetadataList &results);
+    void fetchNotebooksJobDone(ErrorCode errorCode, const QString &errorMessage, const std::vector<evernote::edam::Notebook> &results);
+    void fetchNoteJobDone(ErrorCode errorCode, const QString &errorMessage, const evernote::edam::Note &result);
+    void createNoteJobDone(ErrorCode errorCode, const QString &errorMessage, const evernote::edam::Note &result);
+    void saveNoteJobDone(ErrorCode errorCode, const QString &errorMessage, const evernote::edam::Note &result);
+    void deleteNoteJobDone(ErrorCode errorCode, const QString &errorMessage, const QString &guid);
 
+    // Use this to enqueue a new job. It will automatically start it if there is no other job pending.
+    void enqueue(EvernoteJob *job);
     void startJobQueue();
+
+    // You should not use this. It's called by the job queue.
+    // If you have a new job to run, just enqueue it. The queue will process it eventually.
     void startNextJob();
 
 private:
@@ -78,13 +88,21 @@
     static NotesStore *s_instance;
 
     QString m_token;
-    evernote::edam::NoteStoreClient *m_client;
 
     QHash<QString, Notebook*> m_notebooks;
     QHash<QString, Note*> m_notes;
 
+    // There must be only one job running at a time
+    // Do not start jobs other than with startJobQueue()
     QList<EvernoteJob*> m_jobQueue;
-    QThread *m_currentJob;
+    EvernoteJob *m_currentJob;
+
+    // Those two are accessed from the job thread.
+    // Make sure to not access them while any jobs are running
+    // or we need to mutex them.
+    evernote::edam::NoteStoreClient *m_client;
+    boost::shared_ptr<THttpClient> m_httpClient;
+
 };
 
 #endif // NOTESSTORE_H