Add actions parameter to radio-widget

[pmario]

For a new plugin of mine I need a functionality like this, because I can't use checkboxes.

The checkbox-widget has 3 possibilities to activate custom actions. .... The radio-widget has none.

This PR allows us to do add an actions=<<xxx>> parameter to the radio button. It also builds a hashmap of all parameters, that don't start with a $ ... Similar to the actions-xx widgets. At the moment those params are available as "local" variables in the macro. See example.

Latest test-version see: #5026 (comment))

\define radioActions()
<$action-sendmessage $message="tm-notify" $param="test-radio-template" actions=<<__actions__>> tiddler=<<__tiddler__>> value=<<__value__>> name=<<__name__>> a=<<__a__>> b=<<__b__>> field=<<__field__>> />
\end

<$tiddler tiddler="$:/temp/test">
	<$radio field="id" value="a" name="/name/a" a=x actions=<<radioActions>> > a </$radio>|
	<$radio field="id" value="b" name="/name/b" b=y actions=<<radioActions>> > b </$radio>|
	<$radio field="id" value="c" name="/name/c" actions=<<radioActions>> > c </$radio>|
</$tiddler>

<$radio field="id" value="d"  actions=<<radioActions>> > d </$radio>|
<$radio field="id" value="e" actions=<<radioActions>> > e </$radio>|

@saqimtiaz @BurningTreeC @flibbles @Jermolene What do you think.

[Jermolene]

Apologies @pmario I've now looked at the code, and of course you're already doing exactly what I suggested.

This is actually a good solution to the challenge of using the same action string with a bunch of radio buttons.

I'd strongly suggest adding the prefix attr- to the variables that are automatically converted from attributes.

[saqimtiaz]

@Jermolene I also think that at the most, we need to only set variables that arise from attributes the widget already supports, or attributes with a given prefix. And then only if we can consistently do so for other input widgets as well.

The catch-all approach of assigning any parameter passed as a variable will lead to problems down the line.

If we need to extend a widget in the future with an extra parameter, we are looking at a potential clash where users may have used that parameter name just to set up some variables. Let's consider the disabled parameter we added recently. With these changes in place we would not be able to, as the parameter name may have been in use for other things. Prefixing future parameter names with $ isn't a good option as we have not done so thus far and would lead to inconsistency and confusion for users.

[Jermolene]

Hi @saqimtiaz @pmario

I was suggesting that the attr- prefix be added to the variable names that are created from attributes.

I think in this case it might make sense to just pass all the widget attributes because any of them (including user defined ones) might be useful in the actions handler.

[pmario]

Latest test version: radio-actions.zip

• Attributes are now prefixed with attr- as @Jermolene suggested
• refresh should now be handled if actions parameter changes

[pmario]

Docs will follow if merged.

[saqimtiaz]

Maybe I am missing something, but wouldn't it be useful to pass the selected value as a variable to the actions as well?

Droppable and Draggable widgets use the term actionTiddler, perhaps we could use that name, or call it actionValue? Regardless of the exact name it would be good for it be consistent across input widgets.

[pmario]

Maybe I am missing something, but wouldn't it be useful to pass the selected value as a variable to the actions as well?

It's there. It should be named "value" ... The 4th parameter in the notification.

[pmario]

Droppable and Draggable widgets use the term actionTiddler, perhaps we could use that name, or call it actionValue? Regardless of the exact name it would be good for it be consistent across input widgets.

The radioWidget has 2 prameters: tiddler and field ... Both of them are available as attr-tiddler and attr-field in the actions macro.

I think it would be confusing, if we would duplicate them.

If you need other parameters, you can add them. Every additional param will be prefixed with attr- and is available to the actions.

[saqimtiaz]

@pmario I am very happy with the prefixing of the variables created with "attr-". It addresses one of my key concerns, namely variables declared by the user elsewhere getting overwritten and potential backwards compatibility concerns if the same pattern was introduced in other widgets.

I have a few other concerns and I would appreciate it if we could discuss them. I believe our goals are aligned, to make the widgets flexible and consistent. I'd also like for us to keep in mind the usability aspect of this, considering that a lot of TiddlyWiki users do not have a technical background. Consistency, and clear and non-overlapping terminology, is key to aiding user understanding.

Firstly, let me clarify what I meant by using a consistent name for a variable holding the value associated with an action firing on input or change events. The radio widget is a bit of an outlier in that the value associated with those events is an attribute of the widget, so yes the value is automatically available as attr-value. We document that all attributes to the widget are available as variables with the attr- prefix. Clear and easy to remember and understand. No problems.

However, this isn't the case with widgets such as the edit-text or range widgets. What variable name would be used here to provide the new value associated with input/change events to the actions being invoked? The value is not a widget attribute. If we use attr-value that creates confusion by clashing with the fact that widget parameters are resolved into variables with the attr- prefix. It would also potentially clash with a user deciding to add a parameter called value and rightfully expecting it to create a variable called attr-value

