Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] setTiddlerData triggers a changed tiddler event even if no change #8784

Open
ittayd opened this issue Nov 26, 2024 · 3 comments
Open

Comments

@ittayd
Copy link

ittayd commented Nov 26, 2024

Describe the bug

it calls this.addTiddler which in turn calls this.enqueueTiddlerEvent which causes a refresh even if nothing changed.

this can sometimes cause an infinite loop if data is set during a refresh. In particular it happens for the full-text-search plugin as the count of results is refreshed (of course it is bad to set data as part of a filter evaluation)

Expected behavior

the change event should not trigger

To Reproduce

No response

Screenshots

No response

TiddlyWiki Configuration

5.3.5

Additional context

No response

@pmario
Copy link
Member

pmario commented Nov 26, 2024

The problem is backwards compatibility. At the moment the code looks like this:

exports.setTiddlerData = function(title,data,fields,options) {
options = options || {};
var existingTiddler = this.getTiddler(title),
creationFields = options.suppressTimestamp ? {} : this.getCreationFields(),
modificationFields = options.suppressTimestamp ? {} : this.getModificationFields(),
newFields = {
title: title
};
if(existingTiddler && existingTiddler.fields.type === "application/x-tiddler-dictionary") {
newFields.text = $tw.utils.makeTiddlerDictionary(data);
} else {
newFields.type = "application/json";
newFields.text = JSON.stringify(data,null,$tw.config.preferences.jsonSpaces);
}
this.addTiddler(new $tw.Tiddler(creationFields,existingTiddler,fields,newFields,modificationFields));
};

As you found out, .addTiddler is called using new $tw.Tiddler(creationFields,existingTiddler,fields,newFields,modificationFields), which always sets creationFields and modificationFields.

It may be possible to suppress this.addTiddler() if and only if options.suppressTimestamp === true and a temporary tiddler tmpTiddler.isEqual(tiddler, excludeFields) returns true. It needs to be a tiddler, since isEqual() first checks if(!(tiddler instanceof $tw.Tiddler))

See:

$tw.Tiddler.prototype.isEqual = function(tiddler,excludeFields) {

@Jermolene
Copy link
Member

Hi @ittayd the underlying issue is that the core is written on the assumptions that the tiddler store will not be changed during a refresh cycle. Breaking that assumption will cause oddities and bugs that are unpredictable.

Independently of that fact, there is the question of whether wiki.addTiddler() being called without actually changing any values should invoke a special case and not trigger a refresh cycle. I don't see the necessity for that complexity. Checking whether two tiddlers have the same value is a good order of magnitude more time consuming than simple storing it away.

@pmario
Copy link
Member

pmario commented Nov 27, 2024

I also had concerns about the performance implications

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants