You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@wicket.apache.org by GitBox <gi...@apache.org> on 2022/04/20 18:17:47 UTC
[GitHub] [wicket] theigl opened a new pull request, #517: WICKET-6963 Singleton markup sourcing strategy (Take 2)
theigl opened a new pull request, #517:
URL: https://github.com/apache/wicket/pull/517
This is my second attempt at using a singleton PanelMarkupSourcingStrategy for panels and borders.
My initial PR #503 had to be reverted, because I overlooked the non-final field `AssociatedMarkupSourcingStrategy#noMoreWicketHeadTagsAllowed` resulting in concurrency issues.
The field is questionable and the code related to it full of TODO and Why? comments. This PR gets rid of it. We have nearly two dozen tests for this code in HeaderSectionTest and the only result of deleting the field is a different error message in one of these tests.
The error message for the test changes from:
> \<wicket:head> tags are only allowed before \<body>, \</head>, \<wicket:panel> etc. tag
to the following message thrown from HtmlHeaderResolver:
> Mis-placed \<wicket:head>. \<wicket:head> must be outside of \<wicket:panel>, \<wicket:border>, and <wicket:extend>
Since the flag is used for validation only, this change cannot break existing code.
https://issues.apache.org/jira/browse/WICKET-6963
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a diff in pull request #517: WICKET-6963 Singleton markup sourcing strategy (Take 2)
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #517:
URL: https://github.com/apache/wicket/pull/517#discussion_r860505018
##########
wicket-core/src/main/java/org/apache/wicket/markup/html/panel/PanelMarkupSourcingStrategy.java:
##########
@@ -33,9 +33,28 @@
*/
public class PanelMarkupSourcingStrategy extends AssociatedMarkupSourcingStrategy
{
+ private static final PanelMarkupSourcingStrategy PANEL_INSTANCE = new PanelMarkupSourcingStrategy(
+ false);
+ private static final PanelMarkupSourcingStrategy BORDER_INSTANCE = new PanelMarkupSourcingStrategy(
+ true);
+
// False for Panel and true for Border components.
private final boolean allowWicketComponentsInBodyMarkup;
Review Comment:
> I couldn't think of a good name for the new enum.
`Type` ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a diff in pull request #517: WICKET-6963 Singleton markup sourcing strategy (Take 2)
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #517:
URL: https://github.com/apache/wicket/pull/517#discussion_r860506019
##########
wicket-core/src/main/java/org/apache/wicket/markup/html/panel/PanelMarkupSourcingStrategy.java:
##########
@@ -33,9 +33,28 @@
*/
public class PanelMarkupSourcingStrategy extends AssociatedMarkupSourcingStrategy
{
+ private static final PanelMarkupSourcingStrategy PANEL_INSTANCE = new PanelMarkupSourcingStrategy(
+ false);
+ private static final PanelMarkupSourcingStrategy BORDER_INSTANCE = new PanelMarkupSourcingStrategy(
+ true);
+
// False for Panel and true for Border components.
private final boolean allowWicketComponentsInBodyMarkup;
Review Comment:
I'd drop the usage of the `boolean` in the enum. Just compare the variants.
> I'm not sure this refactoring is really worth it.
I think it is a good idea, but I am not keen on it.
Thanks for trying it anyway!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a diff in pull request #517: WICKET-6963 Singleton markup sourcing strategy (Take 2)
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #517:
URL: https://github.com/apache/wicket/pull/517#discussion_r859865225
##########
wicket-core/src/main/java/org/apache/wicket/markup/html/panel/PanelMarkupSourcingStrategy.java:
##########
@@ -33,9 +33,28 @@
*/
public class PanelMarkupSourcingStrategy extends AssociatedMarkupSourcingStrategy
{
+ private static final PanelMarkupSourcingStrategy PANEL_INSTANCE = new PanelMarkupSourcingStrategy(
+ false);
+ private static final PanelMarkupSourcingStrategy BORDER_INSTANCE = new PanelMarkupSourcingStrategy(
+ true);
+
// False for Panel and true for Border components.
private final boolean allowWicketComponentsInBodyMarkup;
Review Comment:
Is it a good moment to re-work this boolean to an enum ?
With an enum it would be possible to have a third/forth/... state if needed. And it is more clear than `true`/`false`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl commented on a diff in pull request #517: WICKET-6963 Singleton markup sourcing strategy (Take 2)
Posted by GitBox <gi...@apache.org>.
theigl commented on code in PR #517:
URL: https://github.com/apache/wicket/pull/517#discussion_r859999830
##########
wicket-core/src/main/java/org/apache/wicket/markup/html/panel/PanelMarkupSourcingStrategy.java:
##########
@@ -33,9 +33,28 @@
*/
public class PanelMarkupSourcingStrategy extends AssociatedMarkupSourcingStrategy
{
+ private static final PanelMarkupSourcingStrategy PANEL_INSTANCE = new PanelMarkupSourcingStrategy(
+ false);
+ private static final PanelMarkupSourcingStrategy BORDER_INSTANCE = new PanelMarkupSourcingStrategy(
+ true);
+
// False for Panel and true for Border components.
private final boolean allowWicketComponentsInBodyMarkup;
Review Comment:
@martin-g: I experimentally refactored this in https://github.com/theigl/wicket/pull/1. Could you take a look? I couldn't think of a good name for the new enum. I'm not sure this refactoring is really worth it.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl merged pull request #517: WICKET-6963 Singleton markup sourcing strategy (Take 2)
Posted by GitBox <gi...@apache.org>.
theigl merged PR #517:
URL: https://github.com/apache/wicket/pull/517
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org