Modify and improve multi-valued variables implementation #9487
Conversation
✅ Deploy Preview for tiddlywiki-previews ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📊 Build Size Comparison:
|
| Branch | Size |
|---|---|
| Base (master) | 2451.4 KB |
| PR | 2460.3 KB |
Diff: ⬆️ Increase: +8.9 KB
✅ Change Note Status
All change notes are properly formatted and validated!
📝 $:/changenotes/5.4.0/#9487
Type: feature | Category: internal
Release: 5.4.0
TBD
🔗 #9487
👥 Contributors: yaisog
📖 Change Note Guidelines
Change notes help track and communicate changes effectively. See the full documentation for details.
|
Confirmed: yaisog has already signed the Contributor License Agreement (see contributing.md) |
core/modules/parsers/wikiparser/rules/filteredtranscludeblock.js
Outdated
Show resolved
Hide resolved
core/modules/parsers/wikiparser/rules/filteredtranscludeinline.js
Outdated
Show resolved
Hide resolved
yaisog
left a comment
yaisog
left a comment
There was a problem hiding this comment.
I am not happy about this one. There are 2 wikitext syntax variants now, that do the exact same thing.
As I wrote in the PR description (and #9471), I would like to change {{{ filter }}} to return only the first result, to make the syntax consistent. I did not want to implement this without discussion, though.
Also note that I removed the tooltip and CSS feature parsing from the rule as these were never used by the generated ListWidget.
They are currently not used, but there are plans to enhance that functionality in the future. So for consistency reasons this would need to be added to the triple braces That would mean, that there are 2 different syntaxes, that do exactly the same thing, with a minor difference. The existing See this PR: Fix 7701 filtered transclusion fails if "}" follows somewhere in the text #7822 |
That wouldn't get my vote. Tooltips and styles for Filtered Transclusions are super useless. Nobody uses this anyway except for debugging, maybe, because it's so inflexible and just gives you a plain list. The template makes at least some sense and any styles can be put in there. Personally, I would put resources elsewhere and accept the backwards-compatibility break here. I'll keep the bug in mind but won't act on it until some decisions are made. This is a very minor feature of this PR anyway, which is primarily about the
This is exactly the point of this PR: Have two syntaxes that are subtly different, with new features reserved for the new syntax. You could likewise complain in the original PR #8972 implementation that |
|
Thanks @yaisog there is some proximity between the changes here and those in #9055. It might be worth a review to explore whether it would be helpful to merge them, but I suspect that it might be more practical for me to suspend work on #9055 while we get this completed and merged. (The last time I updated that PR from master was a bit of a nightmare due to other changes we've made in the same areas). |
|
I am absolutely concerned. If we really use the same filter syntax |
Your examples are good, because you did choose the right variable names. But I am not happy with the new syntax. (I have lost context somewhere in the middle of #9471 and here. So I am basically unable to use MVVs at all.) If there is some code where MVVs are used, you can not see if they are MVVs or SVVs. So we have to go and search where the variable has been assigned to see what they are. The only way to make it clear is good variable names. But naming variables is hard and most users get it wrong.
IMO That's not a good argument. Our users will use it and imo they will have problems, which causes a lot of maintenance. What do I need to do, If I have a MVV and I only want to assign the first value? |
Wou would use Also note that, like I mentioned in the issue, the documentation does not explicitly state the only the first result will be assigned in the direct or attribute assignment, at least not in the [[Functions]] tiddler. |
Previously, empty filter results were being converted to arrays containing an empty string [""] instead of truly empty arrays []. This change removes the .map() that was adding empty strings and preserves empty arrays as-is, which is correct behavior for multi-valued variables.
This reverts commit ff2ec38.
Unified empty array → empty string conversion for all attribute types Changed string type to return [undefined] instead of [] when attribute value is undefined, making it consistent with missing macro behavior
Modified the getvariable operator to use getVariableInfo() and its resultList property instead of getVariable(). This allows the operator to return all values from multi-valued variables rather than just the first value. Non-existent variables still return "" and empty multi-valued variables are preserved as empty (no output)
These will be replaced by proper unit tests later
The LogWidget now handles mixed variable types more effectively: - Regular variables are displayed in a table with full column width - Multi-valued variables show as placeholders (e.g., "[Array: 3 values (see below)]") in the main table - Each multi-valued variable's values are shown in a separate collapsible console group with a dedicated table This prevents multi-valued variables from creating wide tables with many columns that squeeze regular variables into narrow, hard-to-read spaces.
- Add new ((( ))) syntax as separate parser rules (filteredlisttransclude*)
with simplified parameters (filter and template only)
- Maintain {{{ }}} syntax with full parameters (filter, tooltip, template,
style, classes) for backward compatibility with existing plugins (Relink)
- Fixes parsing of filters with indirect operands like {$:/temp/Tiddler}
The separation into distinct parser rules makes future deprecation of the
{{{ }}} syntax straightforward while ensuring existing functionality remains
intact.
Delegate to the title filter when operands is a multi-value variable for backwards compatibility. This ensures existing multi-value operand usage continues to work while the single-operand behavior is preserved.
|
Thank you @yaisog for your persistence. I'm catching up after the break, if you've time it would be very useful if you could very briefly summarise where you've got to, and any outstanding questions. |
There was a problem hiding this comment.
Thank you @yaisog for your persistence. I'm catching up after the break, if you've time it would be very useful if you could very briefly summarise where you've got to, and any outstanding questions.
Hi @Jermolene, I did have some time to work on this between the holidays, so there were a bunch of commits.
What is currently implemented:
- The
intersection[]operator outputs the intersection of its input and its (MVV) operand. - The
getvariable[]operator was extended to output all of an MVV's values as a list. - The
enlist[]operator accepts MVV operands and falls back totitle[]in that case (this made the transition less error-prone for me, at least) - The
substitute[]operator was extended in #9510 to work with MVV operands. This could be included here. computeAttribute(s)in widget.js was updated heavily- I made separate parser rules for the
((( )))syntax. Otherwise the regex was too unwieldy. Also, this way{{{ }}}remains functionally unchanged (I had problems with Relink when removing the tooltip and classes code). - The syntax
attribute=<<.function>>makes attribute an MVV if the function returns more than one result. This is a backward compatibility break, but very, very useful in my opinion. It seems logical, too. - I found it useful for
<% if %>to make its filter result available as an MVV to replace a lot offormat:titlelist/enlistinstances. So within its scopeconditionListis now available. The ListWidget had to be extended to accept an additional parameterlistVariablewhich is the name of an MVV that contains the complete filter result. - I rewrote the ActionLogWidget for better display of MVVs. In the overview table they are listed with their number of items, and these items are listed for each MVV in a separate table after the overview.
Open questions:
- Do you agree with the above or should anything be changed / reverted? I pretty much went ahead and implemented my ideas as they occured to me, so it boils down to identifying where I overshot the target.
I tried to update the overview tiddler [[Multi-Valued Variables]] as I went along. You can check it out if you need more details than this short summary above.
|
In general I am OK with the additions. The only problem I have is the difference for MVVs in the filter syntax. I think if MVVs in filter runs are indistinguishable from SVVs it will cause problems. -- Just my thoughts. @yaisog ... What was the reasoning bind this change?
|
Pretty much what I wrote in #9471 (comment)
This was inspired by @Jermolene's comment in #9471 (comment)
Mostly, I didn't like that we need another syntax to assign MVVs to attributes, where the natural candidate |
The listVariable was not being properly updated when the filter output changed, particularly when list items were reused during refresh. Changes: - Move listVariable assignment from ListItemWidget to ListWidget for better efficiency (set once instead of per item) - Update listVariable in handleListChanges() before refreshing children to ensure they see the new value - Remove fullList and listVariableName from parseTreeNode since they're no longer needed - Simplify ListItemWidget.refresh() by removing listVariable handling This makes the listVariable feature more efficient and ensures it correctly reflects the current filter results in all cases.
|
Hi @Jermolene, do we also need to adapt |
3 participants
This PR explores some ideas that were discussed in #9471. I propose to keep the non-technical discussion about the feature in that issue.
Specifically, an that is the biggest change, multi-valued variables must be explicitly declared during their assignment by using the
variable=((( filter expression )))syntax, i.e. by using parentheses instead of curly brackets.If so defined, the usual filter syntax
[<variable>]will return a list of all items, while for variables assigned using curly braces nothing changes.Additionally, the TranscludeWidget has been extended to use the
attribute=((( filter expression )))syntax to pass multi-valued variables to procedures and functions where they create multi-valued variables within the scope of the procedure / function.The
:letfilter run prefix will always create multi-valued variables. Within a filter expression, this would in almost all cases be the intended behavior.An
intersection[]filter operator has been added that can take multi-valued variables as a parameter to return the intersection with the input list.The Filtered Transclusion syntax has been extended to also use
((( filter expression )))to output the complete result list. It is currently synonymous with{{{ filter expression }}}in this regard. It is proposed to change the behavior of the latter to only output the first item, to be consistent with the curly brackets behavior elsewhere. This would be a break in backwards compatibility. It should be possible to write a script as part of the PR to find occurences of{{{ filter expression }}}and modify these to keep the output unchanged.I had to remove some tests that were failing because the behavior is intentionally changed.
Note that any filters with the #8972 syntax
[(name)]will cause the same filter parse error messages that they would have in v5.3.8.enlistfilter should work with MVVs as currently implementedgetvariableoperatorDocumentation
Please refer to the tiddler [[Multi-Valued Variables]] in the PR preview for an overview of all features and syntax.
Syntax consistency
It is difficult to find another consistent syntax to distinguish between ordinary variables (SVV) and multi-valued ones (MVV) that hasn't been used in other context and uses ASCII characters only. Also see #9471.
<<name>><<name>><<name>>attribute=<<name>>attribute=<<name>>attribute=<<name>>possibly
attribute=((name))attribute=<<name>>`$(name)$``$(name)$``$(name)$`$name$$(name)$<<__name__>>[<name>][operator<name>][<name>][operator<name>][<name>][operator<name>][(name)][operator(name)]**[<name>][operator<name>]**attribute={{{ filter }}}attribute={{{ filter }}}attribute={{{ filter }}}attribute={{{ filter }}}attribute=((( filter )))transclusion
{{{ filter }}}(proposed)transclusion
{{{ filter }}}{{{ filter }}}((( filter )))`${ filter }$``${ filter }$``${ filter }$`* A useful application would be the extension of the
substitute[]operator to handle multiple inputs and parameters to create a list of unique substitutions at once** Only
title[]andfunction[](+intersection[])