Remove usage of the getTag and setTag methods #1740
Conversation
The WComponent::getTag and WComponent::setTag methods are deprecated and are used by WDefinitionList to attach a term to a component. Using a tag to store this term is replaced with using an attribute instead.
There was a problem hiding this comment.
Change looks good.
Just in case projects are using getTag to refer to the value of the term, can you explain in the change log how they can get the value now. But to do this it might be easier to make the static string TERM_ATTRIBUTE public so they can access it.
By not setting the tag property on items added for a term, then the public API of WDefinitionList is changed. This should be avoided until the next major version. So, in order to maintain this API the tag for is set via setTag(), but the tag is not used internally.
|
By not setting the tag property on items added for a term, then the public API of WDefinitionList is changed. I had not considered this before making the change. This type of change should be avoided until the next major version. So, in order to maintain this existing API, and to avoid increasing the size of the API by exposing |
JohnMcGuinness
requested review from
jonathanaustin
and removed request for
jonathanaustin
JohnMcGuinness
requested review from
jonathanaustin
and removed request for
jonathanaustin
| @@ -16,6 +16,8 @@ | |||
| public class WDefinitionList extends AbstractNamingContextContainer implements AjaxTarget, | |||
| SubordinateTarget, Marginable { | |||
|
|
|||
| private static final String TERM_ATTRIBUTE = "WDefinitionList.term"; | |||
There was a problem hiding this comment.
Even though the change now sets the term on the tag and the attribute, I think planning ahead for getTag() being removed, it would still be good for the TERM_ATTRIBUTE to be made public and explain how they will need to access the term via the attribute in the change log.
It would also be good to include how to use the attribute in the javadoc of WDefinitionList.
There was a problem hiding this comment.
I don't understand why TERM_ATTRIBUTE should be made public. It should be considered an implementation detail. To add a term you can use addTerm(String, WComponent...) to get all of the terms you can use getTerms() or to get a specific term use getComponentsForTerm(String).
There was a problem hiding this comment.
I could envision a completely different internal implementation where WContainers were used as buckets for the components of each term, and each WComponent bucket, which is unknown to the user, has an attribute with the name of the term. In fact this might be a better implementation.
There was a problem hiding this comment.
You are right. The whole implementation of WDefinitionList smells as it originally hijacked the getTag method from the template logic and using the attribute is just as smelly. It might not even be worth doing this change until we consider how it could be refactored and deleting getTag is way off in the future anyway. Let me know what you think.
…ness/wcomponents into remove-usage-of-getTag-setTag
The WComponent::getTag and WComponent::setTag methods are deprecated and
are used by WDefinitionList to attach a term to a component. Using a
tag to store this term is replaced with using an attribute instead.
Partly addresses #1732