-
Notifications
You must be signed in to change notification settings - Fork 394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WICKET-6823: Add a container to AbstractTransformerBehavior to support multiple instances per component. #457
base: wicket-8.x
Are you sure you want to change the base?
Conversation
…t multiple instances per component. The current implementation of AbstractTransformerBehavior replaces responses and doesn't properly take into account use cases in which multiple instances doing the same are assigned to the same component. This adds a container for those use cases to combine all those transformers into one dealing with replacing the responses instead. While that container needs to be used explicitly, it's the easiest implementation to workaround the current limitations.
wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
Outdated
Show resolved
Hide resolved
wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
Outdated
Show resolved
Hide resolved
* markup generated by the component. | ||
* | ||
* <p> | ||
* There's one important limitation with the current implementation: Multiple different instances of |
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.
This Javadoc is obsolete with the improvement below, no ?
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 disagree, AbstractTransformerBehavior
itself keeps the limitation and especially existing users should be made aware. The container is only one possible workaround/solution, a different implementation of AbstractTransformerBehavior
itself would be another one. With that the sentence could be removed, but with the container only the sentence still applies.
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.
You can just say "If you need to apply several transformations on the same component then use {@linkplain AbstractTransformerBehavior.Multi}".
I see no need to write 20 lines as Javadoc explaining history.
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.
Those 20 lines of history are exactly what I regularly miss when running into trouble with Wicket, because without them there's no basic understanding of why things are the way they are. When running into this problem, I needed to look at the entire GIT-history just to find no reason for the current implementation, no clue on how things were supposed to work, for a workaround fitting the existing design etc.
The docs often lack important context from my experience, so I'm trying to add one here.
wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
Outdated
Show resolved
Hide resolved
* @author Juergen Donnerstag | ||
*/ | ||
public abstract class AbstractTransformerBehavior extends Behavior implements ITransformer | ||
{ | ||
private static final long serialVersionUID = 1L; | ||
|
||
/** | ||
* Container to apply multiple {@link AbstractTransformerBehavior} to some component. |
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.
This javadoc should really be a comment in the PR, not Javadoc.
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.
As someone who ran into that problem, I prefer having such details in the file to get a basic understanding of problems like these without looking at source history, JIRA etc. I regularly miss such details from Wicket's docs and rephrased things to not sound like a commit message too much anymore.
wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
Outdated
Show resolved
Hide resolved
wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
Outdated
Show resolved
Hide resolved
wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
Outdated
Show resolved
Hide resolved
wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
Outdated
Show resolved
Hide resolved
...core/src/test/java/org/apache/wicket/markup/transformer/AbstractTransformerBehaviorTest.java
Outdated
Show resolved
Hide resolved
…it message too much.
The current implementation of AbstractTransformerBehavior replaces responses and doesn't properly take into account use cases in which multiple instances doing the same are assigned to the same component. This adds a container for those use cases to combine all those transformers into one dealing with replacing the responses instead. While that container needs to be used explicitly, it's the easiest implementation to workaround the current limitations.
The container is embedded into
AbstractTransformerBehavior
, because it's small overall, can easily be spotted this way by users and I wasn't too sure about a good alternative name anyway. There would be many possibilities like[Abstract]TransformerBehaviors
,[Abstract]TransformerBehavior[Container|Multi]
,MultiAbstractTransformerBehavior
etc..This PR is based on wicket-8.x instead of master, because that is what I'm using currently and the only thing I'm able to test and build right now.