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

[BUG] Global variables set by $set widget don't work #8519

Open
CodaCodr opened this issue Aug 15, 2024 · 11 comments
Open

[BUG] Global variables set by $set widget don't work #8519

CodaCodr opened this issue Aug 15, 2024 · 11 comments

Comments

@CodaCodr
Copy link
Contributor

Describe the bug

See https://talk.tiddlywiki.org/t/best-widget-to-hold-a-static-variable/10412/11

Expected behavior

That the variables are made global as per docs at https://tiddlywiki.com/#SetWidget

To Reproduce

<$set name=r1 value=r1>
  <$set name=r2 value=r2>
    <$set name=r3 value=r3></$set>
  </$set>
</$set>

OR

<$set name=r1 value=r1>
  <$set name=r2 value=r2></$set>
  <$set name=r3 value=r3></$set>
</$set>

Or even... (would be nice)

<$set name=r1 value=r1></$set>
<$set name=r2 value=r2></$set>
<$set name=r3 value=r3></$set>

Screenshots

image

TiddlyWiki Configuration

tiddlywiki.com

Additional context

No response

@pmario
Copy link
Member

pmario commented Aug 15, 2024

If you do it the way it should be done -- it works

title: set-globals
tags: $:/tags/Global
code-body: yes

\define r1() r1
\define r2() r2
\procedure r3() r3
title: test-globals

r1: <<r1>>

r2: <<r2>>

r3: <<r3>>

image

@CodaCodr
Copy link
Contributor Author

You're missing the point. As I laid out in the OP/report, the topic here is SetWidget and its documentation -- I'm not asking for a howto. Even the example given in the docs fails to work:

image

@pmario
Copy link
Member

pmario commented Aug 15, 2024

The last example needs a \whitespace trim. So the docs needs to be updated. eg: https://saqimtiaz.github.io/tw5-docs-pr-maker/#SetWidget

image

@CodaCodr
Copy link
Contributor Author

SetWidget requires strict adherence to undocumented rules regarding whitespace sensitivities?

That's a bug too, IMO.

@CodaCodr
Copy link
Contributor Author

I can't even bring myself to word it: "Oh and by the way, don't indent anything in this example, SetWidgets used to define global variables will break (silently)." 👎

@pmario
Copy link
Member

pmario commented Aug 16, 2024

@Jermolene

I had a closer look at the code. The problem was introduced with commit: 967e2b7 which fixes issue: #7909

So fixing the problem with \procedure definitions we introduced a problem with the import of nested $set widgets.

Since there where no tests for "importing formatted set-widgets" the fix did not throw any errors while testing.
see: #8519 (comment)

@pmario
Copy link
Member

pmario commented Aug 16, 2024

There should be a new test similar to https://github.com/TiddlyWiki/TiddlyWiki5/blob/967e2b7fef0a7f1277e53187a412b6a190e72363/editions/test/tiddlers/tests/data/importvariables/WithSetWidgets2.tid but without \whitespace trim

@Jermolene -- This new test will also need to pass

@pmario
Copy link
Member

pmario commented Aug 16, 2024

The "WithSetWidget2" test should be the test without the whitespace pragma but there seems to be a copy paste error.

title: ImportVariables/WithSetWidgets2
description: Import variables defined with a set widget without whitespace pragma

ImportVariables/WithSetWidgets2 and ImportVariables/WithSetWidgets test tiddlers contain the exact same code

@Jermolene
Copy link
Member

Thanks @CodaCodr @pmario – I think we'll need to fix this with yet another flag for the parser that configures whether the inheritance of whitespace settings is performed.

@pmario
Copy link
Member

pmario commented Aug 21, 2024

I think we'll need to fix this with yet another flag for the parser that configures whether the inheritance of whitespace settings is performed.

I did have a closer look at the $importvariables widget code. I think it has to be fixed there. If there are "text-nodes" that contains whitespace only, they should be ignored.

As soon as there are "real" text nodes or any other "node-type" import is not possible. -- As it is now.

IMO adding a new flag to the parser, without knowing anything about the tiddler to be imported, is not possible.

@Jermolene
Copy link
Member

Hi @pmario the problem here is that importvariables must parse the tiddlers with whitespace trim so that it ignores whitespace between set widgets. However we don't want procedures to inherit the setting in this case unless the tiddler in which they appear has an explicit \whitespace trim. The behaviour in question is in the parser.

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

3 participants