Hence my question: for the sake of consistency and usability, would it make sense to use a consistent and unique variable name for the value associated with the event that fires actions on input widgets? actionValue is just a suggestion. I am not too picky about the exact name as long as it is distinct and wont easily be confused with naming schemes used for other things, like variables with the attr- prefix.

Another thought here is if it might be worth considering to refactor the code that creates the variables from widget parameters into a separate function on the Widget base class.

That took longer than expected, so I'll come back with the one other concern later.

[pmario]

...
It would also potentially clash with a user deciding to add a parameter called value and rightfully expecting it to create a variable called attr-value

Hence my question: for the sake of consistency and usability, would it make sense to use a consistent and unique variable name for the value associated with the event that fires actions on input widgets? actionValue is just a suggestion. I am not too picky about the exact name as long as it is distinct and wont easily be confused with naming schemes used for other things, like variables with the attr- prefix.

Another thought here is if it might be worth considering to refactor the code that creates the variables from widget parameters into a separate function on the Widget base class.

I'm with you ... about all of them. ... I'll need to think about it.

[pmario]

About: actionXXXX variables.

• actionTiddler variable is used for "drag and drop" actions
  • In a slighthy different way, depending on the d&d-widget used.
• actionValue ... could be use for radio-widget and range-widget, because that fits the type of the value
• actionText ... imo could be used for text-widget ... It seems to hold the whole text in the edit-area. right?

For the docs, we can group them under eg: actionXYZ Tiddler

I think this would be consistently starting with an action prefix and uses Value, Text, Tiddler, which specifies the content of the variable.

[saqimtiaz]

• actionTiddler variable is used for "drag and drop" actions

  • In a slighthy different way, depending on the d&d-widget used.

• actionValue ... could be use for radio-widget and range-widget, because that fits the type of the value

• actionText ... imo could be used for text-widget ... It seems to hold the whole text in the edit-area. right?

@pmario that summary is very helpful, thank you.

If we were to group those widgets by what they are used for, we would have:

• drag and drop widgets (draggable, droppable)- all use the actionTiddler variable name
• input widgets (select, radio, range, checkbox) - all use the actionValue variable name
  • but the sole exception would be the input widget edit-text which would use actionText as the variable name.

Exceptions to rules is usually where users get confused and usability can suffer greatly. I think we would do well here to prioritize the user perspective over the developer one, as this a user facing feature rather than an internal JavaScript variable.

I understand why you feel actionText is a better name in the case of the edit-text widget, but I am wondering if from the usability and consistency point of view it might be better to still use actionValue? Not only would that make documentation easier, but it will be a lot more user friendly for users to remember that all input widgets use the variable name actionValue and drag and drop widgets use the variable name actionTiddler, with no exceptions.

Not to mention that semantically it is not wrong to use actionValue for the text widgets. While it would contain the entirety of the tiddler text, which might be long, it does represent the value of the widget's input DOM node like the other input widgets.

Sorry for the late reply. I'm expecting to be needed volunteering at the hospital from Monday, so this week is a bit hectic getting things organized in terms of other commitments.

[pmario]

@saqimtiaz .. see: 89ad523

[saqimtiaz]

@saqimtiaz .. see: 89ad523

Excellent @pmario

[saqimtiaz]

Perhaps:

if($tw.utils.count(changedAttributes) > 0 || changedTiddlers[this.radioTitle]) {
			this.refreshSelf();
			return true;
}

[saqimtiaz]

@Jermolene @pmario So is the intention that all attributes starting with $ are a reserved name space for future attributes that we might want to add to the widget? By allowing users to use arbitrary attribute names, in effect every attribute name becomes a reserved one, as it could clash with widget attributes that we introduce in the future.

At the moment it is just action-widgets that have attributes starting with $. This is confusing enough as it is but at least users can remember the rule that non-action widget attribute names do not start with $.

It would be quite confusing if suddenly any widget could have attribute names that may or not start with $. Especially if the widget previously has many attributes with no $ prefix and then suddenly has one new one introduced with a $ prefix.

[pmario]

What would you suggest?

[pmario]

I could imagine, that "user_attributes" start with an underscore. eg: _user-attribute and reserved-for-core and $reserved-for-core

[pmario]

or: __attribute with 2 underscores

[saqimtiaz]

@pmario I don't have an obvious solution that I particularly love.

However I think it is important that:
a) we try to avoid the potential confusion with attribute names if we can.
b) if we are going to proceed with this change, it needs to be a conscious decision where we consider the benefit of declaring variables in this manner vs the cost of creating usability issues and decide if this change is worthwhile.

In terms of other implementation approaches I considered requiring all user declared attributes to have a specific prefix, like var-. The problem is that while that makes it easier to identify user attributes, it makes it trickier to loop through the attributes and identify which is a widget "native" attribute since they have no specific prefix.

Every widget could have a list of attributes that get turned into variables, plus any user ones with a given prefix.

So in pseudo-code:

