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

Kin filter: Recursively looking for kinship between tiddler titles #3511

Closed
wants to merge 30 commits into from

Conversation

bimlas
Copy link
Contributor

@bimlas bimlas commented Nov 2, 2018

Usage examples:

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:

  • Would you like to use this filter?
    • 5 yes
  • Do you think it should be part of TiddlyWiki itself?
    • 5 yes
  • Which name would be the best to describe the behaviour?
    • 3 kin
    • 1 kindred
    • 1 related

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.

In my way:
<$list filter="[kin::from[Filter Expression]kin::to[Filters]]">...</$list>

Using variables (I'm not sure that this would work):
<$var name="kin-direction" value="from">
  <$list filter="[kin[Filter Expression]]">
    <$var name="kin-direction" value="to">
      <$list filter="[kin[Filters]]">
        ...
      </$list>
    </$var>
  </$list>
</$var>

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?

[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`)
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.
@pmario
Copy link
Contributor

pmario commented Nov 5, 2018

Sorry for my ignorance. ... BUT what is the SINGLE main purpose of this new operator. ... No references to discussions. maximum 3 sentences please!

@bimlas
Copy link
Contributor Author

bimlas commented Nov 5, 2018

Recursively find "direct" (linear) connections between tiddlers (based on a field like "tags").

@bimlas
Copy link
Contributor Author

bimlas commented Nov 5, 2018

It seems that I need to add an easy-to-understand picture to the reference. Maybe this helps to understand:

tree

@twMat
Copy link
Contributor

twMat commented Nov 5, 2018

@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.

@bimlas
Copy link
Contributor Author

bimlas commented Nov 6, 2018

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.

What is the SINGLE main purpose of this new operator?

Let me try to explain again.

  • Finds the "leaves" of the branch of the specified element in a tree-like structure (where the given tag / field item is a leave).
  • Finds the super- and subsets / groups of a mathematical set (where the given tag / field item is a set).
  • Finds the ancestors and successors of a family member.

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.

@Jermolene
Copy link
Owner

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.

@bimlas
Copy link
Contributor Author

bimlas commented Nov 6, 2018

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

  • Display an item at the deepest level that is linked to multiple tags, for example in the demo this means "ToC -> Reference -> Concepts ->Filters" for "Filters" ("ToC -> Reference -> Filters" is in a "higher" level).
  • Based on the above example I can imagine a "dynamic table of contents" where you do not have to look ahead to ToC to know where to place a tiddler, just add all the tags that characterize it (Filters, Example, Syntax) and it "falls into the appropriate compartment" - only the hierarchy of the tags should be established.

@pmario
Copy link
Contributor

pmario commented Nov 6, 2018

@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 ;)

@Jermolene
Copy link
Owner

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).

@bimlas
Copy link
Contributor Author

bimlas commented Nov 6, 2018

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.

https://bimlas.gitlab.io/demo/tw5/kin-filter-toc-demo.html

@pmario
Copy link
Contributor

pmario commented Nov 6, 2018

@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: function uniqueArray(input) { that are already part of the core. Or at least they are very close. So this should be optimized.

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!

Copy link
Owner

@Jermolene Jermolene left a 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.

core/modules/filters/kin.js Outdated Show resolved Hide resolved
core/modules/filters/kin.js Outdated Show resolved Hide resolved
core/modules/filters/kin.js Outdated Show resolved Hide resolved
core/modules/filters/kin.js Outdated Show resolved Hide resolved
core/modules/filters/kin.js Outdated Show resolved Hide resolved
core/modules/filters/kin.js Outdated Show resolved Hide resolved
core/modules/filters/kin.js Outdated Show resolved Hide resolved
core/modules/filters/kin.js Outdated Show resolved Hide resolved
core/modules/filters/kin.js Outdated Show resolved Hide resolved
core/modules/filters/kin.js Outdated Show resolved Hide resolved
@Jermolene
Copy link
Owner

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!

We also did that in #3502; I've left a code review comment to that effect.

@pmario
Copy link
Contributor

pmario commented Nov 6, 2018

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?

@pmario
Copy link
Contributor

pmario commented Nov 6, 2018

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.

@pmario
Copy link
Contributor

pmario commented Nov 6, 2018

Depending on the "settings" it also has some additional possibilities.

  • Finds the super- and subsets / groups of a mathematical set (where the given tag / field item is a set).
  • Finds the ancestors and successors of a family member.

@Jermolene
Copy link
Owner

Hi @pmario

So would it be possible to extract the new logic and make it available for other filters, so we can avoid code duplication?

That's already been done as part of that PR.

@bimlas
Copy link
Contributor Author

bimlas commented Nov 6, 2018

Found a shorter way to build ToC:

<div class="tc-table-of-contents">
<<toc 'Example tree for kin filter' '] -[tag<currentTiddler>tagging[]kin::to[]' >>
</div>

@bimlas
Copy link
Contributor Author

bimlas commented Nov 20, 2018

@Jermolene , if you think that the code is OK and it should be in the core, then merge it please.

@Jermolene
Copy link
Owner

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.

Copy link
Owner

@Jermolene Jermolene left a 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.

core/modules/filters/kin.js Outdated Show resolved Hide resolved
core/modules/filters/kin.js Show resolved Hide resolved
core/modules/wiki.js Show resolved Hide resolved
@bimlas
Copy link
Contributor Author

bimlas commented Nov 20, 2018

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!

@Jermolene
Copy link
Owner

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
@bimlas
Copy link
Contributor Author

bimlas commented Nov 21, 2018

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! 👍

@bimlas
Copy link
Contributor Author

bimlas commented Nov 21, 2018

I've updated the demo so you can try it without building it: https://bimlas.gitlab.io/demo/tw5/kin-filter.html

@BurningTreeC BurningTreeC mentioned this pull request Nov 24, 2018
@Jermolene
Copy link
Owner

Hi @bimlas I'm really sorry about all the confusion about findListingsOfTiddler(). To explain again what I am recommending:

  • That the search logic of your filter be implemented as a new method on the wiki object called "findTiddlersByField"
  • To then consider refactoring "findListingsOfTiddler" to use the new method if it is more efficient than the current implementation

To be clear, there would be no need to change "core/modules/filters/listed.js".

@bimlas
Copy link
Contributor Author

bimlas commented Nov 24, 2018

Sorry, probably because of my weak English knowledge, we do not understand each other so much.

@bimlas: In other words: would you like me to extract the body of findListingsOfTiddler() as a new findTiddlersByField() method, then call the latter from it?
@Jermolene : That's right: it's a two step process: first add the new function, then second refactor the existing function to use it. That way we retain full backwards compatibility
...
@Jermolene : That the search logic of your filter be implemented as a new method on the wiki object called "findTiddlersByField"

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 kin filter to be a method in the wiki.js with the findTiddlersByField() method name, then this method should get all the parameters that are used by kin (field, depth, direction) as this is a recursive function - you want that? So the findListingsOfTiddler() method would call the kin filter limited at depth 1.

@Jermolene
Copy link
Owner

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.

@Jermolene Jermolene added the v5.1.20 Planned for v5.1.20 label Nov 29, 2018
core/modules/wiki.js Outdated Show resolved Hide resolved
@bimlas
Copy link
Contributor Author

bimlas commented Feb 4, 2019

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.

@Jermolene
Copy link
Owner

Hi @bimlas

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.

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).

@bimlas
Copy link
Contributor Author

bimlas commented Feb 6, 2019

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
https://github.com/Jermolene/TiddlyWiki5/pull/3511/files#diff-89406c65507116b5c42accc5f52da5da
https://github.com/Jermolene/TiddlyWiki5/pull/3511/files#diff-4aa6df7cefc567c1f777eacdf3031ec9 (used only by this plugin, but I extracted this method to the plugin itself)

@Jermolene
Copy link
Owner

Hi @bimlas

I wondered if there is anything in the modifications that has to be merged with the master or should I simply close the PR?

We should discard the core changes -- assuming you can make the plugin without them?

@bimlas
Copy link
Contributor Author

bimlas commented Feb 6, 2019

Yes, I can make it, closing the PR.

@bimlas bimlas closed this Feb 6, 2019
@bimlas bimlas deleted the kin-filter branch February 6, 2019 14:30
bimlas added a commit to bimlas/tw5-kin-filter that referenced this pull request Feb 12, 2019
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.

Jermolene/TiddlyWiki5#3511
bimlas added a commit to bimlas/tw5-kin-filter that referenced this pull request Feb 12, 2019
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.

Jermolene/TiddlyWiki5#3511
@bimlas
Copy link
Contributor Author

bimlas commented Feb 14, 2019

Released the plugin: https://bimlas.gitlab.io/tw5-kin-filter/

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

Successfully merging this pull request may close these issues.

None yet

4 participants