[MGNLUI-6948] Fields within a switchable cannot be internationalized Created: 10/Nov/21 Updated: 07/Dec/21 Resolved: 07/Dec/21 |
|
| Status: | Closed |
| Project: | Magnolia UI |
| Component/s: | None |
| Affects Version/s: | 6.2.12 |
| Fix Version/s: | 6.2.14 |
| Type: | Bug | Priority: | Neutral |
| Reporter: | Richard Gange | Assignee: | Adam Siska |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | i18n | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Attachments: |
|
||||
| Issue Links: |
|
||||
| Template: |
|
||||
| Acceptance criteria: |
Empty
|
||||
| Task DoD: |
[X]*
Doc/release notes changes? Comment present?
[X]*
Downstream builds green?
[X]*
Solution information and context easily available?
[X]*
Tests
[X]*
FixVersion filled and not yet released
[ ] 
Architecture Decision Record (ADR)
|
||||
| Bug DoR: |
[X]*
Steps to reproduce, expected, and actual results filled
[X]*
Affected version filled
|
||||
| Date of First Response: | |||||
| Story Points: | 2 | ||||
| Description |
| Comments |
| Comment by Jörg Wirsig [ 11/Nov/21 ] |
|
Attached the RealOptionBuilder to make it easier to reproduce. |
| Comment by Mikaël Geljić [ 11/Nov/21 ] |
|
Hi joergwirsig, Thank you for reporting. At first, I'd like to point out that 5 UI switchable fields require to use the DelegatingCompositeFieldTransformer in order to support nesting fields or—as it turns out— i18n per sub-fields. So for me, the @TabFactory becomes: @TabFactory("Secondary") public void buttonLinkTab(Node node, UiConfig uiConfig, TabBuilder tabBuilder) { tabBuilder.fields( uiConfig.fields.text("keepitsimple").label("Simple Field"), uiConfig.fields.text("keepitlocalized").label("Locale Field") .i18n() .required(), uiConfig.fields.checkbox("checkit").label("Check it") .defaultValue("true") .buttonLabel("Check Button"), uiConfig.fields.switchable("testme").label("Test me") .options(new OptionBuilder().value("test1_text1").label("test1").selected(), new OptionBuilder().value("test2_text2").label("test2")) .fields(uiConfig.fields.text("test1_text1").label("Text 1") .required(), // inner field set to i18n() uiConfig.fields.text("test2_text2").label("Text 2") .required() .i18n()) .transformerClass(DelegatingCompositeFieldTransformer.class) .required() ); } For the story, SwitchableTransformer is the default; but we never got to change this default to the delegating impl, mostly because the delegating impl does not prefix sub-fields' properties with the switchable's own field name. Unfortunately, I cannot confirm a working result after the definition-conversion (despite traces of this transformer's support in SwitchableFieldDefinitionConverter), while it works like a charm in the compatibility Pages app. Cheers, Edit: The RealOptionBuilder helps a bit with selection, but still no i18n support. |
| Comment by Jörg Wirsig [ 12/Nov/21 ] |
|
Hi mgeljic,
Yes sure, in real world code we are using that transformer, but we are using several Helper classes to conviently generate some kind of fields so it would be very difficult to share the real world code. Instead i build that demo dialog to make it easier to strip the problem down to that what matters. Is there already a link to the MGNLUI Ticket you can share? |
| Comment by Mikaël Geljić [ 13/Nov/21 ] |
|
Moved this one to MGNLUI. |
| Comment by Jörg Wirsig [ 15/Nov/21 ] |
|
Hi mgeljic I'm pretty sure i found the reason, for the behaviour. See SwitchableFieldDefinitionConverter.java Line 73 public ConfiguredSwitchableFieldDefinition<Option> convert(SwitchableFieldDefinition oldDefinition) { ConfiguredSwitchableFieldDefinition<Option> newDefinition = super.convert(oldDefinition); OptionGroupFieldDefinition oldOptionField = new OptionGroupFieldDefinition(); oldOptionField.setName(oldDefinition.getName()); oldOptionField.setOptions(oldDefinition.getOptions()); oldOptionField.setI18n(oldDefinition.isI18n()); RadioButtonGroupFieldDefinition optionField = (RadioButtonGroupFieldDefinition) DefinitionConverter.FIELD.convert(oldOptionField); newDefinition.setField(optionField); List<FormDefinition<Option>> formList = new ArrayList<>(); ConfiguredFormDefinition form; for (ConfiguredFieldDefinition fieldDefinition : oldDefinition.getFields()) { fieldDefinition.setI18n(oldDefinition.isI18n()); form = new ConfiguredFormDefinition(); form.setName(fieldDefinition.getName()); form.setProperties(Collections.singletonList(DefinitionConverter.FIELD.convert(fieldDefinition))); formList.add(form); } newDefinition.setForms(formList); convertTransformerStrategy(oldDefinition, newDefinition); return newDefinition; } Where fieldDefinition.setI18n(oldDefinition.isI18n()); is setting i18n of subfields to the same as the i18n setting of the switchable itself. It seems to be done on purpose but i don't know why. But replacing the converter with a different implementation without that line everything seems to be working properly. Can you or someone else give me short feedback, if that line of code is just a bug or if it has a special purpose which I'm currently missing. |
| Comment by Richard Gange [ 16/Nov/21 ] |
|
Hello joergwirsig- It's a bug. Not specifically a blossom bug though. A UI bug instead so the ticket was moved to this project. The problem now is it becomes a low prio bug since we are talking about support for older dialogs. However, you are saying that replacing that single line did fix the issue for you? I can create the PR if so. Cheers |
| Comment by Jörg Wirsig [ 16/Nov/21 ] |
|
I just thought about that issue again, and i guess the code should look like for (ConfiguredFieldDefinition fieldDefinition : oldDefinition.getFields()) { if(oldDefinition.isI18n()){ fieldDefinition.setI18n(oldDefinition.isI18n()); } form = new ConfiguredFormDefinition(); form.setName(fieldDefinition.getName()); form.setProperties(Collections.singletonList(DefinitionConverter.FIELD.convert(fieldDefinition))); formList.add(form); } I guess the purpose was to set all subfields to i18n if switchable is i18n. Deactivation if i18n on subfields if switchable is not i18n seems to be an unwanted side effect. In the meanwhile we will add our own converter with the fix to DefinitionConverter.FIELD_DEFINITION_CONVERTERS. |
| Comment by Roman Kovařík [ 03/Dec/21 ] |
|
Estimated: integrate and QA the existing PR. |