var widgetAttributes = ["value","name",label"];
$tw.utils.each(this.attributes,function(val,key) {
    if(widgetAttributes.indexOf(key) !== -1 || key.substring(0,4) === "var-"){
        // create a variable for this attribute.
    }
}

The other option we could consider is not to enforce a prefix for user attributes in the code, but rather make it a part of documentation and best practice. Explaining that using user attributes without a reserved prefix like "var-" could cause upgrade problems in the future. The problem with this approach is that I suspect most users will ignore it, and we will still have support issues when attribute names do clash in the future.

Does any of this perhaps inspire any ideas for you?

[saqimtiaz]

@pmario I didn't get to see your subsequent comments while typing. But yes, a prefix for user declared attributes would solve the problem.

[pmario]

I'm sometimes using a similar approach in my plugins, to make attribute code "loopable", if there are many of them, or if I'm not sure about the names. So it's easy to change the names, because I only need to change the innitial array.

So IMO @Jermolene should have a closer look.

[saqimtiaz]

@Jermolene to try and summarize for you:

• allowing any arbitrary name for attributes assigned by users to create variables, means that every attribute name is potentially a reserved name when/if we want to add more widget attributes in the future

• The code in this PR reserves the $ prefix namespace for attributes. So presumably future widget attributes would have to start with $. This is a usability concern, as outside of action widgets users can reliably expect widget attributes not to start with $. Imagine if the Button widget which has been around for a long time and has many attributes, suddenly has a new attribute that starts with $.

• One suggestion to mitigate this is, to require user assigned widget attributes to have a specific prefix, such as _ or even attr- (in which case the name of the custom attribute and the variable arising from it could be the same).

[Jermolene]

The behaviour here is so novel compared to other widgets that I favour a big obvious prefix on the user defined attributes that are to be passed through to the action string. I think var-xxxmakes some sense for both the attribute names and variable names because it makes it clearer that this is a mini version of the <$vars> widget...

[pmario]

@Jermolene ... We can not use the same prefix for attributes and user-variables ... because this would also cause a "name clash" later, if we add new attributes. ...

It would be OK to use var- instead of attr- ... BUT I think users already know that there is some magic going on with <<__named__>> variables.

So instead of the user code looking like this: (which imo is unreadable)

\define newText() value: $(attr-value)$

\define radioActions()
<$action-sendmessage $message="tm-notify" $param="test-radio-template" 
    actions=<<attr-actions>> tiddler=<<attr-tiddler>> value=<<attr-value>>
    name=<<var-name>> a=<<var-a>> b=<<var-b>> field=<<attr-field>> 
    current=<<currentTiddler>> />
<$action-createtiddler $basetitle=<<var-name>> $overwrite="yes" text=<<newText>>/>
\end

<$tiddler tiddler="$:/temp/test">
	<$radio field="id" value="a" actions=<<radioActions>> var-name="/name/a" var-a=x > a </$radio>|
	<$radio field="id" value="b" actions=<<radioActions>> var-name="/name/a" var-b=y > b </$radio>|
	<$radio field="id" value="c" actions=<<radioActions>> var-name="/name/a" > c </$radio>|
</$tiddler>

<$radio field="id" value="d"  actions=<<radioActions>> var-name="/name/x"> d </$radio>|
<$radio field="id" value="e" actions=<<radioActions>> var-name="/name/x"> e </$radio>|

It could look like this:

\define newText() value: $(__value__)$

\define radioActions()
<$action-sendmessage $message="tm-notify" $param="test-radio-template" 
    actions=<<__actions__>> tiddler=<<__tiddler__>> value=<<__value__>>
    name=<<var-name>> a=<<var-a>> b=<<var-b>> field=<<__field__>> 
    current=<<currentTiddler>> />
<$action-createtiddler $basetitle=<<var-name>> $overwrite="yes" text=<<newText>>/>
\end

<$tiddler tiddler="$:/temp/test">
	<$radio field="id" value="a" actions=<<radioActions>> var-name="/name/a" var-a=x > a </$radio>|
	<$radio field="id" value="b" actions=<<radioActions>> var-name="/name/a" var-b=y > b </$radio>|
	<$radio field="id" value="c" actions=<<radioActions>> var-name="/name/a" > c </$radio>|
</$tiddler>

<$radio field="id" value="d"  actions=<<radioActions>> var-name="/name/x"> d </$radio>|
<$radio field="id" value="e" actions=<<radioActions>> var-name="/name/x"> e </$radio>|

Which imo is a 100 times more intuitive

[pmario]

@Jermolene ... Since we probably can't reach a consensus about the variable naming convention, I'll close this PR and create a new one, which only passes the actionValue to the actions string. ... This is compatible with the existing widgets that call actions.

Instead of adding hardcoded variables I'll call a th-radio-variables hook, that will allow plugin authors to add hooks that will return a variables object, which fits the plugin. ...

So we are able to play with the functionality, until we are sure about the hardcoded names.

CC @saqimtiaz

new PR for radio-widget follows soon.

[saqimtiaz]

@pmario I think that is a good call and I was considering to suggest the same. There are quite a few things to consider with the variables and it would be good to be able to make a more considered decision and have the chance to test it out in practice before making it a part of an official release.

[pmario]

new PR see "Add radio actions, th-radio-variables hook and fix label refresh problem" #5154