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

Include link text for links retrieved with getTiddlerLinks() #1749

Closed
felixhayashi opened this issue May 20, 2015 · 13 comments
Closed

Include link text for links retrieved with getTiddlerLinks() #1749

felixhayashi opened this issue May 20, 2015 · 13 comments

Comments

@felixhayashi
Copy link
Contributor

Maybe it makes sense, if not only the linked tiddlers are retrieved when using $tw.wiki.getTiddlerLinks and $tw.wiki.getTiddlerBacklinks` but also any alias the user defined in relation with the link ("[[alias|SomeTiddlerTitle]]").

I have to admit, I don't know how to deal with situations where two aliases point to the same tiddler...

Please note (2015-12-22): This issue is originally about making the link alias available (to machines). At some point in the discussion, we instead began talking more about semantic annotations, which is a different topic: #2144.

@felixhayashi felixhayashi changed the title API: Include alias for links retrieved with $tw.wiki.getTiddlerLinks(title) API: Include alias for links retrieved with $tw.wiki.getTiddlerLinks May 20, 2015
@felixhayashi felixhayashi changed the title API: Include alias for links retrieved with $tw.wiki.getTiddlerLinks API: Include alias for links retrieved with wiki.getTiddlerLinks May 20, 2015
@felixhayashi felixhayashi changed the title API: Include alias for links retrieved with wiki.getTiddlerLinks API: Include alias for links retrieved with getTiddlerLinks() May 20, 2015
@Jermolene Jermolene changed the title API: Include alias for links retrieved with getTiddlerLinks() Include link text for links retrieved with getTiddlerLinks() May 23, 2015
@Jermolene
Copy link
Owner

Hi @felixhayashi the trouble is that computing the link text is actually a complex operation compared to computing link targets. Consider something like this:

<$link to="HelloThere">
This is
<strong>
a
</strong>
link
</$link>

To compute the link text requires walking through the parse tree nodes and concatenating the text. getTiddlerLinks() is highly performance sensitive, because it's called many times to compute missing and orphan links, so we don't want to make it any slower.

It would make more sense to make a new method that did the additional processing. You could look at the existing getTiddlerLinks() code as a starting point.

@felixhayashi
Copy link
Contributor Author

Hi Jeremy

thanks for responding

To compute the link text requires walking through the parse tree nodes and concatenating the text

No, there exist native properties that just return the combined text without markup:

The Node.textContent property represents the text content of a node and its descendants.
https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent

For compatibility reasons:

var text = el.innerText || el.textContent;

My hotzone plugin uses this property to quickly extract the title from the tiddler frame:

/**
 * Extracts the tiddler title from dom elements holding it.
 * 
 * @param {Element} target - The tiddler frame element.
 * @return {string} The title or undefined
 */
var extractTitleFromFrame = function(target) {

  if(!(target instanceof Element)) return;
  if(!$tw.utils.hasClass(target, config.classNames.tiddlerFrame)) return;

  var el = target.getElementsByClassName(config.classNames.tiddlerTitle)[0];
  if(el) {
    var title = el.innerText || el.textContent;
    return title.trim();
  }

};

-Felix

@Jermolene
Copy link
Owner

No, there exist native properties that just return the combined text without markup:

That's a DOM function. Here we're dealing with a parse tree.

@felixhayashi
Copy link
Contributor Author

That's a DOM function. Here we're dealing with a parse tree.

Ehm ok. Then this would really be resource intensive I guess.

because it's called many times to compute missing and orphan links, so we don't want to make it any slower.

I agree that performance is a top prio here. Should I leave this ticket open anyways, so it may be picked up in the future by anyone? It is clearly not a must-have for me but would be a nice-to-have (for tiddlymap it would make sense in some occasions to be able to directly infer the edge name from the link alias).

-Felix

@Jermolene
Copy link
Owner

@felixhayashi we would definitely never change getTiddlerLinks() to incorporate this suggestion because of the performance impact. The only change I can envisage making to deal with the original request is to add helper functions to make it easier to perform tasks like this directly on the widget tree.

@tobibeer
Copy link
Contributor

Is there anything left to be done about this?

@felixhayashi
Copy link
Contributor Author

Nope

@ybabel
Copy link

ybabel commented Sep 19, 2015

@Jermolene does recent comment's change your minds ?
felixhayashi/TW5-TiddlyMap#119

@Jermolene
Copy link
Owner

Hi @ybabel I'm not against helping @felixhayashi fix that issue, but the proposal here is not a viable solution; we can't make such a deep change to the existing function wiki.getTiddlerLinks().

@ybabel
Copy link

ybabel commented Sep 19, 2015

thanks for the quick response.
I think it's more about adding a getTiddlerLinksAndType(tiddler) than changing the existing wiki.getTiddlerLinks()
We where talking about adding a new parser rule like https://github.com/Jermolene/TiddlyWiki5/blob/f8027a37080c06bc4ee02295a6dbe4a997417b2b/core/modules/parsers/wikiparser/rules/prettylink.js but with extended regex to match something like [[pretty-title|tiddler-title]](qualifier|qualifier|qualifier) or [[Displayed Link Title|Tiddler Title]]->tmap:"demo:uses"
I tried to looked how to do that myself but it seems out of my coding skills right now.

@felixhayashi
Copy link
Contributor Author

Hi, I created a general issue for this topic: #2144
I'll also reopen this one.

-Felix

@felixhayashi felixhayashi reopened this Dec 22, 2015
@Jermolene
Copy link
Owner

I think @felixhayashi this one should stay closed. The OP makes a very specific proposal which has been categorically rejected. #2144 fixes the same underlying problem, but it doesn't really have a direct relationship with the OP here.

@felixhayashi
Copy link
Contributor Author

Yes, I am also much in favour of #2144 but wasn't sure whether this one here was rejected. Now that you make it clear, I'll close it.

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

4 participants