Skip to content
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

Open
wants to merge 6 commits into
base: wicket-8.x
Choose a base branch
from

Conversation

ams-tschoening
Copy link
Contributor

@ams-tschoening ams-tschoening commented Oct 9, 2020

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.

…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.
* markup generated by the component.
*
* <p>
* There's one important limitation with the current implementation: Multiple different instances of
Copy link
Member

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 ?

Copy link
Contributor Author

@ams-tschoening ams-tschoening Oct 20, 2020

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

* @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.
Copy link
Member

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.

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants