wikilabs / plugins

TW5 plugins: https://wikilabs.github.io
30 stars 8 forks source link

Relink and Uni-link don't play nice anymore. #46

Open flibbles opened 4 years ago

flibbles commented 4 years ago

Someone submitted a bug report to Relink where attempting to rename causes RSoD. This is caused because I made a change to my prettylink.js module which makes your integration no longer work right. Sorry. This was my mistake.

What I did is break prettylink into more compartmentalized functions in an effort to make Relink more maintainable. Prettylink is definitely more extensible, but the proper way to integrate with it is not

exports.name = "unilink";
exports.relink = require('$:/plugins/flibbles/relink/js/relinkoperations/text/wikitext/prettylink.js').relink;

anymore, since there are multiple methods to scoop up. Instead, use this:

var prettylink = require('$:/plugins/flibbles/relink/js/relinkoperations/text/wikitext/prettylink.js');

function Unilink() {}

Unilink.prototype = prettylink;

module.exports = new Unilink();
module.exports.name = "unilink";

This will make it impervious to any added or removed functions. And should work with all versions of Relink.

It also allows you to alter prettyLink's behavior in more specific ways without rewriting it. ...which I think you want to do. I'm still not 100% on how uni-link works (It looks like it's got a billion different subtle ways to link to the same tiddler), but I think there are some syntaxes unique to uni-link where Relinking would be necessary. Like [[MyTiddler|]], I think. Maybe others.

I'll be perfectly happy to help you on my end if I'm right. I can subdivide prettylink further, and make some of those module-private methods overrideable.

-Flibbles

joshuafontany commented 4 years ago

Testing the proposed fix. Works fine so far, I'll let you know if there are any issues I run across. Thanks @flibbles !

pmario commented 4 years ago

I'll have a look at it soon. thx for the info

pmario commented 4 years ago

@flibbles

I'm not sure, what you changed up to V1.9.2 ... I did work with V1.6.0. But I'm pretty sure that the fix proposed here doesn't work.

eg: [[test|aaa]] ... If you change a tiddler named: aaa to bbb relinking doesn't find the tiddler, that contains the prettylink.


... I'm still not 100% on how uni-link works (It looks like it's got a billion different subtle ways to link to the same tiddler), but I think there are some syntaxes unique to uni-link where Relinking would be necessary. Like [[MyTiddler|]], I think. Maybe others.

unilink is a standard wikirule it uses the same mechanisms. ... But it overrules the prettylink wikirule, which is not executed anymore. It's the same thing as if you did disable the prettylink parsing rule.

relink V1.6.0 did add a .relink() function to every wikirule. So I did the same with $:/plugins/wikilabs/uni-link/relinkoperations/text/wikitext/unilink.js

Every thing worked. Adding the .relink() rule allowed your relink plugin to detect unilinks. The same way as it did with prettylinks. ...

I'll need to have a closer look.

flibbles commented 4 years ago

Huh. You're right. It works again for [[aaa]], but not [[test|aaa]]. I'll look into this, since it was my changes that caused this.

flibbles commented 4 years ago

Actually, are you sure it ever worked? It looks like it works with [[aaa]], but I've gone back to my V1.6.0 build and tried it, and even then, it doesn't work with [[test|aaa]].

You're regexp is not the same as the prettylink regexp. It's

this.matchRegExp = /\[\[(.*?)(?:(\|)(\?)?(.*?))?\]\]/mg;

Which means m = this.matchRegExp(string) gives you:

m[2-4] are only used in [[test|aaa]] instances. And in those cases, it diverges from what prettylink's matchRegExp would be. It's:

And this is the kind of match my prettylink relinker expects. So in [[test|aaa]], it tests m[2] against "aaa", but m[2] is a "|" in this case. It also looks like you're using that elaborated regexp from before you even had Relink integration. It came with your inaugural V1.0.0 release.

I'll see what I can toss together.

flibbles commented 4 years ago

Okay, if the unilink relinkwikitextrule is this:

var prettylink = require('$:/plugins/flibbles/relink/js/relinkoperations/text/wikitext/prettylink.js');

function Unilink() {}

Unilink.prototype = prettylink;

module.exports = new Unilink();
module.exports.name = "unilink";

module.exports.relink = function() {
    // The underlying prettylink rule expects everything after the "|" to be the link. We'll just
    // swap it in from the 4th position before we apply prettylink relinking.
    this.match[2] = this.match[4];
    return prettylink.relink.apply(this, arguments);
};

Then it will deal with [[aaa]] and [[test|aaa]].

