Re: [Wicket-user] Lifecycle issue with getVariation
1.2.6 we no longer need bodycontainer in 1.3 because we have a much more elegant system for handling such things -igor On 3/31/07, Chris Colman [EMAIL PROTECTED] wrote: Java objects construct the way that they do and we use Java object constructors because we like that simplicity. Your bug is reported and will be fixed. Any time schedule for that? the fix is in svn -igor Excellent news! What will be the first public release that includes it - 1.2.x or 1.3.x? - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV ___ Wicket-user mailing list Wicket-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/wicket-user - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV___ Wicket-user mailing list Wicket-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/wicket-user
Re: [Wicket-user] Lifecycle issue with getVariation
can you show us your code?
...
ah, sorry. this is the code i take it. just not in response to my
original post (at least on nabble).
There is possibly an even simpler solution using onAttach - overriding
it in WebPage:
onAttach()
{
commonInit();
init();
}
public void init()
{
// developers can override this in their own pages to perform
// any post construction initialization
}
With this solution RequestCycle and BookmarkablePageRequestTarget don't
need to be touched.
Possible issues with the onAttach option:
1. The commonInit might not get called until a little later than it does
with the original option.
2. Users still have to remember to call super.onAttach in their
overridden handlers though this could be rectified by creating a new
method internalOnAttach that only the framework calls:
public void internalOnAttach()
{
if ( !inited )
commonInit();
onAttach();
}
For some reason the first option 'feels more elegant' because it remains
independent of the 'attach' phase of the lifecycle. There may be issues
with coupling the init and the attach phases of the lifecycle together
like that.
-
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV
___
Wicket-user mailing list
Wicket-user@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/wicket-user
Re: [Wicket-user] Lifecycle issue with getVariation
Okay, here's my opinion: People have been using Wicket for years now and this is the first bug of this type I have heard of. I am very reluctant attempt any sort of generic framework-level fix to the semantics of Java object construction (regardless of how anyone feels about the practices defined by the JLS). All OO languages share similar semantics of object construction and the same issues arise when attempting to use partially constructed objects in any of them so it's not an issue specific to Java object construction. It's a 20 year old problem that has been solved time and time again in many different OO languages and different frameworks by using a two phase initialization pattern but if you've found a way to fix the getVariation issue without resorting to a two phase initialization pattern then that's great - let me at it!!! Like I said, that's the only issue I need to have fixed right now. Java objects construct the way that they do and we use Java object constructors because we like that simplicity. Your bug is reported and will be fixed. Any time schedule for that? Back in my day we didn't need Tapestry, or Wicket or WebObjects, or even Servlets, or for that matter Java! We just coded in plain assembly language. And before that we had to just type in 1's and 0's. Sometimes we didn't even have 1's. I once had to code for an entire month, barefoot, in the dead of winter, using just 0's... but you didn't hear me complaining. - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV ___ Wicket-user mailing list Wicket-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/wicket-user
Re: [Wicket-user] Lifecycle issue with getVariation
On 3/30/07, Chris Colman [EMAIL PROTECTED] wrote: Java objects construct the way that they do and we use Java object constructors because we like that simplicity. Your bug is reported and will be fixed. Any time schedule for that? the fix is in svn -igor - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV___ Wicket-user mailing list Wicket-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/wicket-user
Re: [Wicket-user] Lifecycle issue with getVariation
can you show us your code? Chris Colman wrote: there is a reason why some of the stuff is done where it is. now, I believe that with the minor change that I have made everything is still being done in the exact same order so there should be no consequences of this change. It's just that all the stuff done in commonInit is now being slightly deferred so that it uses completely constructed objects instead of partially constructed objects - that can't be a bad thing can it? I've certainly had no trouble with my page creation and usage with this change - in fact things that I couldn't get to work before now work perfectly! Source changes: I've taken a more inclusive, simpler approach that changes RequestCycle and BookmarkablePageRequestTarget instead of changing the DefaultPageFactory. This means absolutely no explicit calling of the commonInit method by users. I had to change 3 pages in the test cases so that they do their init in commonInit instead of the constructor and all test cases now succeed. I can make available a copy of my modified 1.2.5 source with all changes clearly marked if any one is interested. Java is interesting in that virtual methods appear to 'semi' work on partially constructed objects (which can make you think everything is fine). C++ on the other hand did not allow the virtual function mechanism to work properly until objects were completely constructed. So prior to constructor completion, for non abstract methods, C++ just calls the current class' method instead of the most derived class' method and, for abstract methods, it could result in the strange but extremely useful 'call of pure virtual (abstract) function' error. When you got that error you knew you were dancing with the devil. I think Java lets you keep on dancing... im not sure there is a reason for the markup loading for bodcontainer, we might be able to do that lazily when getbodycontainer() is called. Other frameworks (eg,. Echo2) handle this partial construction problem by automatically calling an init method on the class whenever you add it to the system after instantiation. The user doesn't need to (and shouldn't) explicitly call init() as it's done for them. They can even override init() if they want to but must first call super.init() prior to doing any of their own initialization work. In fact this is the exact pattern of initialization that you have implemented for wicket's WebApplication. It seems to make sense to me that the same pattern would apply to WebPage. juergen are you reading with us? -igor - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV ___ Wicket-user mailing list Wicket-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/wicket-user -- View this message in context: http://www.nabble.com/Lifecycle-issue-with-getVariation-tf3476789.html#a9747169 Sent from the Wicket - User mailing list archive at Nabble.com. - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV ___ Wicket-user mailing list Wicket-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/wicket-user
Re: [Wicket-user] Lifecycle issue with getVariation
ah, sorry. this is the code i take it. just not in response to my original
post (at least on nabble).
Chris Colman wrote:
i'm not sure what i think about this yet. can you show us the
exact code modifications and use cases you have coded up?
(boiled down to the important parts, if possible)
Ok, here's the changed bits you requested. Only 3 Java classes need
minor changes in the framework:
[I added the deferred init to WebPage but a more simpler approach is to
add the auto init mechanism to Page to avoid the need for casting.
Option: You could make the mechanism switchable via an application
setting so that existing code and test cases work in the same way as
they do now]
RequestCycle.java
public final void setResponsePage(final Page page)
{
// CJC 20070329 - added to auto init WebPages to allow
// getVariation to work properly
if ( page instanceof WebPage )
{
WebPage webPage = (WebPage)page;
if ( !webPage.isInitialized() )
webPage.commonInit();
}
IRequestTarget target = new PageRequestTarget(page);
setRequestTarget(target);
}
WebPage.java
// CJC 20070329 - defer commonInit till after complete
construction
/** true when the page has been initialized */
private boolean initialized = false;
/**
* Returns the initialization state. This is used by other parts
of
* the framework that will automatically call commonInit on
* uninitialized pages
*/
public boolean isInitialized()
{
return initialized;
}
public void commonInit()
{
// CJC 20070329 - defer commonInit till after
// complete construction
initialized = true;
...
}
+remove the commonInit calls from WebPage constructors
BookmarkablePageRequestTarget.java
private final Page getPage(RequestCycle requestCycle)
{
if (page == null pageClass != null
!requestCycle.getRedirect())
{
page = newPage(pageClass, requestCycle);
// CJC 20070329 - added to auto init WebPages to
allow
// getVariation to work properly
if ( page instanceof WebPage )
{
WebPage webPage = (WebPage)page;
if ( !webPage.isInitialized() )
webPage.commonInit();
}
}
return page;
}
In terms of use cases they're basically our own app plus all the wicket
test cases. Three test cases required initialization to be performed in
an overridden commonInit instead of the constructor to work - which is
standard for most OO frameworks of any kind, not just UI frameworks
(MFC::createWindow, OWL::SetupWindow) - you just don't want to be
playing with partially constructed objects.
also, it would be good if you could explain how this solves any
problems
other than getVariation().
getVariation is the biggy at the moment for me but I'm sure as more
users adopt wicket they'll come up with similar scenarios that highlight
this problem. It's feasible to come up with scenarios that don't even
involve framework features but rather, a user's own WebPage derived
class hierarchy.
This example will cause problems doing initialization in the
constructors:
class BasePage
{
public BasePage(params)
{
addTitleComponent();
}
public void addTitleComponent()
{
calls getTitle() to get text of the title and creates a
Label
}
public abstract String getTitle();
}
class MyPage extends BasePage
{
String title = null;
public MyPage(params)
{
title = use params to lookup a particular
object in database andget its name
}
// !! this gets called before the constructor has initialized
title !!
public String getTitle() { return title; }
}
I trust that's informative enough to help you guys make a good decision
on how to deal with this but if you need anything else please let me
know.
-
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share
your
opinions on IT business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV
___
Wicket-user mailing list
Wicket-user@lists.sourceforge.net
Re: [Wicket-user] Lifecycle issue with getVariation
Okay, here's my opinion: People have been using Wicket for years now and this is the first bug of this type I have heard of. I am very reluctant attempt any sort of generic framework-level fix to the semantics of Java object construction (regardless of how anyone feels about the practices defined by the JLS). Java objects construct the way that they do and we use Java object constructors because we like that simplicity. Your bug is reported and will be fixed. Further, any other problem like this (and again, I've not heard of any) can be worked around using onAttach(). Chris Colman wrote: I have a constructor that takes parameters and then uses the parameters to set up a variable that is used in getVariation(). There appears to be a lifecycle issue here because getVariation is being called from within a base class constructor - before my constructor's code below the super(parameters) has had a chance to get called. Consequently getVariation() is called before the variable has been initialized causing a NPE. Here's the stack dump: at com.MyPage.getVariation(MyPage.java:66) at wicket.Component.getStyle(Component.java:1207) at wicket.markup.MarkupCache.markupKey(MarkupCache.java:371) at wicket.markup.MarkupCache.getMarkup(MarkupCache.java:166) at wicket.markup.MarkupCache.getMarkupStream(MarkupCache.java:106) at wicket.MarkupContainer.getAssociatedMarkupStream(MarkupContainer.java:82 7) at wicket.markup.html.WebPage.commonInit(WebPage.java:235) at wicket.markup.html.WebPage.init(WebPage.java:120) at com.sas.av.ui.wicket.templates.BasePage.init(BasePage.java:66) at com.MyPage.init(MyPage.java:83) Often in these cases it serves the framework users well if the basic construction takes place first and then, only after returning from the instantiation, further initialization is performed - initialization that might only work properly after all constructors have been fully executed. Currently it appears possible for the getStyle and getVariation methods to be invoked while a page is only semi constructed (ie., the constructor stack has not unwound as shown above). Is there a current workaround for this or is it possible to readjust the Wicket lifecycle model slightly to avoid this problem? - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV ___ Wicket-user mailing list Wicket-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/wicket-user -- View this message in context: http://www.nabble.com/Lifecycle-issue-with-getVariation-tf3476789.html#a9747403 Sent from the Wicket - User mailing list archive at Nabble.com. - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV ___ Wicket-user mailing list Wicket-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/wicket-user
Re: [Wicket-user] Lifecycle issue with getVariation
there is a reason why some of the stuff is done where it is. now, I believe that with the minor change that I have made everything is still being done in the exact same order so there should be no consequences of this change. It's just that all the stuff done in commonInit is now being slightly deferred so that it uses completely constructed objects instead of partially constructed objects - that can't be a bad thing can it? I've certainly had no trouble with my page creation and usage with this change - in fact things that I couldn't get to work before now work perfectly! Source changes: I've taken a more inclusive, simpler approach that changes RequestCycle and BookmarkablePageRequestTarget instead of changing the DefaultPageFactory. This means absolutely no explicit calling of the commonInit method by users. I had to change 3 pages in the test cases so that they do their init in commonInit instead of the constructor and all test cases now succeed. I can make available a copy of my modified 1.2.5 source with all changes clearly marked if any one is interested. Java is interesting in that virtual methods appear to 'semi' work on partially constructed objects (which can make you think everything is fine). C++ on the other hand did not allow the virtual function mechanism to work properly until objects were completely constructed. So prior to constructor completion, for non abstract methods, C++ just calls the current class' method instead of the most derived class' method and, for abstract methods, it could result in the strange but extremely useful 'call of pure virtual (abstract) function' error. When you got that error you knew you were dancing with the devil. I think Java lets you keep on dancing... im not sure there is a reason for the markup loading for bodcontainer, we might be able to do that lazily when getbodycontainer() is called. Other frameworks (eg,. Echo2) handle this partial construction problem by automatically calling an init method on the class whenever you add it to the system after instantiation. The user doesn't need to (and shouldn't) explicitly call init() as it's done for them. They can even override init() if they want to but must first call super.init() prior to doing any of their own initialization work. In fact this is the exact pattern of initialization that you have implemented for wicket's WebApplication. It seems to make sense to me that the same pattern would apply to WebPage. juergen are you reading with us? -igor - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV ___ Wicket-user mailing list Wicket-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/wicket-user
Re: [Wicket-user] Lifecycle issue with getVariation
On 3/28/07, Chris Colman [EMAIL PROTECTED] wrote: there is a reason why some of the stuff is done where it is. now, I believe that with the minor change that I have made everything is still being done in the exact same order so there should be no consequences of this change. It's just that all the stuff done in commonInit is now being slightly deferred so that it uses completely constructed objects instead of partially constructed objects - that can't be a bad thing can it? introducing a required post-construct init method is hardly a minor change. while this works well for you because you mostly use bookmarkable pages it wont hold for normal apps where bookmarkable pages are a minority. and there are in fact consequences to your change. i can no longer do getBodyContainer().addOnLoadModifier(...) from the constructor of my page. so where am i to do it now? i need to use that post-construct callback, so now there are two places where i need to initialize my component - hardly simple still. Source changes: I've taken a more inclusive, simpler approach that changes RequestCycle and BookmarkablePageRequestTarget instead of changing the DefaultPageFactory. This means absolutely no explicit calling of the commonInit method by users. I had to change 3 pages in the test cases so that they do their init in commonInit instead of the constructor and all test cases now succeed. have you also changed all other page-related request targets as well? have you built safeguards to make sure this method is atomic? the fact that you had to call this method manually in tests is proof enough for me that what you propose does not work well. Java is interesting in that virtual methods appear to 'semi' work on partially constructed objects (which can make you think everything is fine). C++ on the other hand did not allow the virtual function mechanism to work properly until objects were completely constructed. So prior to constructor completion, for non abstract methods, C++ just calls the current class' method instead of the most derived class' method and, for abstract methods, it could result in the strange but extremely useful 'call of pure virtual (abstract) function' error. When you got that error you knew you were dancing with the devil. I think Java lets you keep on dancing... yes, we are all aware of the fact that it is not a good idea to call overloaded methods from the parent's constructor. like i said, the problem is that getvariation() is called as a side effect of something done in the commoninit method - and that is what needs to be fixed. there shouldnt be a reason why we need to call getvariation()/load markup at construction time. Other frameworks (eg,. Echo2) handle this partial construction problem by automatically calling an init method on the class whenever you add it to the system after instantiation. The user doesn't need to (and shouldn't) explicitly call init() as it's done for them. They can even override init() if they want to but must first call super.init() prior to doing any of their own initialization work. yes, but in wicket we prefer constructors. they are in java for a reason :) i dont know about echo2 but in wicket components are not beans. In fact this is the exact pattern of initialization that you have implemented for wicket's WebApplication. It seems to make sense to me that the same pattern would apply to WebPage. webapplication is a managed class, so there we didnt really have a choice. webapplication also does not implement the compound pattern where such a call has to be cascaded. in short, webapplication and webpage (which is a component) are very different. other devs feel free to weigh in. i will create a jira issue for this so we can track it and think about possible solutions. -igor - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV___ Wicket-user mailing list Wicket-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/wicket-user
Re: [Wicket-user] Lifecycle issue with getVariation
https://issues.apache.org/jira/browse/WICKET-432 -igor On 3/28/07, Igor Vaynberg [EMAIL PROTECTED] wrote: On 3/28/07, Chris Colman [EMAIL PROTECTED] wrote: there is a reason why some of the stuff is done where it is. now, I believe that with the minor change that I have made everything is still being done in the exact same order so there should be no consequences of this change. It's just that all the stuff done in commonInit is now being slightly deferred so that it uses completely constructed objects instead of partially constructed objects - that can't be a bad thing can it? introducing a required post-construct init method is hardly a minor change. while this works well for you because you mostly use bookmarkable pages it wont hold for normal apps where bookmarkable pages are a minority. and there are in fact consequences to your change. i can no longer do getBodyContainer().addOnLoadModifier(...) from the constructor of my page. so where am i to do it now? i need to use that post-construct callback, so now there are two places where i need to initialize my component - hardly simple still. Source changes: I've taken a more inclusive, simpler approach that changes RequestCycle and BookmarkablePageRequestTarget instead of changing the DefaultPageFactory. This means absolutely no explicit calling of the commonInit method by users. I had to change 3 pages in the test cases so that they do their init in commonInit instead of the constructor and all test cases now succeed. have you also changed all other page-related request targets as well? have you built safeguards to make sure this method is atomic? the fact that you had to call this method manually in tests is proof enough for me that what you propose does not work well. Java is interesting in that virtual methods appear to 'semi' work on partially constructed objects (which can make you think everything is fine). C++ on the other hand did not allow the virtual function mechanism to work properly until objects were completely constructed. So prior to constructor completion, for non abstract methods, C++ just calls the current class' method instead of the most derived class' method and, for abstract methods, it could result in the strange but extremely useful 'call of pure virtual (abstract) function' error. When you got that error you knew you were dancing with the devil. I think Java lets you keep on dancing... yes, we are all aware of the fact that it is not a good idea to call overloaded methods from the parent's constructor. like i said, the problem is that getvariation() is called as a side effect of something done in the commoninit method - and that is what needs to be fixed. there shouldnt be a reason why we need to call getvariation()/load markup at construction time. Other frameworks (eg,. Echo2) handle this partial construction problem by automatically calling an init method on the class whenever you add it to the system after instantiation. The user doesn't need to (and shouldn't) explicitly call init() as it's done for them. They can even override init() if they want to but must first call super.init() prior to doing any of their own initialization work. yes, but in wicket we prefer constructors. they are in java for a reason :) i dont know about echo2 but in wicket components are not beans. In fact this is the exact pattern of initialization that you have implemented for wicket's WebApplication. It seems to make sense to me that the same pattern would apply to WebPage. webapplication is a managed class, so there we didnt really have a choice. webapplication also does not implement the compound pattern where such a call has to be cascaded. in short, webapplication and webpage (which is a component) are very different. other devs feel free to weigh in. i will create a jira issue for this so we can track it and think about possible solutions. -igor - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV___ Wicket-user mailing list Wicket-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/wicket-user
Re: [Wicket-user] Lifecycle issue with getVariation
i'm not sure what i think about this yet. can you show us the exact code modifications and use cases you have coded up? (boiled down to the important parts, if possible) also, it would be good if you could explain how this solves any problems other than getVariation(). Chris Colman wrote: Just looking at the source code for WebPage it looks like a very minor change to achieve a more flexible lifecycle initialization phase. All the WebPage constructors call private void commonInit() As you will no doubt understand this is being called before construction/instantiation has properly completed - which has serious consequences if any methods are called that rely on constructor initialization that has not yet occurred (eg., getVariation that returns a variation based on the constructor's parameter values). I think the following changes would fix this problem and allow getVariation() to work properly when it's implementation relies on parameters passed into the constructor: 1. Remove all calls to commonInit() from the WebPage constructors. 2. Make commonInit public 3. Change the DefaultPageFactory to explicitly call commonInit on any new page that it instantiates. These changes, to my thinking, are quite simple with low to no ripple effect and they allow wicket users to avoid entry into the 'world of pain' that is 'partially constructed objects'. - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV ___ Wicket-user mailing list Wicket-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/wicket-user -- View this message in context: http://www.nabble.com/Lifecycle-issue-with-getVariation-tf3476789.html#a9724558 Sent from the Wicket - User mailing list archive at Nabble.com. - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV ___ Wicket-user mailing list Wicket-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/wicket-user
Re: [Wicket-user] Lifecycle issue with getVariation
i'm not sure what i think about this yet. can you show us the
exact code modifications and use cases you have coded up?
(boiled down to the important parts, if possible)
Ok, here's the changed bits you requested. Only 3 Java classes need
minor changes in the framework:
[I added the deferred init to WebPage but a more simpler approach is to
add the auto init mechanism to Page to avoid the need for casting.
Option: You could make the mechanism switchable via an application
setting so that existing code and test cases work in the same way as
they do now]
RequestCycle.java
public final void setResponsePage(final Page page)
{
// CJC 20070329 - added to auto init WebPages to allow
// getVariation to work properly
if ( page instanceof WebPage )
{
WebPage webPage = (WebPage)page;
if ( !webPage.isInitialized() )
webPage.commonInit();
}
IRequestTarget target = new PageRequestTarget(page);
setRequestTarget(target);
}
WebPage.java
// CJC 20070329 - defer commonInit till after complete
construction
/** true when the page has been initialized */
private boolean initialized = false;
/**
* Returns the initialization state. This is used by other parts
of
* the framework that will automatically call commonInit on
* uninitialized pages
*/
public boolean isInitialized()
{
return initialized;
}
public void commonInit()
{
// CJC 20070329 - defer commonInit till after
// complete construction
initialized = true;
...
}
+remove the commonInit calls from WebPage constructors
BookmarkablePageRequestTarget.java
private final Page getPage(RequestCycle requestCycle)
{
if (page == null pageClass != null
!requestCycle.getRedirect())
{
page = newPage(pageClass, requestCycle);
// CJC 20070329 - added to auto init WebPages to
allow
// getVariation to work properly
if ( page instanceof WebPage )
{
WebPage webPage = (WebPage)page;
if ( !webPage.isInitialized() )
webPage.commonInit();
}
}
return page;
}
In terms of use cases they're basically our own app plus all the wicket
test cases. Three test cases required initialization to be performed in
an overridden commonInit instead of the constructor to work - which is
standard for most OO frameworks of any kind, not just UI frameworks
(MFC::createWindow, OWL::SetupWindow) - you just don't want to be
playing with partially constructed objects.
also, it would be good if you could explain how this solves any
problems
other than getVariation().
getVariation is the biggy at the moment for me but I'm sure as more
users adopt wicket they'll come up with similar scenarios that highlight
this problem. It's feasible to come up with scenarios that don't even
involve framework features but rather, a user's own WebPage derived
class hierarchy.
This example will cause problems doing initialization in the
constructors:
class BasePage
{
public BasePage(params)
{
addTitleComponent();
}
public void addTitleComponent()
{
calls getTitle() to get text of the title and creates a
Label
}
public abstract String getTitle();
}
class MyPage extends BasePage
{
String title = null;
public MyPage(params)
{
title = use params to lookup a particular
object in database andget its name
}
// !! this gets called before the constructor has initialized
title !!
public String getTitle() { return title; }
}
I trust that's informative enough to help you guys make a good decision
on how to deal with this but if you need anything else please let me
know.
-
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV
___
Wicket-user mailing list
Wicket-user@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/wicket-user
Re: [Wicket-user] Lifecycle issue with getVariation
On 3/28/07, Chris Colman [EMAIL PROTECTED] wrote:
i am against this because i do not think the value it brings is worth the
complexity it adds.
you can persuade me by coming up with examples that actually demonstrate the
advantage. it is common practice in wicket to use imodel to deffer lookup of
values until render-time, so your usecase below is trivial to fix.
class BasePage
{
public BasePage(params)
{
addTitleComponent();
}
public void addTitleComponent()
{
add(new Label(title, new PropertyModel(this, title));
}
public abstract String getTitle();
}
class MyPage extends BasePage
{
String title = null;
public MyPage(params)
{
title = use params to lookup a particular
object in database andget its name
}
// !! this gets called before the constructor has initialized
title !!
// no it doesnt, it will be called at render time
public String getTitle() { return title; }
}
-igor
-
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV___
Wicket-user mailing list
Wicket-user@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/wicket-user
Re: [Wicket-user] Lifecycle issue with getVariation
I believe that with the minor change that I have made everything is
still being done in the exact same order so there should be no
consequences of this change. It's just that all the stuff done in
commonInit is now being slightly deferred so that it uses completely
constructed objects instead of partially constructed objects - that
can't be a bad thing can it?
introducing a required post-construct init method is hardly a minor
change.
while this works well for you because you mostly use bookmarkable
pages it
wont hold for normal apps where bookmarkable pages are a minority.
You can easily make it a switchable option in the application settings
and default to the traditional 'possibly get weird crash because you're
working on a partially constructed object' mode then everyone's existing
code will continue to work as is.
Other people, who like their objects fully constructed before using them
so that they can successfully use polymorphism in their page classes
during initialization, can switch to 'two phase initialization' mode.
and there are in fact consequences to your change. i can no longer do
getBodyContainer().addOnLoadModifier(...) from the constructor of my
page.
so where am i to do it now? i need to use that post-construct
callback, so
now there are two places where i need to initialize my component -
hardly
simple still.
Easy if you deal with the parameters in the constructor and deal with
adding children, accessing markup etc., in the overridden init (or
commonInit) method.
In cases where people have a number of constructors they normally create
their own separate init method anyway to avoid code duplication so it's
not that bad.
For me I'd be happy to have getVariation working properly right now but
I'm just trying to do something here for the greater good of the wicket
community and the children of the future that come to wicket
(...children are our future, teach them right and let them know the
way... but I digress) Seriously thought: there's going to be other cases
of partially constructed objects causing problems as more and more
people use wicket. Better to fix it properly now than wait until it's
too late to fix it.
Source changes: I've taken a more inclusive, simpler approach that
changes RequestCycle and BookmarkablePageRequestTarget instead of
changing the DefaultPageFactory. This means absolutely no explicit
calling of the commonInit method by users. I had to change 3 pages
in
the test cases so that they do their init in commonInit instead of
the
constructor and all test cases now succeed.
have you also changed all other page-related request targets as well?
have
you built safeguards to make sure this method is atomic?
Every unit test now runs successfully so if those scenarios are covered
by the wicket test suite then I've covered them with my changes.
the fact that you had to call this method manually in tests is proof
enough for me that what you propose does not work well.
I never had to call it manually anywhere. In a few of the tests I just
overrode commonInit and did the initialization in there after calling
super.commonInit(). There's even a technique to avoid needing to do that
call also:
WebPage.commonInit()
{
// do all the regular commonInit stuff
...
// then call virtual init() - users override init() instead of
// commonInit and they don't have to call the super class method
// to ensure that the commonInit code gets executed - it already
// has been executed. If their own base classes do stuff in init
// then they can call super.init() if they desire but the
// WebPage.init() is just an empty method and does nothing.
init();
}
yes, we are all aware of the fact that it is not a good idea to call
overloaded methods from the parent's constructor. like i said, the
problem
is that getvariation() is called as a side effect of something done in
the
commoninit method - and that is what needs to be fixed. there shouldnt
be
a
reason why we need to call getvariation()/load markup at construction
time.
No one ever deliberately calls overloaded methods from parent
constructors. It always happens as an accidental side effect and will
happen in the future again I'm sure - whether in using a framework
feature or in a user's own WebPage derived classes.
Other frameworks (eg,. Echo2) handle this partial construction problem
by automatically calling an init method on the class whenever you
add it
to the system after instantiation. The user doesn't need to (and
shouldn't) explicitly call init() as it's done for them. They can
even
override init() if they want to but must first call super.init()
prior
to doing any of their own initialization work.
yes, but in wicket we prefer constructors. they are in java for a
reason
:)
I prefer them too - but only up to the point where they stop you from
using certain OO features during
Re: [Wicket-user] Lifecycle issue with getVariation
the fact remains that getvariation() call from webpage constructor is a bug
we need to fix.
further we already have listener methods you can use - onattach and ondetach
what you want can be just as easily accomplished on every component
class mypage extends webpage {
private boolean initted=false;
public void onattach() {
super.onattach();
if (!initted) {
initted=true;
// perform one time initialization
}
}
}
no need for an extra method that we need to worry about calling all over the
place.
in fact if you want create your own page subclass that does this with a
virtual init() method you can override. we already have the facilities.
as far as our own code goes until we hit a brick wall i dont think we should
introduce something like this, but then again this is just my opinion.
-igor
On 3/28/07, Chris Colman [EMAIL PROTECTED] wrote:
I believe that with the minor change that I have made everything is
still being done in the exact same order so there should be no
consequences of this change. It's just that all the stuff done in
commonInit is now being slightly deferred so that it uses completely
constructed objects instead of partially constructed objects - that
can't be a bad thing can it?
introducing a required post-construct init method is hardly a minor
change.
while this works well for you because you mostly use bookmarkable
pages it
wont hold for normal apps where bookmarkable pages are a minority.
You can easily make it a switchable option in the application settings
and default to the traditional 'possibly get weird crash because you're
working on a partially constructed object' mode then everyone's existing
code will continue to work as is.
Other people, who like their objects fully constructed before using them
so that they can successfully use polymorphism in their page classes
during initialization, can switch to 'two phase initialization' mode.
and there are in fact consequences to your change. i can no longer do
getBodyContainer().addOnLoadModifier(...) from the constructor of my
page.
so where am i to do it now? i need to use that post-construct
callback, so
now there are two places where i need to initialize my component -
hardly
simple still.
Easy if you deal with the parameters in the constructor and deal with
adding children, accessing markup etc., in the overridden init (or
commonInit) method.
In cases where people have a number of constructors they normally create
their own separate init method anyway to avoid code duplication so it's
not that bad.
For me I'd be happy to have getVariation working properly right now but
I'm just trying to do something here for the greater good of the wicket
community and the children of the future that come to wicket
(...children are our future, teach them right and let them know the
way... but I digress) Seriously thought: there's going to be other cases
of partially constructed objects causing problems as more and more
people use wicket. Better to fix it properly now than wait until it's
too late to fix it.
Source changes: I've taken a more inclusive, simpler approach that
changes RequestCycle and BookmarkablePageRequestTarget instead of
changing the DefaultPageFactory. This means absolutely no explicit
calling of the commonInit method by users. I had to change 3 pages
in
the test cases so that they do their init in commonInit instead of
the
constructor and all test cases now succeed.
have you also changed all other page-related request targets as well?
have
you built safeguards to make sure this method is atomic?
Every unit test now runs successfully so if those scenarios are covered
by the wicket test suite then I've covered them with my changes.
the fact that you had to call this method manually in tests is proof
enough for me that what you propose does not work well.
I never had to call it manually anywhere. In a few of the tests I just
overrode commonInit and did the initialization in there after calling
super.commonInit(). There's even a technique to avoid needing to do that
call also:
WebPage.commonInit()
{
// do all the regular commonInit stuff
...
// then call virtual init() - users override init() instead of
// commonInit and they don't have to call the super class method
// to ensure that the commonInit code gets executed - it already
// has been executed. If their own base classes do stuff in init
// then they can call super.init() if they desire but the
// WebPage.init() is just an empty method and does nothing.
init();
}
yes, we are all aware of the fact that it is not a good idea to call
overloaded methods from the parent's constructor. like i said, the
problem
is that getvariation() is called as a side effect of something done in
the
commoninit method - and that is what needs to be fixed. there shouldnt
be
a
reason why we need to call getvariation()/load markup at
Re: [Wicket-user] Lifecycle issue with getVariation
the fact remains that getvariation() call from webpage constructor is
a
bug we need to fix.
Yeah, that's my numero uno motivation at this point also.
further we already have listener methods you can use - onattach and
ondetach
what you want can be just as easily accomplished on every component
class mypage extends webpage {
private boolean initted=false;
public void onattach() {
super.onattach();
if (!initted) {
initted=true;
// perform one time initialization
}
}
}
Wow, if I had used onAttach I could have made my source changes a whole
lot quicker (maybe) and only limited to one class:
WebApp.java
private boolean initted=false;
protected void init() {
}
private void commonInit() {
...
init(); // override this to add children etc.,
}
public void onattach() {
super.onattach();
if (!initted) {
initted=true;
// perform one time initialization
commonInit();
}
}
+remove calls to commonInit from WebPage constructors.
-
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV
___
Wicket-user mailing list
Wicket-user@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/wicket-user
Re: [Wicket-user] Lifecycle issue with getVariation
On 3/28/07, Chris Colman [EMAIL PROTECTED] wrote:
Easy if you deal with the parameters in the constructor and deal with
adding children, accessing markup etc., in the overridden init (or
commonInit) method.
In cases where people have a number of constructors they normally create
their own separate init method anyway to avoid code duplication so it's
not that bad.
yes but the init that constructors forward to has a custom sig.
what i dont like is code like this
public class MyPage extends WebPage {
private IModel user;
private IModel role;
public MyPage(IModel user, IModel role) {
this.user=user;
this.role=role;
}
public void init() {
add(new ViewUserPanel(user));
add(new ViewRolePanel(role));
}
// maybe not strictly necessary, but i like to detach all models that are
members anyways
public void onDetach() {
user.detach();
role.detach();
}
}
instead of
public class MyPage extends WebPage {
public MyPage(IModel user, IModel role) {
add(new ViewUserPanel(user));
add(new ViewRolePanel(role));
}
}
-igor
-
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV___
Wicket-user mailing list
Wicket-user@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/wicket-user
Re: [Wicket-user] Lifecycle issue with getVariation
Just looking at the source code for WebPage it looks like a very minor change to achieve a more flexible lifecycle initialization phase. All the WebPage constructors call private void commonInit() As you will no doubt understand this is being called before construction/instantiation has properly completed - which has serious consequences if any methods are called that rely on constructor initialization that has not yet occurred (eg., getVariation that returns a variation based on the constructor's parameter values). I think the following changes would fix this problem and allow getVariation() to work properly when it's implementation relies on parameters passed into the constructor: 1. Remove all calls to commonInit() from the WebPage constructors. 2. Make commonInit public 3. Change the DefaultPageFactory to explicitly call commonInit on any new page that it instantiates. These changes, to my thinking, are quite simple with low to no ripple effect and they allow wicket users to avoid entry into the 'world of pain' that is 'partially constructed objects'. - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV ___ Wicket-user mailing list Wicket-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/wicket-user
Re: [Wicket-user] Lifecycle issue with getVariation
what about pages that are constructed with the new operator? the users then have to explicitly call commoninit() from their constructors? i think that is horrible. what we need to do is somehow delay the getvariation() until render time. -igor On 3/27/07, Chris Colman [EMAIL PROTECTED] wrote: Just looking at the source code for WebPage it looks like a very minor change to achieve a more flexible lifecycle initialization phase. All the WebPage constructors call private void commonInit() As you will no doubt understand this is being called before construction/instantiation has properly completed - which has serious consequences if any methods are called that rely on constructor initialization that has not yet occurred (eg., getVariation that returns a variation based on the constructor's parameter values). I think the following changes would fix this problem and allow getVariation() to work properly when it's implementation relies on parameters passed into the constructor: 1. Remove all calls to commonInit() from the WebPage constructors. 2. Make commonInit public 3. Change the DefaultPageFactory to explicitly call commonInit on any new page that it instantiates. These changes, to my thinking, are quite simple with low to no ripple effect and they allow wicket users to avoid entry into the 'world of pain' that is 'partially constructed objects'. - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV ___ Wicket-user mailing list Wicket-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/wicket-user - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV___ Wicket-user mailing list Wicket-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/wicket-user
Re: [Wicket-user] Lifecycle issue with getVariation
I went ahead and implemented these changes in my copy of the wicket source code and now I can specify a variation at the Page level (never been able to do that before!) without causing a NPE. It works very nicely indeed. what about pages that are constructed with the new operator? I mainly use bookmarkable pages and so my pages are constructed (and now commonInit'd) by the DefaultPageFactory - result: I didn't have to change any of my own code to get my getVariation working properly. I get the feeling that error pages don't get instantiated via the DefaultPageFactory and so I will need to find out where they are new'ed so I can change the wicket source to call commonInit on them also. The HelloWorld example just returns the page Class to use - ie., doesn't do new MyPage(). So presumably the DefaultPageFactory is doing the initialization for that one also. So HelloWorld would work ok with the improved initialization lifecycle. the users then have to explicitly call commoninit() from their constructors? Not necessarily. After you create a page explicitly yourself don't you have to hand it on to 'something else' before you can use it to render a page? That 'something else' could call a boolean isInitialized() method and call commonInit if not already initialized. There's plenty of ways to make this work well so users don't have to call commonInit themselves. i think that is horrible. It would be horrible if users had to do it explicitly but I'm sure there are ways (above example) to make it automagically done for the user so they don't have to call commonInit themselves. what we need to do is somehow delay the getvariation() until render time. Well, that sounds like it will fix the getVariation problem but how many other problems are going to give us a world of pain in the future because of the use of partially constructed WebPage objects? The other problem is that commonInit is doing a whole lot of stuff with the markup and it needs to know the variation to get the correct markup - I don't see how you can defer getVariation unless you defer the whole markup analysis code that follows the call to getVariation as well... I ended up deferring the whole commonInit call but you might be able to carve out a smaller portion of code to defer. - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV ___ Wicket-user mailing list Wicket-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/wicket-user
Re: [Wicket-user] Lifecycle issue with getVariation
there is a reason why some of the stuff is done where it is. now, im not sure there is a reason for the markup loading for bodcontainer, we might be able to do that lazily when getbodycontainer() is called. juergen are you reading with us? -igor On 3/27/07, Chris Colman [EMAIL PROTECTED] wrote: I went ahead and implemented these changes in my copy of the wicket source code and now I can specify a variation at the Page level (never been able to do that before!) without causing a NPE. It works very nicely indeed. what about pages that are constructed with the new operator? I mainly use bookmarkable pages and so my pages are constructed (and now commonInit'd) by the DefaultPageFactory - result: I didn't have to change any of my own code to get my getVariation working properly. I get the feeling that error pages don't get instantiated via the DefaultPageFactory and so I will need to find out where they are new'ed so I can change the wicket source to call commonInit on them also. The HelloWorld example just returns the page Class to use - ie., doesn't do new MyPage(). So presumably the DefaultPageFactory is doing the initialization for that one also. So HelloWorld would work ok with the improved initialization lifecycle. the users then have to explicitly call commoninit() from their constructors? Not necessarily. After you create a page explicitly yourself don't you have to hand it on to 'something else' before you can use it to render a page? That 'something else' could call a boolean isInitialized() method and call commonInit if not already initialized. There's plenty of ways to make this work well so users don't have to call commonInit themselves. i think that is horrible. It would be horrible if users had to do it explicitly but I'm sure there are ways (above example) to make it automagically done for the user so they don't have to call commonInit themselves. what we need to do is somehow delay the getvariation() until render time. Well, that sounds like it will fix the getVariation problem but how many other problems are going to give us a world of pain in the future because of the use of partially constructed WebPage objects? The other problem is that commonInit is doing a whole lot of stuff with the markup and it needs to know the variation to get the correct markup - I don't see how you can defer getVariation unless you defer the whole markup analysis code that follows the call to getVariation as well... I ended up deferring the whole commonInit call but you might be able to carve out a smaller portion of code to defer. - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV ___ Wicket-user mailing list Wicket-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/wicket-user - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.phpp=sourceforgeCID=DEVDEV___ Wicket-user mailing list Wicket-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/wicket-user
