-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Kin filter: Recursively looking for kinship between tiddler titles #3511
Conversation
[kindred:<direction>[<field>]] <direction>: up | down | both <field>: name of the field
I fetched the tiddler, but not used it, thus I removed this to speed up the things.
In the "header" there is a line global $tw: false Since it's declared, I think I should use `options.wiki` instead of `$tw.wiki`.
I misunderstood the behaviour: I thought that I need to collect tiddlers (initialize a list) and return with them to let the `+[...]` filter expression collect the elements common with the previous expression, but instead of this I have to filter the input tiddlers by checking that it's a member of the family or not. The new syntax is: [kindred:<field>[<tiddler_from_family>]] [kindredup:<field>[<tiddler_from_family>]] [kindreddown:<field>[<tiddler_from_family>]] Because of this filter behaviour, the "base tiddler" (passed as operand) has to be included in to the output list (I excluded it earlier). WARNING: Commit introducing TODOs
For example: [[Drag and Drop]kindred[]] [[Drag and Drop]] [[DragAndDropMechanism]] +[kindred[]] WARNING: Commit introducing TODOs
For example the usage of `tags` and `list` fields are controversal: `tags` points to parents while `list` points to children, thus `up`/`down` has no sense. Use `field:direction` syntax for suffix (`[kindred:tags:to[Something]]`) instead of defining multiple operators (`kindred/kindredup/kindreddown`)
People voted this name at https://goo.gl/forms/bywjS7UiuaosfRlR2
If there is a cyclical family tree (the "bottom" (C) element is tagging the "top" (A)) and a branch is derived from one of the members (B), then the branch (B2) is not always found in the filter. A |\ | B |/ \ C B2 During the crawling of the tree, it first adds all the elements to the list, but does not scan them in the other direction: if it finds a that has been checked already, it will be skipped to prevent endless loops. To find all the elements, but avoid endless loops, I'll pick up the results in two separate lists and then combine them. WARNING: Commit introducing TODOs
Examples also contains an other branch of "Filters" to make it clear.
Sorry for my ignorance. ... BUT what is the SINGLE main purpose of this new operator. ... No references to discussions. maximum 3 sentences please! |
Recursively find "direct" (linear) connections between tiddlers (based on a field like "tags"). |
@bimlas - first, let me say you've done a wonderful job in presenting this. Docs and all. IMO, this is a filter operator that outputs as if it were a widget or a macro. It kind of feels unlikely that it would be used as a filter operator in the usual sense, e.g where succeeding operators do something with its output. Or maybe my imagination is too limited? The key question is: Is it justified to ship this to everyone who downloads a TW? Will people likely have use of it? IMO: no (...but, admittedly, I'd say the same about a few other things that are in the standard distro). One idea might be to re-shape it into e.g a macro or so but I'm not sure exactly what it should do. Or could it, or part of it, be made to extend the existing ToC functionality somehow? I have a feeling I will use this some day and I hope it'll be easily found when I will need it one day. Big thanks for making it. |
It does not seem clear what the filter is doing. This is a general filter, I use ToC only to visualize the results, only a list of titles that match the condition appears at the output of it (as with any other filter), you can try with Advanced Search.
Let me try to explain again.
So, it finds out where a given item originates and what other elements originate from it. It can be used in a variety of ways, thus I've put some examples in the opening comment. |
Hi @bimlas apologies for the brief interjection, but I do wonder if this new operator might make it possible to re-implement the TOC macros more efficiently/readably. If so, then I think it's a good candidate for the core. If not, then perhaps it's better to keep it as a plugin while users experiment with it and assess where it might be most useful. |
Hi @Jermolene ! What do you mean by "more efficiently / readably"? I do not know the exact way, but the filter makes a macro able to
|
@bimlas Thank you very much for 3 point explanation and your great contribution!! IMO this info should be the "short description" for the docs. It makes things much clearer, without the need for heavy testing ;) |
Hi @bimlas apologies I don't think I had grasped what you're trying to do with this operator, but I think it's clearer now. The usual determination for whether something should go into the core centres on whether it is universally useful. That's sometimes a tricky debate because it leaves room for matters of opinion. But, if we were able to use this new filter to improve an existing core feature then the debate would be much easier. In the meantime, I noticed a few coding style issues and will leave some comments. (Our coding standards are not well documented -- see https://tiddlywiki.com/dev/#TiddlyWiki%20Coding%20Style%20Guidelines). |
I just created a demo for ToC: It's removing the redundant elements, puting them on the "deepest" level. I have to work on it to remove the duplicated items ("String in music/computer science"). See the tags, it's just a draft, thus I did not documented it well, but I hope it's clear. |
@Jermolene I think it should be part of the core. ... but imo it isn't ready for prime time yet. ... I did review the code a little bit. There are some internal functions eg: The documentation needs to start with "simpler" more basic examples. The existing ones are sophisticated but too complicated for new users. We should play a little bit more with a plugin and then see how it goes. I see it more as an "analysing tool" for the internal document structure. It also extends the possibility to have more then 1 suffix parameter. We need to check if the mechanism is generic enough, that we can use it for future filters, or extend it even more ... I'm not sure here! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @bimlas lots of coding style points I'm afraid, apologies for that.
We also did that in #3502; I've left a code review comment to that effect. |
So would it be possible to extract the new logic and make it available for other filters, so we can avoid code duplication? |
Just some thoughts. The existing TOC builds the structure with wikitext, starting from the "root" tag down to the leafs. This mechanism has the problem, that tiddlers with more than 1 tag show up in the TOC at every "level". The kin filter internally goes down to the leafs and lets us build a "less" cluttered TOC. |
Depending on the "settings" it also has some additional possibilities.
|
Hi @pmario
That's already been done as part of that PR. |
Found a shorter way to build ToC:
|
@Jermolene , if you think that the code is OK and it should be in the core, then merge it please. |
Hi @bimlas apologies, I've not had a chance to properly review this PR yet (it takes a long time to properly review changes like this I'm afraid). I'll leave a couple of notes now for some issues I spotted on a brief glance. Part of the bigger review it needs is deciding whether this should go into the core, I'm open minded at the moment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bimlas this is a high quality PR, and I appreciate the care you've taken with the documentation particularly.
I apologize for sending you such messages, I do not want to bother you, I just do not know why you do not merging the finished PRs; it seems like you just forget about them. By adding a label to it, I think you might be saying that PR is not forgotten, it just "waiting for review". I understand you have to think and decide things; if the filter does not get into the core, I do not mind, you know better than me. Ultimately, thanks to your choices and knowledge, this program is stable for 14 years. Thank You! |
Hi @bimlas gosh sorry if I sounded brusque, not intended. And please don't worry about reminding me of things that I may have forgotten -- in fact, I think I rely on people doing that :) |
Earlier I dropped `findListingsOfTiddler()` (d6ae19b), but it broke the backward compatibility.
To check the performance: * Enable $:/config/Performance/Instrumentation * Open only [[kin Operator (Examples)]] (close others) * Open the browser's console * Press "Show tree" button below these examples [kin::from[Filter Expression]kin::to[Filters]] Without cache: performance: mainRefresh: 1359.20ms performance: +filter: 1286.80ms With cache: performance: mainRefresh: 161.00ms performance: +filter: 97.30ms [kin:list:to[Filter Syntax]] Without cache: performance: mainRefresh: 222.40ms performance: +filter: 153.50ms With cache: performance: mainRefresh: 98.30ms performance: +filter: 38.20ms
Using the cache improved the performance very much: responds much better than before - you can find the concrete values in the commit message. Although the filter is very useful, but I was afraid that because of its sluggishness it will not be really useful, but following your advice, it is resolved. Thank you very much! 👍 |
I've updated the demo so you can try it without building it: https://bimlas.gitlab.io/demo/tw5/kin-filter.html |
Hi @bimlas I'm really sorry about all the confusion about
To be clear, there would be no need to change "core/modules/filters/listed.js". |
Sorry, probably because of my weak English knowledge, we do not understand each other so much.
I really do not understand what you want, I'm sorry. 😞 Perhaps the simplest thing would be to do it by yourself. :( If you want the search mechanism of |
Hi @bimlas not to worry, I'll pick this up after we get v5.1.18 released. Please don't hesitate to remind me if necessary. |
I've tried the filter in the wild and indeed it is very useful, but I'm not sure it can make the core better. The plugin I use to: https://bimlas.gitlab.io/tw5-locator/ I use the filter to list tiddlers associated with the given tags at any depth and to avoid duplication in ToC (filtering out "grandchildren", as I did this earlier in this thread: #3511 (comment)). Since it is unlikely that this kind of table of contents will get into the core, I think we would only use it to filter out duplications from the classical ToC, which is slow. Here is a copy of tw.com with the plugin mentioned above: "With duplications" sidebar tab does not filter duplications, "Without duplications" does. For example navigate to Reference, switch between these tabs and see the results (performance instrumentation enabled) Do you think I should implement a separate plugin? If so, I leave only those parts of the PR that affect the core. If you see other benefits to the filter, I'd be happy if it would be in the core. |
Hi @bimlas
I think it does make sense to go ahead and make it a plugin, that's probably the best way to find out how useful it is to other people. Your experience with performance matches my concerns about the inherent slowness of the filter (but of course we've already got slow filters, like missing and orphans). |
While converting the PR to a plugin, I wondered if there is anything in the modifications that has to be merged with the master or should I simply close the PR? https://github.com/Jermolene/TiddlyWiki5/pull/3511/files#diff-3378be2ff14e6ff5521c34b06144efeb |
Hi @bimlas
We should discard the core changes -- assuming you can make the plugin without them? |
Yes, I can make it, closing the PR. |
Previously a pull request was opened to merge this filter to the core, but eventually it became a separate plugin, so I had to use the already available methods of Tiddly. TiddlyWiki/TiddlyWiki5#3511
Previously a pull request was opened to merge this filter to the core, but eventually it became a separate plugin, so I had to reorganize directory structure. NOTE: Examples not work because the tiddlers missing. TiddlyWiki/TiddlyWiki5#3511
Released the plugin: https://bimlas.gitlab.io/tw5-kin-filter/ |
Usage examples:
(uses an older name:
kindred[]
=kin[]
;kindredup[]
=kin::to[]
;kindreddown[]
=kin::from[]
)Discussion: https://groups.google.com/forum/#!topic/tiddlywiki/YZlPGP0qX1o
Demo: https://bimlas.gitlab.io/demo/tw5/kin-filter.html
Voting: https://goo.gl/forms/bywjS7UiuaosfRlR2
Current results:
I know the suffix is not used in the usual way, but I think it's the most efficient way to pass certain parameters to the filter. For example, if you passing
from
/to
as variables, you would not be able to crawl the tree in both directions in a single phrase, you should be writing two separate queries.Since I want to add more options ("depth", "field contains a single title instead of a list"), so I understand that the suffix can not be expanded indefinitely, thus the variables will be needed: the options used by all filters ("field", "field contains a single title instead of a list") could be passed as variables, but for passing options applicable to each filter individuallys, I think the suffix would be the best.
What do you think? Can I use the suffix in this form? If not, what are your suggestions for passing the various options?