Conversation
@resetnow : this one is for you ,) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my remark, otherwise LGTM
@@ -82,7 +82,8 @@ public boolean canRender(Widget w) { | |||
String button = getSnippet("button"); | |||
button = StringUtils.replace(button, "%item%", w.getItem()); | |||
button = StringUtils.replace(button, "%cmd%", mapping.getCmd()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need escape here as well. Test case:
Switch item=DemoString mappings=["value__1"="description_1", "value_\"_2"="description_2"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, using StringEscapeUtils.escapeHTML
here is probably not the right choice — only quotes and backslashes need to be prepended with \
so resulting HTML attribute has correct value when it's being read from Javascript.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about mappings from number or ON/OFF/OPEN/CLOSE... for example but not from a general string.
You're right, it has to be tested and fixed for general string commands too.
Fix #3744 Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks! |
Fix #3744
Signed-off-by: Laurent Garnier lg.hc@free.fr