However, this still doesn't deal with [[aaa|]], since Relink will always think that's a caption and not a link. You could swap m[2] = m[1] if m[2] === "|" && !m[4], but then Relink will replace the uni-link with [[aaa|aaa]], which technically is correct, although it's not a unilink anymore.

pmario commented 4 years ago

thx for the digging. I'll have a closer look

joshuafontany commented 4 years ago

Oooh, good catch guys. I have used [[alias|?]] a lot, but not [[caption|tiddler]] in my wiki. Still, it has been very useful for me to do data-input on a large foreign-language martial-arts dictionary and keep everything straight, so KUDOS for working so hard to make the plugins play well together.

pmario commented 4 years ago

However, this still doesn't deal with [[aaa|]], since Relink will always think that's a caption and not a link. You could swap m[2] = m[1] if m[2] === "|" && !m[4], but then Relink will replace the uni-link with [[aaa|aaa]], which technically is correct, although it's not a unilink anymore.

I'm not overly happy with the decision to implement the caption and subtitle "substitution". They are global settings, which probably isn't very useful in reality. ... But it is a possibility to "animate" users, to use aliases instead of links.

So the [[aaa|]] construction should be a very rare edge case. .. and I don't have a big problem, if it breaks. ... BUT it would be nice, if relink could print a warning, if it can detect it.

[[aaa|]] is a valid, but rare, link construction in standard wikitext, where the | is simply ignored. ... That's why uni-link can use it, without breaking wikitext backwards compatibility.

The same is true for alias links eg: if the text: ´[[aliasName|?]]´ will be copied to a wiki, that doesn't have uni-link installed, it will create standard prettylinks to a tiddler named: ? ...

Which will cause the user to ask: "What's going on?". Someone will hopefully point them to the "missing" plugin.

@flibbles

So it would be helpful, if relink could recognise links that contain |?, |?t, |?c, |?s, |?xxx, and ignore relinking, because they should use aliases.


I'm also thinking about to be able to use constructions like [[xxx|?<macro-name]] or [[xxx|<macro-name]] or [[xxx|>template]]

Where [[xxx|>template]] would be the same as ´{{xxx||template}}´ BUT xxx would be an alias ... but template would probably be something that needs to be relinked.

pmario commented 4 years ago

@flibbles This fix seems to work: https://github.com/wikilabs/plugins/issues/46#issuecomment-626189663

I'm testing it atm.

flibbles commented 4 years ago

Where [[xxx|>template]] would be the same as ´{{xxx||template}}´ BUT xxx would be an alias ... but template would probably be something that needs to be relinked.

Since your plugin effectively outlaws titles starting with "?", wouldn't {{xxx||?template}} make more sense?

... BUT it would be nice, if relink could print a warning, if it can detect it.

The current Relink can kind of do this. If you have this for your method:

module.exports.relink = function(text_or_tiddler) {
    // Check that this version of Relink supports [object] return.
    if (typeof text_or_tiddler === "string") {
        if (this.match[2] == "|" && !this.match[3] && !this.match[4]) {
            this.parser.pos = this.matchRegExp.lastIndex;
            return { name: "unilink", impossible: true};
        }
    }
    // The underlying prettylink rule expects everything after the "|" to
    // be the link. We'll just swap it in from the 4th position before we
    // apply prettylink relinking.
    this.match[2] = this.match[4];
    return prettylink.relink.apply(this, arguments);
};

