Re: Add onAddToPage() to Component
Hi, I still find it confusing :-/ 1) with this we will have: the constructor, onInitialize() and onAddToPage() being called as component initializers. Way too much IMO. onInitialize() and onAddToPage() are exactly the same thing at this stage. 2) onRemove() is not named onRemoveFromPage(). (I don't like the name onAddToPage()) Is it possible to redo it as onReAdd(), i.e. it will be called when the component is added to a parent and it is already initialized. Not sure but maybe all that is needed to do it is to remove the call to onAddToPage() in onInitialize(), and rename the method. My 2c. Martin Grigorov Wicket Training and Consulting https://twitter.com/mtgrigorov On Mon, Aug 18, 2014 at 11:29 AM, Carl-Eric Menzel cmen...@wicketbuch.de wrote: Hi everybody, I am currently dealing with https://issues.apache.org/jira/browse/WICKET-5265: A FencedFeedbackPanel marks the component it uses as a fence with a metadata entry, so that outer FencedFeedbackPanels do not traverse beyond that component to steal feedback messages. When an FFP is removed from the hierarchy (either by itself or as part of a subtree that is being removed), onRemove is called on it (and on all other components of that subtree). In onRemove, the FFP removes the fence mark. So far, so correct. When the FFP is added back to the page, there is no corresponding lifecycle callback that would tell it about the fact that it has been added back, so it cannot recreate the fence mark. This leads to outer FFPs stealing the messages. This adding the same component back is not a rare operation: It is for example what the Wizard component does with its WizardStep panels. They get removed and added all the time. I tried working around this by using onConfigure and/or onBeforeRender. In the really simple cases that works, but there can be more complicated component hierarchies. Even in a simple wizard the way Wicket traverses the tree and calls these two events can't guarantee that the inner FFP's onConfigure is called before the outer FFP tries to get the messages. I have uploaded a quickstart to the WICKET-5265 ticket to demonstrate this. Using a filter to prevent already rendered messages from being rendered again does not help, because then it is only the outer panel that shows them, not the inner one where they should be. martin-g suggested using onInitialize, but that does not work. onInitialize is called only once over the entire lifetime of the component, when it is first added to a page, and never again. It doesn't matter whether one sees it as basically a delayed constructor that allows calling overridden subclass methods, or as an indication of hey, your page is here for the first time. The problem is that it is by definition only called once. The remove-and-add situation is not covered. onRemove is called every time a component (or a parent) is removed from the page hierarchy, so components can clean up after themselves. I think that just like behaviors have bind() and unbind(), components should have a kind of onAdd() to complement onRemove(). I have an implementation for this that is tested and works. With this, the problem described above can be elegantly solved. I named the method onAddToPage to make it more clear when it will be called and to reduce the chance for a name collision. I have defined it as follows: onAddToPage is called whenever a component is added to a parent in a way that connects it to the Page instance, but not before onInitialize has been called. For example: - a component gets added to the page after it is constructed 1. onInitialize 2. onAddToPage - a component is removed from the page and then added again 1. onAddToPage - a component is added to a container that is already added to the page 1. onInitialize (if necessary) 2. onAddToPage - a component is added to a container that is *not yet* added to the page 1. nothing is called! When the container is added to the page, its onAddToPage is called, and recursively for all its children, just like all the other lifecycle events. I think this is quite well-defined. martin-g commented there might be confusion with onInitialize. The javadoc of onInitialize says: This method is meant to be used as an alternative to initialize components. Usually the component's constructor is used for this task, but sometimes a component cannot be initialized in isolation, it may need to access its parent component or its markup in order to fully initialize. This method is invoked once per component's lifecycle when a path exists from this component to the {@link Page} thus providing the component with an atomic callback when the component's environment is built out. This is reasonably well-defined, though it completely ignores the fact that onRemove might happen. Also, it only says that onInitialize will be called sometime before
Re: git commit: re-create fence mark after remove and re-add
On Mon, Aug 18, 2014 at 11:50 AM, cmen...@apache.org wrote: Repository: wicket Updated Branches: refs/heads/WICKET-5265 [created] 9c60d34e3 Please use WICKET-5265 in the commit message It is used to link the commit with the ticket (the auto comments in the tickets) and for debugging later (e.g. 3 years later) why a change has been made Please amend the commit before merging this branch to wicket-6.x/master Thanks! re-create fence mark after remove and re-add Project: http://git-wip-us.apache.org/repos/asf/wicket/repo Commit: http://git-wip-us.apache.org/repos/asf/wicket/commit/9c60d34e Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/9c60d34e Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/9c60d34e Branch: refs/heads/WICKET-5265 Commit: 9c60d34e3b5e7e0c1702e7fd504dbeac8749959a Parents: 70b7327 Author: Carl-Eric Menzel cmen...@apache.org Authored: Mon Aug 18 11:50:15 2014 +0200 Committer: Carl-Eric Menzel cmen...@apache.org Committed: Mon Aug 18 11:50:15 2014 +0200 -- .../wicket/feedback/FencedFeedbackPanel.java| 70 ++-- 1 file changed, 49 insertions(+), 21 deletions(-) -- http://git-wip-us.apache.org/repos/asf/wicket/blob/9c60d34e/wicket-core/src/main/java/org/apache/wicket/feedback/FencedFeedbackPanel.java -- diff --git a/wicket-core/src/main/java/org/apache/wicket/feedback/FencedFeedbackPanel.java b/wicket-core/src/main/java/org/apache/wicket/feedback/FencedFeedbackPanel.java index f7df59e..042a8e7 100644 --- a/wicket-core/src/main/java/org/apache/wicket/feedback/FencedFeedbackPanel.java +++ b/wicket-core/src/main/java/org/apache/wicket/feedback/FencedFeedbackPanel.java @@ -29,13 +29,13 @@ import org.apache.wicket.markup.html.panel.FeedbackPanel; * the nesting of these panels to work correctly without displaying the same feedback message twice. * A constructor that does not takes a fencing component creates a catch-all panel that shows * messages that do not come from inside any fence or from the {@link Session}. - * + * p/ * h2IN DEPTH EXPLANATION/h2 * p * It is often very useful to have feedback panels that show feedback that comes from inside a * certain container only. For example given a page with the following structure: * /p - * + * p/ * pre * Page * Form1 @@ -72,7 +72,7 @@ import org.apache.wicket.markup.html.panel.FeedbackPanel; * fencing component. There is usually one instance of such a panel at the top of the page to * display notifications of success. * /p - * + * * @author igor */ public class FencedFeedbackPanel extends FeedbackPanel @@ -89,7 +89,7 @@ public class FencedFeedbackPanel extends FeedbackPanel /** * Creates a catch-all feedback panel that will show messages not coming from any fence, * including messages coming from {@link Session} -* +* * @param id */ public FencedFeedbackPanel(String id) @@ -100,7 +100,7 @@ public class FencedFeedbackPanel extends FeedbackPanel /** * Creates a feedback panel that will only show messages if they original from, or inside of, * the {@code fence} component and not from any inner fence. -* +* * @param id * @param fence */ @@ -111,11 +111,10 @@ public class FencedFeedbackPanel extends FeedbackPanel /** * Creates a catch-all instance with a filter. -* -* @see #FencedFeedbackPanel(String) -* +* * @param id * @param filter +* @see #FencedFeedbackPanel(String) */ public FencedFeedbackPanel(String id, IFeedbackMessageFilter filter) { @@ -124,12 +123,11 @@ public class FencedFeedbackPanel extends FeedbackPanel /** * Creates a fenced feedback panel with a filter. -* -* @see #FencedFeedbackPanel(String, Component) -* +* * @param id * @param fence * @param filter +* @see #FencedFeedbackPanel(String, Component) */ public FencedFeedbackPanel(String id, Component fence, IFeedbackMessageFilter filter) { @@ -137,12 +135,17 @@ public class FencedFeedbackPanel extends FeedbackPanel this.fence = fence; if (fence != null) { - Integer count = fence.getMetaData(FENCE_KEY); - count = count == null ? 1 : count + 1; - fence.setMetaData(FENCE_KEY, count); + incrementFenceCount(); } } + private void incrementFenceCount() + { + Integer
Re: Add onAddToPage() to Component
Wait for some more opinions. Martin Grigorov Wicket Training and Consulting https://twitter.com/mtgrigorov On Mon, Aug 18, 2014 at 12:38 PM, Carl-Eric Menzel cmen...@wicketbuch.de wrote: On Mon, 18 Aug 2014 11:49:51 +0200 Martin Grigorov mgrigo...@apache.org wrote: Hi, I still find it confusing :-/ 1) with this we will have: the constructor, onInitialize() and onAddToPage() being called as component initializers. Way too much IMO. onInitialize() and onAddToPage() are exactly the same thing at this stage. At this stage, yes. 2) onRemove() is not named onRemoveFromPage(). (I don't like the name onAddToPage()) I'm not invested in the name, so that can be changed :-) Is it possible to redo it as onReAdd(), i.e. it will be called when the component is added to a parent and it is already initialized. Not sure but maybe all that is needed to do it is to remove the call to onAddToPage() in onInitialize(), and rename the method. Hm. That means that if I want to always react to being added, I'd have to do it in both onInitialize and onReAdd. However, I can see that it would potentially remove some confusion by more clearly defining the difference between the two. I'll give it a try and update the branch. Carl-Eric
Re: Add onAddToPage() to Component
One difficulty I am finding: If I do not simply fire onAdd for everything that gets added, then I have to differentiate. What do I do if I re-add a container that is already initialized, so its onReAdd should be called, but which has a new child that is not initialized? Which event should happen in the child, and how should it be propagated? Or the other way around, what if I have a new container but an already initialized child? If the container checks whether it is initialized before calling onReAdd on its children, then the child would not receive the onReAdd. This may look complicated, but I don't think it is a problem with the idea of having an onAdd or onReAdd. It is rather a consequence of the fact that Wicket allows components to be removed and added again in different compositions. You can remove a subtree and then add a new subtree that can have some of the old components. In light of this, I am in favor of having a general onAdd that is always called, and to more clearly document that onInitialize is supposed to be a delayed constructor. Carl-Eric On Mon, 18 Aug 2014 13:00:26 +0200 Martin Grigorov mgrigo...@apache.org wrote: Wait for some more opinions. Martin Grigorov Wicket Training and Consulting https://twitter.com/mtgrigorov On Mon, Aug 18, 2014 at 12:38 PM, Carl-Eric Menzel cmen...@wicketbuch.de wrote: On Mon, 18 Aug 2014 11:49:51 +0200 Martin Grigorov mgrigo...@apache.org wrote: Hi, I still find it confusing :-/ 1) with this we will have: the constructor, onInitialize() and onAddToPage() being called as component initializers. Way too much IMO. onInitialize() and onAddToPage() are exactly the same thing at this stage. At this stage, yes. 2) onRemove() is not named onRemoveFromPage(). (I don't like the name onAddToPage()) I'm not invested in the name, so that can be changed :-) Is it possible to redo it as onReAdd(), i.e. it will be called when the component is added to a parent and it is already initialized. Not sure but maybe all that is needed to do it is to remove the call to onAddToPage() in onInitialize(), and rename the method. Hm. That means that if I want to always react to being added, I'd have to do it in both onInitialize and onReAdd. However, I can see that it would potentially remove some confusion by more clearly defining the difference between the two. I'll give it a try and update the branch. Carl-Eric
Re: ETA for 6.17.0 ?
I can't promise a specific date/time, but I intent to do so this week (tonight, tomorrow, ...) The issue is project deadline and having to figure out user right management on my NAS and a shared iPhoto library (not a good idea with named users and a wrong default umask). Martijn On Mon, Aug 18, 2014 at 10:54 AM, Martin Grigorov mgrigo...@apache.org wrote: Hi Martijn, Any idea when you can cut Wicket 6.17.0/7.0.0-M3 ? Martin Grigorov Wicket Training and Consulting https://twitter.com/mtgrigorov -- Become a Wicket expert, learn from the best: http://wicketinaction.com
Re: Add onAddToPage() to Component
I don't like the whole idea of re-adding component. Wizard is the only component in Wicket distro which fails the org.apache.wicket.core.util.objects.checker.OrphanComponentChecker. I think only the data (i.e. the models) should be kept around and the components should be re-created for every view of that data when this is needed. For me Wizard component is a bad practice. Martin Grigorov Wicket Training and Consulting https://twitter.com/mtgrigorov On Mon, Aug 18, 2014 at 1:07 PM, Carl-Eric Menzel cmen...@wicketbuch.de wrote: One difficulty I am finding: If I do not simply fire onAdd for everything that gets added, then I have to differentiate. What do I do if I re-add a container that is already initialized, so its onReAdd should be called, but which has a new child that is not initialized? Which event should happen in the child, and how should it be propagated? Or the other way around, what if I have a new container but an already initialized child? If the container checks whether it is initialized before calling onReAdd on its children, then the child would not receive the onReAdd. This may look complicated, but I don't think it is a problem with the idea of having an onAdd or onReAdd. It is rather a consequence of the fact that Wicket allows components to be removed and added again in different compositions. You can remove a subtree and then add a new subtree that can have some of the old components. In light of this, I am in favor of having a general onAdd that is always called, and to more clearly document that onInitialize is supposed to be a delayed constructor. Carl-Eric On Mon, 18 Aug 2014 13:00:26 +0200 Martin Grigorov mgrigo...@apache.org wrote: Wait for some more opinions. Martin Grigorov Wicket Training and Consulting https://twitter.com/mtgrigorov On Mon, Aug 18, 2014 at 12:38 PM, Carl-Eric Menzel cmen...@wicketbuch.de wrote: On Mon, 18 Aug 2014 11:49:51 +0200 Martin Grigorov mgrigo...@apache.org wrote: Hi, I still find it confusing :-/ 1) with this we will have: the constructor, onInitialize() and onAddToPage() being called as component initializers. Way too much IMO. onInitialize() and onAddToPage() are exactly the same thing at this stage. At this stage, yes. 2) onRemove() is not named onRemoveFromPage(). (I don't like the name onAddToPage()) I'm not invested in the name, so that can be changed :-) Is it possible to redo it as onReAdd(), i.e. it will be called when the component is added to a parent and it is already initialized. Not sure but maybe all that is needed to do it is to remove the call to onAddToPage() in onInitialize(), and rename the method. Hm. That means that if I want to always react to being added, I'd have to do it in both onInitialize and onReAdd. However, I can see that it would potentially remove some confusion by more clearly defining the difference between the two. I'll give it a try and update the branch. Carl-Eric
Re: Add onAddToPage() to Component
On Mon, 18 Aug 2014 13:30:03 +0200 Martin Grigorov mgrigo...@apache.org wrote: I don't like the whole idea of re-adding component. Wizard is the only component in Wicket distro which fails the org.apache.wicket.core.util.objects.checker.OrphanComponentChecker. I think only the data (i.e. the models) should be kept around and the components should be re-created for every view of that data when this is needed. For me Wizard component is a bad practice. I'm not convinced it is entirely bad, but I can see that it is at least not optimal. However, Wicket does not prohibit it and so I think we should make sure it doesn't break in surprising ways. That said, I don't want to conflate this with the problems of Wizard too much, because I still think we should have a complement to onRemove, whether we're looking at Wizard or not, just to be consistent. I had a chat with Martin on IRC, he had a very good idea on how to simplify onAddToPage to a more clearly defined onReAdd. Thank you Martin! I've pushed an update including tests to the WICKET-5677 branch. The much simpler implementation is now not much more than an else branch in Component#fireInitialize. If the component is already initialized, it doesn't call onInitialize, but rather onReAdd. This way we have a clear distinction between the two callbacks and we don't need a second tree traversal, since this just uses the one that is there for onInitialize already. Martin and everyone else, what do you think? Carl-Eric