search

icfnext / cq-component-maven-plugin

Other
22 stars 34 forks source link

Fix for using namePrefix with multifield #30

Closed alexlockhart7 closed 8 years ago

alexlockhart7 commented 8 years ago

Right now the child field of the MultiField does not get the namePrefix if the MultiField is the child of a DialogFieldSet. Basically, a "bandaid" method was added to allow the name of the DialogFieldSet child to be changed after being created, in order to account for the namePrefix. However, in the case that the child was a MultiField, the MultiField's child's name was not updated.

This is a fix for that issue. Now, the name of the field is updated before creating the child of the DialogFieldSet. That way, in the case that the child is a MultiField, both the MultiField itself and the MultiField's child will receive the namePrefix.

alexlockhart7 commented 8 years ago

This is ready for code review @pmichelotti or someone else.

michaelhodgdon commented 8 years ago

Alex, Why was setName removed? That would make this no longer backwards compatible and require a major version update. Also, does this issue occur in classic ui?

-Mike

alexlockhart7 commented 8 years ago

Why was setName removed? setName was removed because it appeared to be added for the specific purpose of resetting the name based upon the namePrefix. With this change that was no longer needed. As you can see the class that had the setName method was not originally intended to have setters.

That would make this no longer backwards compatible and require a major version update. I don’t believe that this would require a major version update because it is internal to the implementation of the component. No external interface for using this component is changed. The change is simply in respect to how the namePrefix gets added to the name.

Does this issue occur in classic ui? It appears that the classic ui dialog multi field actually uses the name of the multi field node itself instead of the child. Therefore, this problem is not relevant to classic ui.

michaelhodgdon commented 8 years ago

AbstractTouchUIWidget is not just an internal class, it is one that is meant to be extended by any widget internally or externally, therefore removing it (even if it is currently used internally only for this) would require a major version update as we don't know where it was used externally. We need to add that back.

alexlockhart7 commented 8 years ago

@michaelhodgdon Sounds good. That has been added back.