Then later versions of relink will issue a warning for `[[aaa|]] that it can't perform a rename. This will be backward compatible with old version of Relink, even if it won't issue warnings with earlier versions.

So it would be helpful, if relink could recognise links that contain...

Unless someone has a tiddler named c or xxx or something like that, those won't ever get picked up. But if you want to handle those edge cases, then probably something like:

module.exports.relink = function(text_or_tiddler) {
    // Check that this version of Relink supports [object] return.
    if (typeof text_or_tiddler === "string") {
        if (this.match[3] == "?") {
            this.parser.pos = this.matchRegExp.lastIndex;
            // Ignore this unilink. The link portion has a "?", so it's an alias.
            return undefined;
        }
        if (this.match[2] == "|" && !this.match[3] && !this.match[4]) {
            this.parser.pos = this.matchRegExp.lastIndex;
            return { name: "unilink", impossible: true};
        }
    }
    // The underlying prettylink rule expects everything after the "|" to
    // be the link. We'll just swap it in from the 4th position before we
    // apply prettylink relinking.
    this.match[2] = this.match[4];
    return prettylink.relink.apply(this, arguments);
};

(I didn't test this last block myself. Just wrote it in github. So make sure you give it a whirl.)

Also, I don't know if you know about Relink References, the TiddlyInfo pane, but if you're interested, it might be possible for Relink to list uni-links there productively.

Relink References

I'd need to test and patch up some aspects of Relink first before I could confidently say you could do that, but if you think it'd be neato to list alias links there, along with handy blurbs like [[alias name|?c]], then I may take a crack at it again once I finish my xml plugin.

pmario commented 4 years ago

I'd need to test and patch up some aspects of Relink first before I could confidently say you could do that, but if you think it'd be neato to list alias links there, along with handy blurbs like [[alias name|?c]], then I may take a crack at it again once I finish my xml plugin.

uni-link has it's own aliasbacklinks filters. The info is shown in the default "References" tab, which is modified by the plugin.

pmario commented 3 years ago

@flibbles ... Thinking about your "relink-info-tab" .. It actually would be nice to see the link-type, that the users use.

eg: [[alias name|?c]] ... BUT you need to be aware, that "alias links" link to completely different titles. ... So you'd probably need to do a feature-check, if the uni-link plugin is installed.

flibbles commented 3 years ago

I am currently in the process of writing Relink V2, which will be separating out reporting from relinking. Before, it would detect references by calling renameTiddler(targetTitle, targetTitle), and it'd see what got "changed" even though everything was the same. It put some limitations on how uni-link could work.

Now there'll be a report method, which detects ALL references in a tiddler, which should make it easy for uni-link to detect links to aliases, even if it's not something that would get changed if the target tiddler were renamed.

I don't know if that answers your question. (I think it doesn't.) But it should make implementing any changes easier. And actually I was going to contact you soon anyway, because uni-link would have needed to elaborate its integration with Relink slightly anyway. Seems like it'll be a good time to do both.

pmario commented 3 years ago

@flibbles We can start a general discussion here: https://github.com/wikilabs/plugins/discussions if you want

pmario commented 3 years ago

I am currently in the process of writing Relink V2,

Cool, Is there some code already?

flibbles commented 3 years ago

Sure. I've pushed what I've got so far to a "V2" branch on Relink's github repository. It's a little messy at the moment, with TODOs all over the place and partially migrated code here and there. The big changes that will impact you are the introduction of the report method to wikitext rules. You can see what I mean here: https://github.com/flibbles/tw5-relink/blob/V2/plugins/relink/js/relinkoperations/text/wikitext/prettylink.js

The PrettyLinkEntry stuff will be deprecated, but I haven't removed any of it yet. What you have for uni-link right now would continue to relink just fine, but its reporting with the relink-tab will be wonky, since it would be inheriting the prettylink report method instead of making its own.

The biggest reason for making this change (besides being a better design) is that it'll allow Relink to integrate with the new indexers to make it WAAAAAY faster than it currently is. Right now, if you look at the relink-tab for 6 or so tiddlers, you'll start noticing big slowdowns all over the place.

flibbles commented 3 years ago

And just to make clear, the code is a WIP right now. For instance, I intend to move that makeLink method into a common utilities library, among other changes.

flibbles commented 3 years ago

(And also I'm pretty sure I'll be switching around arguments in the report's callback argument. All in all, this code is to give an idea, but I wouldn't write anything to integrate with it yet.)

pmario commented 3 years ago

(And also I'm pretty sure I'll be switching around arguments in the report's callback argument. All in all, this code is to give an idea, but I wouldn't write anything to integrate with it yet.)

No problem. I just wanted to know, where I can follow it. Once you go for a public beta, I'd like to know a little bit in advance, so I can change UNI-LINK if needed.

We should also try to establish a possibility to identify each others options. ... I probably have to have some code, that is compatible with relink 1.x and 2.x

flibbles commented 3 years ago

Understood. I am being very careful to maintain backward compatibility as much as possible. Your old code will relink just fine. (In fact, you'll find some of your old code verbatim in the test suite). I'll make sure that what you write should work with all versions of Relink.

flibbles commented 3 years ago

Okay. It's finally done and pushed. the relinkwikitext rule that you have should continue to work with V1, and it'll also work with V2, but in order for your aliases to show up in the relink tiddlerinfo tab in V2, you just need to introduce a report method.

exports.report = function(text, callback, options) {
    // The underlying prettylink rule expects everything after the "|" to
    // be the link. We'll just swap it in from the 4th position before we
    // apply prettylink relinking.
    this.match[2] = this.match[4];
    return prettylink.report.apply(this, arguments);
};

That should do it? I didn't test it though.

You may actually want something more sophisticated. the new report method would allow you to report any aliases, even if they wouldn't be relinked during title changes. Don't know if you're interested in that. I think I remember you mentioning uni-link already had a reference panel or something.

Anyway, it's super late at night where I am. And I am burned out on coding today. If you have any questions, I will be able to help with anything tomorrow.