danielo515 / tiddlypouch

Other
20 stars 9 forks source link

syncadaptor.getSkinnyTiddlers causes all system tiddlers to reload #66

Closed Arlen22 closed 7 years ago

Arlen22 commented 7 years ago

By Christof Beurger:

I use since some time tiddlywiki with the tiddlypouch plugin (NoteSelf). And I am really excited about the functionality of TW and the posibility to synchronize to a backend database. There is just one drawback which is: while editing text in the tiddler body text field periodically the focus gets stolen and set to the tiddly name field and having marked it. During fluent text typing this leads to the fact that regularily you delete the header and have to restore it and reset the cursor to the end of the tiddler text.

In order to diagnose this I am using Chrome Dev Tools to explore an un-minified version of the code (thanks @danielo515 ).

Arlen22 commented 7 years ago

I am using cloudant.com

$:/plugins/danielo515/tiddlypouch/pouchadaptor.js:158

PouchAdaptor.prototype.getSkinnyTiddlers = function (callback) {
    $TPouch.database.getSkinnyTiddlers()
    .then(callback.bind(null,null))
    .catch(callback);
};

$:/plugins/danielo515/tiddlypouch/DbStore.js:228

return rows.map(function (doc) {
    return doc.doc ? doc.doc : { // if doc is included just retur it or try to make a conversion otherwhise
        // the key is missed! maybe provide a conversion function as parameter?
        _id: doc.id,
        fields: doc.value
    }
})

Here, doc.doc does not exist, so it returns the object. Note there is no _rev field.

$:/plugins/danielo515/tiddlypouch/converters/tiddler:87

db._convertFromCouch = function convertFromCouch(doc) {
    var result = {};
    this.logger && this.logger.debug("Converting from ", doc);
    // Transfer the fields, pulling down the `fields` hashmap
    $tw.utils.each(doc, function (element, field, obj) {
        if (field === "fields") {
            $tw.utils.each(element, function (element, subTitle, obj) {
                result[subTitle] = element;
            });
        } else if (field === "_id" || field === "_rev") {
            /* skip these */
        } else {
            result[field] = doc[field];
        }
    });
    result["revision"] = doc["_rev"];  // <--- this is the line that causes the problem
    //console.log("Conversion result ", result);
    return result;
};

And here it then sets the revision property to the value of the missing _rev property.

Now, here is what is strange. The Cloudant account does show a _rev property. But I think this is access the pouchdb skinny tiddler list, which might operate different. So I think this is where the problem lies. At least, that is how it runs in the step through.

Arlen22 commented 7 years ago

And I found the culprit, thanks to a comment somewhere else in the code.

$:/plugins/danielo515/tiddlypouch/startup/pouch.js:56

$TPouch.database.createIndex('skinny_tiddlers', function (doc) {
    if(doc.fields['plugin-type']){ // skip plugins!
        return;
    }
    var fields = {};
    for (var field in doc.fields) {
        if (['text'].indexOf(field) === -1) {
            fields[field] = doc.fields[field];
        }
    }
    fields.revision = doc._rev; //<---- this is the real culprit!!! 
    fields._rev = doc._rev;       //<---- should be this !!!
    emit(doc._id, fields);
})
Arlen22 commented 7 years ago

I'm not sure, though, what happens if you create the index after the database is already setup.

Arlen22 commented 7 years ago

DbStore:MyNotebook: Index skinny_tiddlers exists already.

So it looks like any database already setup would need some kind of special treatment.

Actually, now that I'm running it through, I'm getting a slightly different result than before. Still checking.

Scratch that, it's back to the original. I had messed with the index.

Arlen22 commented 7 years ago

So I'm thinking that since the index is already created for a lot of people. We should change db._convertFromCouch.

$:/plugins/danielo515/tiddlypouch/converters/tiddler:87

db._convertFromCouch = function convertFromCouch(doc) {
    var result = {};
    this.logger && this.logger.debug("Converting from ", doc);
    // Transfer the fields, pulling down the `fields` hashmap
    $tw.utils.each(doc, function (element, field, obj) {
        if (field === "fields") {
            $tw.utils.each(element, function (element, subTitle, obj) {
                result[subTitle] = element;
            });
        } else if (field === "_id" || field === "_rev") {
            /* skip these */
        } else {
            result[field] = doc[field];
        }
    });
    if(doc["_rev"])   // <-- add this line here
        result["revision"] = doc["_rev"];  // <--- this is the line that causes the problem
    //console.log("Conversion result ", result);
    return result;
};

This would only set the revision tag if _rev exists, otherwise it would just leave it however it is.

Arlen22 commented 7 years ago

I guess that fixes it for me. That would be my recommendation.

Arlen22 commented 7 years ago

I think it should be if(doc["_rev"]) as I wrote above rather than if(!result["revision"]) just in case there is an old revision field saved on the tiddler, but a fresh load brings in a new _rev tag. Just a thought.

danielo515 commented 7 years ago

Hello @Arlen22 ,

Thank you for your detailed walk-trough. It's often easy to loose the focus when there are so many moving parts. Could you please specify why do you think that

   fields.revision = doc._rev; //<---- this is the real culprit!!! 
    fields._rev = doc._rev;       //<---- should be this !!!

As long as I know, tiddlers should have a revision field

danielo515 commented 7 years ago

Oh, I think I understand what you mean, The convertFromCouch function expects the revision field under _rev, because it does not exist it creates an empty field. Is that it?

Topic aside, I recommend you to use the following syntax ```js when posting java-script snippets.

Arlen22 commented 7 years ago

Yes, that is correct. However the index is already created, so I thought it would be better to just change convertFromCouch.

danielo515 commented 7 years ago

Dear @Arlen22 , let's focus on the best solution. I faced so many times this "index already exists" problem that I developed a update process. Is the TPouch version registered on the database is older, then the update process kicks in, deleting all the indexes and creating new ones.

danielo515 commented 7 years ago

I can see that my main problem was that the getTiddlers function is not as widely used as I thought it would be. The skinny_tiddlers index tries to be as independent as possible, not requiring any conversion. That feature that I thought was a good thing has ended being the actual problem. convertFromCouch is a widely used function, I would prefer to only change it to make it more robust and better, not to just add a workaround

Arlen22 commented 7 years ago

Ok, well then that's great. I didn't know how easy it would be to update that. But it sounds like you understand the problem, and you know the code better than I do :)

danielo515 commented 7 years ago

Dear @Arlen22 , After checking your solution in more detail I think it makes the convertFromCouch function better and more robust, so I think I will implement it. At the end, taking a field for granted is usually a bad idea.

danielo515 commented 7 years ago

Dear @Arlen22 ,

Thank you very much for your help on this matter. The new version is online already and I'm glad to announce that the problem no longer happens. Seems that the revision field is more important than I though. Tiddlywiki relies on it to determine if a tiddler should be loaded again or not. For that reason, it was loading all the tiddlers over and over again. The issue with the focus being lost is still a mistery to me. Maybe @jermolene could explain it better to us.

Thank you , thank you!