Emergency sync override switch

[robert7]

Currently if the sync fails for what ever reason, even if I know, which note if "offending" there is no workaround for user.I just can't sync and this make the app unusable.

Suggestion: add "Developer mode switch" in Preferences.
This would enable context menu option "Clear dirty flag". Maybe with some warnings etc..

..if you give me some hints how to do it, I can try to implement

example from #196
2018-07-21 13:18:02.415 CEST libquentier/src/synchronization/SynchronizationManager_p.cpp @ 711 [Debug]: SynchronizationManagerPrivate::onSendLocalChangesFailure: Failed to send new and/or modified saved searches to Evernote service, BAD_DATA_FORMAT exception during the attempt to create a saved search, saved search has no query

[d1vanov]

I'll try to give you some hints but it would take me some time to solidify them in my mind and to write them down.

[d1vanov]

Ok, here are some of my thoughts:

1. I think the tab in preferences dialog should be called somewhat like "Troubleshooting tools", without explicit regard to "developer mode". Developer can in fact just open the local storage with SQLite, find the "offending note" or whatever other think blocks the sync and clear the dirty flag via SQL command. That is, however, a little too much for power users who want to workaround the sync issue.
2. In this tab we'll start from putting a single checkbox there: "Allow to clear local modification flag". Somewhere nearby should be a QLabel explaining that "This option can be used to enable the possibility to clear the special flag with which locally modified objects are marked in order to send them to Evernote during sync. Use it with great care and only clear the flag from objects which cannot be synced properly for some reason, otherwise you may lose your local modifications".
3. When this flag is set, it should be put into ApplicationSettings i.e. to be made an account-specific persistent preference.
4. NoteListView should add "clear dirty flag" entry to the note item's context menu. This entry should call NoteModel::setData for 'Dirty` column and this flag set to false.
5. The behavior of NoteModel::setData should be modified to only allow setting the dirty column value to false if that persistent preference is set but not otherwise. In fact, currently there's a test for each model which checks that attempts to set dirty column's value to false fail. Perhaps it would be useful to write a separate new test which
   • checks that without this persistent preference NoteModel works the old way i.e. it's not possible to set the value of dirty column to false
   • after this persistent preference is set it becomes possible
   • after this persistent preference is switched off it becomes impossible again

It seems to be from looking at the sync algorithm's source code that this measure i.e. clearing the dirty flag, should cause the locally modified object be overridden by the remote object during sync. However, it would only be the case if the remote object was updated i.e. Evernote decided to actually send us this object during the sync.

I was thinking about another kind of troubleshooting tool - like "reset the note/notebook/tag/saved search to its current state from Evernote", without any actual sync. However, this might bring in even more problems if we are not careful: for example, newest version of note from Evernote might contain linkage to tags which have not yet been synced to our account. So perhaps these need to be removed. Furthermore, the new version of note can be moved to a notebook which we haven't synced yet. So we need to stick the note within its old notebook (which our version thinks is the note's notebook is) and so on. It appears there can be a lot of cases in which we can screw up the integrity of the account's data in an unrecoverable way so let's not do this for now.

Let me know if you'd need any further guidance.

[robert7]

OK I can implement the version with setting dirty to false and I'll adjust the tests.

It appears there can be a lot of cases in which we can screw up the integrity of the account's data in an unrecoverable way so let's not do this for now.

Yes. I agree. And for beginning the simple version should be sufficient.

[robert7]

I started with this... seems to work as expected (also the test fails as expected (great!)).
In NoteListView.cpp there are few snippets of code which are repeated quite often. After the different POV with access of app settings I'm bit confused, what code style is OK for you.

With current style the context menu callback looks like this:

It is OK to refactor to a "less WET" version?

Proposed version:

[d1vanov]

I guess it is ok to refactor this kind of repeated code but I'd propose to do it a little differently, somewhat like this:

void NoteListView::clearDirtyFlag()
{
    QString localUid = actionDataAsString();
    if (Q_UNLIKELY(localUid.isEmpty())) {
        return;
    }

    const NoteModel * pNoteModel = noteModel();
    if (Q_UNLIKELY(!pNoteModel)) {
        return;
    }

    pNoteModel->clearDirty(localUid);
}

You might argue that these unlikely ifs still somewhat defeat the purpose of the change since they would still be repeated. Well, you can slightly simplify it e.g. using macros:

#define CHECK_NOTE_LOCAL_UI(str) \
    if (Q_UNLIKELY(localUid.isEmpty())) { \
        return; \
    }

#define CHECK_NOTE_MODEL_PTR(pNoteModel) \
    if (Q_UNLIKELY(!pNoteModel)) { \
        return; \
    }

void NoteListView::clearDirtyFlag()
{
    QString localUid = actionDataAsString();
    CHECK_NOTE_LOCAL_UID(localUid)

    const NoteModel * pNoteModel = noteModel();
    CHECK_NOTE_MODEL_PTR(pNoteModel)

    pNoteModel->clearDirty(localUid);
}

If you want to go further, you can simplify these macros with further macros:

#define PREPARE_NOTE_MODEL_FOR_ACTION() \
    QString localUid = actionDataAsString(); \
    CHECK_NOTE_LOCAL_UID(localUid) \
    const NoteModel * pNoteModel = noteModel(); \
    CHECK_NOTE_MODEL_PTR(pNoteModel)

void NoteListView::clearDirtyFlag()
{
    PREPARE_NOTE_MODEL_FOR_ACTION()
    pNoteModel->clearDirty(localUid);
}

However, overuse of macros leads to poorly readable code and sometimes to problems with debugging. So I don't really encourage you to go that deep. There are some snippets like this in the gory guts of libquentier's local storage database handling but I am not proud of these at all.

I don't quite see how would your current proposal deal with any condition which in my N times repeated code leads to returning from the method. You could probably throw an exception if anything goes wrong (I can sense it seems convenient to you from your Java experience) but in Qt using exceptions is generally a bad idea - any uncaught exceptions go to QApplication::notify and good luck trying to debug them then. That is not to mention the lack of complete exception safety i.e. Qt gives no guarantee that stuff within it would keep its integrity if some uncaught exception leaves some slot.

One other thing: Qt generally discourages the use of get prefix for accessory methods. I'm trying to follow this principle although sometimes you can meet getters with get prefix in my code because without it the names of some methods just sound kinda weird. But where it's not the issue, I'd prefer to have a getter without any prefix.

[robert7]

OK, I will read the Qt article you referenced and I will someway handle undefined values (I indented to do it anyway, the proposal was just quick draft version - I forgot there to check the values - you are correct).

And I will someways apply your suggestion; I agree with not overusing macros. I'll send you some preview version in next day.

:) I know how exceptions work in Java, but have only vague info about exceptions in C++; so I read that reference also. ok. thx

[d1vanov]

One adjustment to my suggestion: ideally I'd like actionDataToString method accept the action as its input parameter:

QString localUid = actionDataAsString(qobject_cast<QAction*>(sender()));

but it would once again defeat the purpose of code duplication prevention - this ugly fragment with qobject_cast would be repeated multiple times. So let's not do so. Instead let's just do

QString localUid = actionDataAsString();

but I'd like actionDataAsString method declaration to have a brief Doxygen doc mentioning that this method is intended to be used in slots invoked by view's actions:

/**
 * @brief actionDataAsString - convenience method called in slots invoked by QAction's signals
 * @return string from QAction sender's data
 */
QString actionDataAsString() const;

[robert7]

OK agree.