very interesting!!
i think 50ms on a 1GHz+ machine is actually pretty slow... you can seek
and read on a disk in that time. also, serialization (which is really
the only way to clone something easily in java) is notoriously thorny.
but this approach may still be worthwhile (btw, thanks for finding those
serialization problems! i think the good changes there ought to be
checked into the main branch... has someone done this?).
some issues to think about:
are there model problems with this? if the user inncocently creates
BookEditPage, when this copy happens, the new page will no longer be
editing the same book model. we'd at least need to document this
behavior. furthermore, if the copying doesn't /always/ happen (it
depends on what components they use), things could feel a bit
inconsistent to the user. sometimes it's the same page and sometimes
its a copy. is there some simple thing we can tell people here?
in terms of hibernate, jdo and other frameworks, is it going to be a
problem to clone models like this?
also, detaching a model like the code below could be really expensive
because reattaching might involve a database query, for example. should
we even detach the model? if we don't is hibernate going to be happy?
i suspect the answer is that we don't need to do the detach, actually.
isn't hibernate designed to deal with this?
another issue is that detaching the model could cause it to change
unexpectedly when its reattached. but that's a problem with our
detachable model idea in general. wicket sessions are only guaranteed
to cluster accurately if the attach/detach is somehow isolated from
database changes.
i think a better name for savePageCopyOnStructureChange() might be
wantPageCloned(). also, i don't understand why
savePageCopyOnStructureChange() is in MarkupContainer. i think the
implementation of this is going to have to be at the page level because
there may actually be /more than one component/ on the page (for
example, two tab components or some complex dynamic page or wizard) that
want to trigger cloning at the same time. i'd think we'd need to ask
every component on the page if it wants the page copied and then do it
just once. so maybe it should be boolean Component.wantPageCloned() and
the logic for onStructureChange would then migrate to Page, which makes
some good sense doesn't it?
clearRendering() could use a comment since the reason for its existence
is fairly involved. also, doesn't the container version of
clearRendering need to recurse using visitChildren?
the fix to choicelist should definitely go in the main branch if it
hasn't already. has someone done this?
finally, maybe we should consider keeping the current stale-detection
functionality via an ApplicationSetting and have the option default to
this (hopefully) idiot-proof transparent copying thing... that way
things work more often, but users who would rather not pay the cost of
serialization OR deal with pagefactory could get the current
functionality. i think this might be worth saving. we may get a lot of
criticism over the performance of cloning via serialization.
thoughts? replies?
jon
Eelco Hillenius wrote:
The branch is named TRY_REPLACE_AND_COPY.
I refatored it a bit and introduced IPageCopyFactory, so that people
can implement the copying strategy they want. Also, the copying is not
done when the page is marked stale, and I added protected method
MarkupContainer.savePageCopyOnStructureChange that can be overriden
when you don't want copying to happen at all. For example this is the
case with FeebackPanel that removes all components on each render, but
as they are read only components (no inputs, no links etc) anyway, we
don't want the copying happen here.
Please everyone, take a look at the branch and try it out. We (Johan,
Maurice and I) think we finally have a complete solution for the
backbutton problem (indeed for the application Maurice and I are
working on, the problems are gone now).
Note that these problems were not only there for the replace method,
which you can still argue about, but also about the removeAll method.
Sure, you could mark a page stale when you did a removeAll, but I had
a thousand times that our analists forgot not to use the back button
as we instructed them :) and they ended up with a stale data page
without understanding why.
Eelco
Johan Compagner wrote:
Hi,
in the branch we just checked in we implemented a first cut of what
we think works great
and exactly what it should do when using replace (or later on also
with add or remove).
In the MarkupContainer.replace() method we now do this first:
Page page = (Page)findParent(Page.class);
if(page != null && page.getRendering() > 0)
{
page.visitChildren(Component.class, new IVisitor()
{
public Object component(final Component component)
{
component.detachModel();
return CONTINUE_TRAVERSAL;
}
});
// first make a copy of the complete page..
try
{
ByteArrayOutputStream out = new ByteArrayOutputStream
();
ObjectOutputStream oout = new ObjectOutputStream (out);
oout.writeObject (page);
ObjectInputStream in = new
ObjectInputStream ( new ByteArrayInputStream (out.toByteArray ()));
Page copy = (Page)in.readObject ();
getSession().addPage(page);
getSession().setPage(copy);
page.clearRendering();
}
catch (Exception e)
{
log.error("Renderer", e);
}
}
So we first test if this page already rendered, if this is the case
then normally the replace couldn't happen.
But what we do then is create a copy.
That creating of a copy now happens with Serialization. But we could
implement a Factory so that a developer can choose what ever he wants.
Like Copy Constructor, or even Clone...
Then when the copy is created that copy is being set as it was the
current page for the cache (session.setPage)
and the current page itself is being added to the session and the
rendering count is cleared so that it looks like a completely new
page.
So if you selected another tab and then go back with the back button.
the page can still be found with the exact same state (the one of the
copy)
By testing this, we found some serialization errors in wicket!
Some like Page.session that is not transient (it should be and
getSession() of page should test for null and if null then do
Session.get()) are easy to fix.
But others and those map on the ImageResouce/IResouce (like
UrlResource) are harder. Those are all not serializeable and
currently we are searching for the best way to make those
serializeable . Any ideas?
But the how it works now is really great. It is completely
transparent for the developer.
And can also be used for ListView.removeAll(). Then exactly the same
should happen. right before the remove all make a copy of the current
page
and use that copy for the cache so that a back button will map on the
right listview..
If the usage of serialization is to slow or eats to much resources
then a developer should be able to override this behavior and make use
of its own implementation by using for example a copy constructor..
This is ofcourse faster..But the developer has then much more work
todo.
We already timed the serialization approache and it is only slow the
very first time you serialize a page (1.5s) but all the following
calls are 50-60ms
for very complex pages.
johan
-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real
users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
_______________________________________________
Wicket-develop mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/wicket-develop
-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real
users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
_______________________________________________
Wicket-develop mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/wicket-develop
-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
_______________________________________________
Wicket-develop mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/wicket-develop
