[GitHub] [wicket] ams-tschoening opened a new pull request #457: WICKET-6823: Add a container to AbstractTransformerBehavior to support multiple instances per component.

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] ams-tschoening opened a new pull request #457: WICKET-6823: Add a container to AbstractTransformerBehavior to support multiple instances per component.

GitBox

ams-tschoening opened a new pull request #457:
URL: https://github.com/apache/wicket/pull/457


   The current implementation of AbstractTransformerBehavior replaces responses and [doesn't properly take into account](https://issues.apache.org/jira/projects/WICKET/issues/WICKET-6823) 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 `TransformerBehaviors`, `TransformerBehavior[Container|Multi]`, `MultiTransformerBehavior` 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.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] martin-g commented on a change in pull request #457: WICKET-6823: Add a container to AbstractTransformerBehavior to support multiple instances per component.

GitBox

martin-g commented on a change in pull request #457:
URL: https://github.com/apache/wicket/pull/457#discussion_r508031845



##########
File path: wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
##########
@@ -26,17 +31,114 @@
 import org.apache.wicket.request.http.WebResponse;
 
 /**
- * A {@link Behavior} which can be added to any component. It allows to post-process (transform) the
+ * A {@link Behavior} which can be added to any component, allowing to post-process (transform) the
  * markup generated by the component.
- *
+ * <p>
+ * There's one important limitation with the current implementation: Multiple different instances of
+ * this behavior CAN NOT be assigned to the same component! If one whiches to do so, the contained

Review comment:
       s/whiches/wishes/

##########
File path: wicket-core/src/test/java/org/apache/wicket/markup/transformer/AbstractTransformerBehaviorTest.java
##########
@@ -69,7 +75,77 @@ public void transformationInAjaxRequest()
  tester.clickLink("updateLabel", true);
  tester.assertContains("ajax request");
  tester.assertContainsNot("normal request");
+ }
 
+ /**
+ * Test how multiple different transformers applied to the same component behave.
+ * <p>
+ * The current implementation of {@link AbstractTransformerBehavior} doesn't support multiple
+ * instances on the same component, a container needs to be used explicitly instead. So make
+ * sure the implementation is as expected, as otherwise the container might not be necessary at
+ * all anymore, and that the container really works around the problem.
+ * </p>
+ * @see <a href="https://issues.apache.org/jira/projects/WICKET/issues/WICKET-6823">JIRA issue</a>
+ */
+ @Test
+ public void multiTransesSameComp()
+ {
+ TestPage testPage = new TestPage();
+ Label label = new Label(TestPage.LABEL_ID, TestPage.LABEL_VAL);
+
+ Function<String, AbstractTransformerBehavior> transProd = (val) ->

Review comment:
       s/transProd/transformerProducer/

##########
File path: wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
##########
@@ -26,17 +31,114 @@
 import org.apache.wicket.request.http.WebResponse;
 
 /**
- * A {@link Behavior} which can be added to any component. It allows to post-process (transform) the
+ * A {@link Behavior} which can be added to any component, allowing to post-process (transform) the
  * markup generated by the component.
- *
+ * <p>
+ * There's one important limitation with the current implementation: Multiple different instances of
+ * this behavior CAN NOT be assigned to the same component! If one whiches to do so, the contained
+ * container needs to be used to wrap existing behaviors and that container needs to be added to the
+ * component instead. The current implementation of works with temporary responses, but does not
+ * support nesting itself properly, which results in missing rendered output and most likely broken
+ * HTML documents in the end.
+ * </p>
  * @see org.apache.wicket.markup.transformer.AbstractOutputTransformerContainer
- *
+ * @see <a href="https://issues.apache.org/jira/projects/WICKET/issues/WICKET-6823">JIRA issue</a>
+ *
  * @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.
+ * <p>
+ * This container is by design NOT about multiple arbitrary transformations, but really only for the
+ * one use case supporting multiple instances of {@link AbstractTransformerBehavior} on one and the
+ * same component. The current implementation of that works with temporary responses, but doesn't
+ * support nesting itself properly in case multiple behaviors assigned to the same component, which
+ * results in missing rendered output and most likely entirely broken HTML documents in the end.
+ * </p>
+ * <p>
+ * The easiest workaround for that problem is simply introducing this container which users need to
+ * use in those cases: An instance needs to be created with all transformers of interest in the
+ * order they should be applied and the container takes care of doing so. Because the container is
+ * an {@link AbstractTransformerBehavior} itself, things simply work like with individual behaviors,
+ * while response handling is only managed by the container. So when used with this container, the
+ * callbacks of the maintained instances like {@link AbstractTransformerBehavior#afterRender(Component)}
+ * etc., are NOT used anymore! OTOH, the individual behaviors stay useful without the container as
+ * well.
+ * </p>
+ * @see <a href="https://issues.apache.org/jira/projects/WICKET/issues/WICKET-6823">JIRA issue</a>
+ */
+ public static class Multi extends AbstractTransformerBehavior
+ {
+ private static final long serialVersionUID = 1L;
+
+ /**
+ * All transformers which need to be applied in the order they need to be applied.

Review comment:
       Too many "need to be applied". The sentence does not sound good/clear.

##########
File path: wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
##########
@@ -26,17 +31,114 @@
 import org.apache.wicket.request.http.WebResponse;
 
 /**
- * A {@link Behavior} which can be added to any component. It allows to post-process (transform) the
+ * A {@link Behavior} which can be added to any component, allowing to post-process (transform) the
  * markup generated by the component.
- *
+ * <p>
+ * There's one important limitation with the current implementation: Multiple different instances of
+ * this behavior CAN NOT be assigned to the same component! If one whiches to do so, the contained
+ * container needs to be used to wrap existing behaviors and that container needs to be added to the
+ * component instead. The current implementation of works with temporary responses, but does not
+ * support nesting itself properly, which results in missing rendered output and most likely broken
+ * HTML documents in the end.
+ * </p>
  * @see org.apache.wicket.markup.transformer.AbstractOutputTransformerContainer
- *
+ * @see <a href="https://issues.apache.org/jira/projects/WICKET/issues/WICKET-6823">JIRA issue</a>
+ *
  * @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.
+ * <p>
+ * This container is by design NOT about multiple arbitrary transformations, but really only for the
+ * one use case supporting multiple instances of {@link AbstractTransformerBehavior} on one and the
+ * same component. The current implementation of that works with temporary responses, but doesn't
+ * support nesting itself properly in case multiple behaviors assigned to the same component, which
+ * results in missing rendered output and most likely entirely broken HTML documents in the end.
+ * </p>
+ * <p>
+ * The easiest workaround for that problem is simply introducing this container which users need to
+ * use in those cases: An instance needs to be created with all transformers of interest in the
+ * order they should be applied and the container takes care of doing so. Because the container is
+ * an {@link AbstractTransformerBehavior} itself, things simply work like with individual behaviors,
+ * while response handling is only managed by the container. So when used with this container, the
+ * callbacks of the maintained instances like {@link AbstractTransformerBehavior#afterRender(Component)}
+ * etc., are NOT used anymore! OTOH, the individual behaviors stay useful without the container as
+ * well.
+ * </p>
+ * @see <a href="https://issues.apache.org/jira/projects/WICKET/issues/WICKET-6823">JIRA issue</a>
+ */
+ public static class Multi extends AbstractTransformerBehavior
+ {
+ private static final long serialVersionUID = 1L;
+
+ /**
+ * All transformers which need to be applied in the order they need to be applied.
+ */
+ private final List<AbstractTransformerBehavior> transes;

Review comment:
       s/trances/transformers/

##########
File path: wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
##########
@@ -26,17 +31,114 @@
 import org.apache.wicket.request.http.WebResponse;
 
 /**
- * A {@link Behavior} which can be added to any component. It allows to post-process (transform) the
+ * A {@link Behavior} which can be added to any component, allowing to post-process (transform) the
  * markup generated by the component.
- *
+ * <p>
+ * There's one important limitation with the current implementation: Multiple different instances of
+ * this behavior CAN NOT be assigned to the same component! If one whiches to do so, the contained
+ * container needs to be used to wrap existing behaviors and that container needs to be added to the
+ * component instead. The current implementation of works with temporary responses, but does not
+ * support nesting itself properly, which results in missing rendered output and most likely broken
+ * HTML documents in the end.
+ * </p>
  * @see org.apache.wicket.markup.transformer.AbstractOutputTransformerContainer
- *
+ * @see <a href="https://issues.apache.org/jira/projects/WICKET/issues/WICKET-6823">JIRA issue</a>
+ *
  * @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.

Review comment:
       This javadoc should really be a comment in the PR, not Javadoc.

##########
File path: wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
##########
@@ -26,17 +31,114 @@
 import org.apache.wicket.request.http.WebResponse;
 
 /**
- * A {@link Behavior} which can be added to any component. It allows to post-process (transform) the
+ * A {@link Behavior} which can be added to any component, allowing to post-process (transform) the
  * markup generated by the component.
- *
+ * <p>
+ * There's one important limitation with the current implementation: Multiple different instances of

Review comment:
       This Javadoc is obsolete with the improvement below, no ?

##########
File path: wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
##########
@@ -26,17 +31,114 @@
 import org.apache.wicket.request.http.WebResponse;
 
 /**
- * A {@link Behavior} which can be added to any component. It allows to post-process (transform) the
+ * A {@link Behavior} which can be added to any component, allowing to post-process (transform) the
  * markup generated by the component.
- *
+ * <p>
+ * There's one important limitation with the current implementation: Multiple different instances of
+ * this behavior CAN NOT be assigned to the same component! If one whiches to do so, the contained
+ * container needs to be used to wrap existing behaviors and that container needs to be added to the
+ * component instead. The current implementation of works with temporary responses, but does not

Review comment:
       s/The current implementation of works with temporary responses/The current implementation works with temporary responses/

##########
File path: wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
##########
@@ -26,17 +31,114 @@
 import org.apache.wicket.request.http.WebResponse;
 
 /**
- * A {@link Behavior} which can be added to any component. It allows to post-process (transform) the
+ * A {@link Behavior} which can be added to any component, allowing to post-process (transform) the
  * markup generated by the component.
- *
+ * <p>
+ * There's one important limitation with the current implementation: Multiple different instances of
+ * this behavior CAN NOT be assigned to the same component! If one whiches to do so, the contained
+ * container needs to be used to wrap existing behaviors and that container needs to be added to the
+ * component instead. The current implementation of works with temporary responses, but does not
+ * support nesting itself properly, which results in missing rendered output and most likely broken
+ * HTML documents in the end.
+ * </p>
  * @see org.apache.wicket.markup.transformer.AbstractOutputTransformerContainer
- *
+ * @see <a href="https://issues.apache.org/jira/projects/WICKET/issues/WICKET-6823">JIRA issue</a>
+ *
  * @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.
+ * <p>
+ * This container is by design NOT about multiple arbitrary transformations, but really only for the
+ * one use case supporting multiple instances of {@link AbstractTransformerBehavior} on one and the
+ * same component. The current implementation of that works with temporary responses, but doesn't
+ * support nesting itself properly in case multiple behaviors assigned to the same component, which
+ * results in missing rendered output and most likely entirely broken HTML documents in the end.
+ * </p>
+ * <p>
+ * The easiest workaround for that problem is simply introducing this container which users need to
+ * use in those cases: An instance needs to be created with all transformers of interest in the
+ * order they should be applied and the container takes care of doing so. Because the container is
+ * an {@link AbstractTransformerBehavior} itself, things simply work like with individual behaviors,
+ * while response handling is only managed by the container. So when used with this container, the
+ * callbacks of the maintained instances like {@link AbstractTransformerBehavior#afterRender(Component)}
+ * etc., are NOT used anymore! OTOH, the individual behaviors stay useful without the container as
+ * well.
+ * </p>
+ * @see <a href="https://issues.apache.org/jira/projects/WICKET/issues/WICKET-6823">JIRA issue</a>
+ */
+ public static class Multi extends AbstractTransformerBehavior
+ {
+ private static final long serialVersionUID = 1L;
+
+ /**
+ * All transformers which need to be applied in the order they need to be applied.
+ */
+ private final List<AbstractTransformerBehavior> transes;
+
+ /**
+ * CTOR simply storing the given transformers.

Review comment:
       s/CTOR/Constructor/

##########
File path: wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
##########
@@ -26,17 +31,114 @@
 import org.apache.wicket.request.http.WebResponse;
 
 /**
- * A {@link Behavior} which can be added to any component. It allows to post-process (transform) the
+ * A {@link Behavior} which can be added to any component, allowing to post-process (transform) the
  * markup generated by the component.
- *
+ * <p>
+ * There's one important limitation with the current implementation: Multiple different instances of
+ * this behavior CAN NOT be assigned to the same component! If one whiches to do so, the contained
+ * container needs to be used to wrap existing behaviors and that container needs to be added to the
+ * component instead. The current implementation of works with temporary responses, but does not
+ * support nesting itself properly, which results in missing rendered output and most likely broken
+ * HTML documents in the end.
+ * </p>
  * @see org.apache.wicket.markup.transformer.AbstractOutputTransformerContainer
- *
+ * @see <a href="https://issues.apache.org/jira/projects/WICKET/issues/WICKET-6823">JIRA issue</a>
+ *
  * @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.
+ * <p>
+ * This container is by design NOT about multiple arbitrary transformations, but really only for the
+ * one use case supporting multiple instances of {@link AbstractTransformerBehavior} on one and the
+ * same component. The current implementation of that works with temporary responses, but doesn't
+ * support nesting itself properly in case multiple behaviors assigned to the same component, which
+ * results in missing rendered output and most likely entirely broken HTML documents in the end.
+ * </p>
+ * <p>
+ * The easiest workaround for that problem is simply introducing this container which users need to
+ * use in those cases: An instance needs to be created with all transformers of interest in the
+ * order they should be applied and the container takes care of doing so. Because the container is
+ * an {@link AbstractTransformerBehavior} itself, things simply work like with individual behaviors,
+ * while response handling is only managed by the container. So when used with this container, the
+ * callbacks of the maintained instances like {@link AbstractTransformerBehavior#afterRender(Component)}
+ * etc., are NOT used anymore! OTOH, the individual behaviors stay useful without the container as
+ * well.
+ * </p>
+ * @see <a href="https://issues.apache.org/jira/projects/WICKET/issues/WICKET-6823">JIRA issue</a>
+ */
+ public static class Multi extends AbstractTransformerBehavior
+ {
+ private static final long serialVersionUID = 1L;
+
+ /**
+ * All transformers which need to be applied in the order they need to be applied.
+ */
+ private final List<AbstractTransformerBehavior> transes;
+
+ /**
+ * CTOR simply storing the given transformers.
+ *
+ * @param transes, which must not be {@code null} or empty, as neither make sense here.
+ */
+ private Multi(List<AbstractTransformerBehavior> transes)
+ {
+ if ((transes == null) || transes.isEmpty())
+ {
+ throw new IllegalArgumentException("No transformers given.");
+ }
+
+ this.transes = transes;
+ }
+
+ @Override
+ public CharSequence transform(Component component, CharSequence output) throws Exception
+ {
+ CharSequence retVal = output;
+ for (AbstractTransformerBehavior trans : this.transes)
+ {
+ retVal = trans.transform(component, retVal);
+ }
+
+ return retVal;
+ }
+
+ /**
+ * Create a new container with the given transformers and with keeping their order.
+ * <p>
+ * This factory expects multiple individual transformers already, because creating a container
+ * with less doesn't make too much sense and users should reconsider then if this container is
+ * of use at all. In most cases users do have individual transformers to apply only anyway and
+ * don't need to provide a collection themself this way. OTOH, a collection could be empty or
+ * contain only one element would then defeat the purpose of this container again.
+ * </p>
+ * @param first First transformer to apply.
+ * @param second Second transformer to apply.
+ * @param moreIf All other transformers to apply, if at all, in given order.
+ * @return A container with multiple transformers being applied.
+ */
+ public static Multi newFor( AbstractTransformerBehavior first,

Review comment:
       s/newFor/of/ ?! Like Model.of() and the new collection methods in Java 9

##########
File path: wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
##########
@@ -26,17 +31,114 @@
 import org.apache.wicket.request.http.WebResponse;
 
 /**
- * A {@link Behavior} which can be added to any component. It allows to post-process (transform) the
+ * A {@link Behavior} which can be added to any component, allowing to post-process (transform) the
  * markup generated by the component.
- *
+ * <p>
+ * There's one important limitation with the current implementation: Multiple different instances of
+ * this behavior CAN NOT be assigned to the same component! If one whiches to do so, the contained
+ * container needs to be used to wrap existing behaviors and that container needs to be added to the
+ * component instead. The current implementation of works with temporary responses, but does not
+ * support nesting itself properly, which results in missing rendered output and most likely broken
+ * HTML documents in the end.
+ * </p>
  * @see org.apache.wicket.markup.transformer.AbstractOutputTransformerContainer
- *
+ * @see <a href="https://issues.apache.org/jira/projects/WICKET/issues/WICKET-6823">JIRA issue</a>

Review comment:
       No need of this link here




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] ams-tschoening commented on a change in pull request #457: WICKET-6823: Add a container to AbstractTransformerBehavior to support multiple instances per component.

GitBox
In reply to this post by GitBox

ams-tschoening commented on a change in pull request #457:
URL: https://github.com/apache/wicket/pull/457#discussion_r508235530



##########
File path: wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
##########
@@ -26,17 +31,114 @@
 import org.apache.wicket.request.http.WebResponse;
 
 /**
- * A {@link Behavior} which can be added to any component. It allows to post-process (transform) the
+ * A {@link Behavior} which can be added to any component, allowing to post-process (transform) the
  * markup generated by the component.
- *
+ * <p>
+ * There's one important limitation with the current implementation: Multiple different instances of

Review comment:
       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 `AbstractTransformerBehavior` itself would be another one. With that the sentence could be removed, but with the container only the sentence still applies.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] ams-tschoening commented on a change in pull request #457: WICKET-6823: Add a container to AbstractTransformerBehavior to support multiple instances per component.

GitBox
In reply to this post by GitBox

ams-tschoening commented on a change in pull request #457:
URL: https://github.com/apache/wicket/pull/457#discussion_r508235530



##########
File path: wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
##########
@@ -26,17 +31,114 @@
 import org.apache.wicket.request.http.WebResponse;
 
 /**
- * A {@link Behavior} which can be added to any component. It allows to post-process (transform) the
+ * A {@link Behavior} which can be added to any component, allowing to post-process (transform) the
  * markup generated by the component.
- *
+ * <p>
+ * There's one important limitation with the current implementation: Multiple different instances of

Review comment:
       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.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] martin-g commented on a change in pull request #457: WICKET-6823: Add a container to AbstractTransformerBehavior to support multiple instances per component.

GitBox
In reply to this post by GitBox

martin-g commented on a change in pull request #457:
URL: https://github.com/apache/wicket/pull/457#discussion_r508238822



##########
File path: wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
##########
@@ -26,17 +31,114 @@
 import org.apache.wicket.request.http.WebResponse;
 
 /**
- * A {@link Behavior} which can be added to any component. It allows to post-process (transform) the
+ * A {@link Behavior} which can be added to any component, allowing to post-process (transform) the
  * markup generated by the component.
- *
+ * <p>
+ * There's one important limitation with the current implementation: Multiple different instances of

Review comment:
       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.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] ams-tschoening commented on a change in pull request #457: WICKET-6823: Add a container to AbstractTransformerBehavior to support multiple instances per component.

GitBox
In reply to this post by GitBox

ams-tschoening commented on a change in pull request #457:
URL: https://github.com/apache/wicket/pull/457#discussion_r508260776



##########
File path: wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
##########
@@ -26,17 +31,114 @@
 import org.apache.wicket.request.http.WebResponse;
 
 /**
- * A {@link Behavior} which can be added to any component. It allows to post-process (transform) the
+ * A {@link Behavior} which can be added to any component, allowing to post-process (transform) the
  * markup generated by the component.
- *
+ * <p>
+ * There's one important limitation with the current implementation: Multiple different instances of
+ * this behavior CAN NOT be assigned to the same component! If one whiches to do so, the contained
+ * container needs to be used to wrap existing behaviors and that container needs to be added to the
+ * component instead. The current implementation of works with temporary responses, but does not
+ * support nesting itself properly, which results in missing rendered output and most likely broken
+ * HTML documents in the end.
+ * </p>
  * @see org.apache.wicket.markup.transformer.AbstractOutputTransformerContainer
- *
+ * @see <a href="https://issues.apache.org/jira/projects/WICKET/issues/WICKET-6823">JIRA issue</a>
+ *
  * @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.

Review comment:
       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.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] ams-tschoening commented on a change in pull request #457: WICKET-6823: Add a container to AbstractTransformerBehavior to support multiple instances per component.

GitBox
In reply to this post by GitBox

ams-tschoening commented on a change in pull request #457:
URL: https://github.com/apache/wicket/pull/457#discussion_r508265392



##########
File path: wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java
##########
@@ -26,17 +31,114 @@
 import org.apache.wicket.request.http.WebResponse;
 
 /**
- * A {@link Behavior} which can be added to any component. It allows to post-process (transform) the
+ * A {@link Behavior} which can be added to any component, allowing to post-process (transform) the
  * markup generated by the component.
- *
+ * <p>
+ * There's one important limitation with the current implementation: Multiple different instances of

Review comment:
       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.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]