[BUG] List template syntax inconsistent about tiddlywiki5

[rmunn]

Related: #7710 (comment)

[pmario]

I can reproduce the problem using prerelease and FF latest on ubuntu 22.04

[Jermolene]

Hi @rmunn I am inclined to think that including <$list-empty> but not <$list-template> should be treated as an error.

I am not in favour of the idea of removing the <$list-empty> widget from the contents of the list widget and using what remains as the template because it is complex and unintuitive, and results in behaviour that is inconsistent with the rest of wikitext.

Instead, perhaps we can explore improving how we report this error. For example, if we find <$list-empty> but not <$list-template> then we might apply a default list template that is something along the lines of "Error: Missing <$list-template>".

[rmunn]

What about <$list-join> once it's added? Should having a <$list-join> force you to use <$list-template>?

I don't think it should. I think you should be able to write this:

<$list filter="[range[3]]" joinWikified="<br>" variable="num">
Number: <<num>>
</$list>

And then decide to change it to this:

<$list filter="[range[3]]" joinWikified="<br>" variable="num">
Number: <<num>>
<$list-join><br>Some <b>complicated</b> HTML here, maybe<br></$list-join>
</$list>

And have it still work. To me, that's far more intuitive than saying "if you use <$list-join> because your joining text has become too complicated to fit nicely in an attribute, suddenly you also have to change how you're specifying the template". And the same goes for <$list-empty>.

The list widget already offers a way to specify the template, and lots of people are relying on it to continue to work. Users are going to expect that adding <$list-empty> will be the equivalent of using emptyMessage, and will not break their existing template. Currently, that's not how it's going to work, and I believe that's a design mistake. (I can't prove that other people will expect that until TW 5.3.2 releases, but by that point it will be too late to change the design mistake).

[Jermolene]

Thanks @rmunn. I agree with your points; the part I don't like is the parse tree modifications necessary to remove the <$list-empty> widget.

Perhaps alternatively we could just define the <$list-*> widgets properly, and make them function as no-ops that don't render anything?

[rmunn]

In #7710 (comment) you expressed concern about parse tree nodes being cached and shared, which is why I made sure not to touch the existing parse tree nodes and instead use a clone-and-mutate approach. (Only if needed, so that existing list widgets which don't use <$list-template> won't be slowed down at all by the need to clone that branch of the tree). So the existing parse tree remains untouched, and the explicit template becomes a copy of the parse tree, with any $list-* subwidgets simply omitted from the tree.

But okay, I'll try another approach where $list-template and $list-empty (and, eventually, $list-join) become real widgets that do nothing and don't render anything. There would then end up being a copy of $list-join and/or $list-empty in every list item, but if they don't render anything and therefore their update function can be empty as well, then that's probably not a big deal in terms of performance. I'll submit that one as a separate PR, leaving #7810 open for now, so that it's easy to compare the two approaches. Then I'll close whichever one you decide not to merge.

[Jermolene]

In #7710 (comment) you expressed concern about parse tree nodes being cached and shared, which is why I made sure not to touch the existing parse tree nodes and instead use a clone-and-mutate approach. (Only if needed, so that existing list widgets which don't use <$list-template> won't be slowed down at all by the need to clone that branch of the tree). So the existing parse tree remains untouched, and the explicit template becomes a copy of the parse tree, with any $list-* subwidgets simply omitted from the tree.

The clone-and-mutate approach has the quite severe disadvantage that it has to be repeated each time a parse tree is rendered, with no obvious route to caching it.

But okay, I'll try another approach where $list-template and $list-empty (and, eventually, $list-join) become real widgets that do nothing and don't render anything. There would then end up being a copy of $list-join and/or $list-empty in every list item, but if they don't render anything and therefore their update function can be empty as well, then that's probably not a big deal in terms of performance. I'll submit that one as a separate PR, leaving #7810 open for now, so that it's easy to compare the two approaches. Then I'll close whichever one you decide not to merge.

Thanks @rmunn that's great, much appreciated.

[rmunn]

The clone-and-mutate approach has the quite severe disadvantage that it has to be repeated each time a parse tree is rendered, with no obvious route to caching it.

I wonder if it really does have to be repeated? I mean, the way the code currently is, yes, it does, because findExplicitTemplates() is called during rendering. But can the parse tree change in between renders? My understanding was that the parse tree would never change, only the values of variables or attributes (if those attributes are filtered values, transclusions, etc.). So perhaps the findExplicitTemplates() call could be moved into the initialise step? Then the clone-and-mutate only needs to be done once per widget. (The code that loads the body template would need to be done at render time, because it needs to check the template attribute, which might be from a filter. But if template is blank, it could then load the already-built render tree without needing to clone-and-mutate a second time).

If I'm wrong in my understanding of the TW internals, and the parse tree can change in between renders, then of course the clone-and-mutate step has to be done each time as well. And anyway, I'm still going to write that second implementation with real widgets, because it probably will be better. (Next week, as I have other commitments this weekend that have to take priority over open-source work for now). But I wanted to double-check my understanding of TW internals with you, the person who knows TW internals better than anyone else. :-) Because if parse trees can change then the approach I was going to take for the <$if>...<$then> widget would have been wrong, too.

[Jermolene]

Hi @rmunn if we wanted to optimise the clone-and-mutate approach I think we'd be better off moving this processing into a new post-processing phase of the parser.

[rmunn]

Working on the "real widgets" implementation now. Just ran into the following issue: what would you expect this to print?

<$list filter=<<filter>>>
  <$list-empty>
    None!
  </$list-empty>
</$list>

Putting on my "naïve user" hat for a moment, I'd expect that to be treated as a list widget with no list template, so the default template is used and the tiddler titles from the filter are displayed as links. (And the ListWidget/WithMissingTemplate test expects the same thing). Thing is, the parse tree that results consists of three children: a text node with whitespace, a $list-empty widget, and another text node with whitespace. (Or, if \whitespace trim is on, a single $list-empty node). Which means that the list widget's body is considered non-empty, and so it ends up rendering nothing but whitespace for each list item.

The way that @Jermolene originally solved this issue was to check if $list-empty was present, and skip using the $list widget's body as template if there was a $list-empty. Which does solve this particular case, but creates the very discrepancy that this bug report is all about.

Which means that to solve this issue, we need to come up with a way to detect the case where the $list body contains nothing but $list-* templates, possibly buried inside <p> elements if the list widget is being rendered in block mode. And the code for that is extremely similar to the clone-and-mutate code I wrote earlier.

I'll submit what I have so far as a draft PR, so that you can see the problem more easily than just with my description.

[rmunn]

#7827 is the draft PR I mentioned, created to show off the issue I mentioned in #7804 (comment) where we end up using the wrong template if $list-empty is present.

[rmunn]

@Jermolene - Going back to my original solution, I've added commit 69b7192 to #7810. That moves the explicit template discovery out of execute() and into initialise(), because it doesn't depend on any attributes, only on the contents of the parse tree. Since the parse tree never changes between re-renders, and if the parse tree does change than a new instance of the list widget is constructed with the new parse tree, this should be perfectly safe, and get rid of the performance concern you had in #7804 (comment). Please let me know if there's a reason this approach won't work.

[rmunn]

@Jermolene - I managed to get the alternate approach (using real list widgets) to work in a simple and sane way, so I've marked #7827 as ready for review. Whether you choose #7810 or #7827 as the one to merge, either way it will unblock the PR for adding a join attribute to lists, which I've been wanting to finish before 5.3.2 is released.

[rmunn]

I decided to go ahead and close #7810 in favor of #7827. @Jermolene, if you actually preferred #7810 after all, I'll be happy to reopen it. But now that #7827 is working, it looks like that's the better approach, because I found a way to avoid the clone-and-mutate step entirely.

[rmunn]

Closing now that #7827 has been merged.