Re: Why you should not override isVisible

2009-01-16 Thread Scott Swank
gt;> On Fri, Jan 16, 2009 at 12:53 AM, wrote: >> >>> >>> What's taking so long in your isVisible() method? >>> >>> >>> The model object should be cached, and is isPositive() so expensive? >>> >>> >>> Sven >>> >>

Re: Why you should not override isVisible

2009-01-16 Thread Sven Meier
1.09 02:06 Uhr An: users@wicket.apache.org Betreff: Re: Why you should not override isVisible We have implemented this, perhaps a dozen times or more across our application. For example, there are several payment options whose relevance is determined by whether the customer owes any money on th

Re: Re: Why you should not override isVisible

2009-01-16 Thread Scott Swank
; Von: Scott Swank > Gesendet: 16.01.09 02:06 Uhr > An: users@wicket.apache.org > Betreff: Re: Why you should not override isVisible > > > > We have implemented this, perhaps a dozen times or more across our > > application. For example, there are several payment optio

Re: Why you should not override isVisible

2009-01-16 Thread Erik van Oosten
Created an issue: http://issues.apache.org/jira/browse/WICKET-2025 Regards, Erik. Erik van Oosten wrote: Martijn, I just went through the source (1.4-rc1) to trace a detach manually and find suspect callers of isVisible. I found none during detach, but I did find one call to isVisible /

Re: Why you should not override isVisible

2009-01-16 Thread Erik van Oosten
Sorry Sven, You of course meant to say: /If/ isVisible would no longer be called after detach /then/ it would be possible to do the caching yourself (as you can use detach to clear the cache). /If/ you can cache yourself /then/ Wicket does not need to cache the result of isVisible. Althou

Re: Why you should not override isVisible

2009-01-16 Thread Erik van Oosten
Please don't turn the logic around. Caching is only needed because isVisible can be a performance hit. /If/ you want caching /then/ isVisible should not be called after detach as detach is needed to clear the cache. Regards, Erik. s...@meiers.net wrote: Ok, IMHO it's a bug that wicket c

RE: Why you should not override isVisible

2009-01-16 Thread Hoover, William
+1 -Original Message- From: s...@meiers.net [mailto:s...@meiers.net] Sent: Friday, January 16, 2009 7:47 AM To: users@wicket.apache.org Subject: Re: Why you should not override isVisible Ok, IMHO it's a bug that wicket calls isVisible() after detachment. Thus caching isVi

Re: Why you should not override isVisible

2009-01-16 Thread sven
Ok, IMHO it's a bug that wicket calls isVisible() after detachment. Thus caching isVisible() is not needed. Sven - Ursprüngliche Nachricht - Von: Michael Sparer Gesendet: 16.01.09 11:20 Uhr An: users@wicket.apache.org Betreff: Re: Re: Why you should not override isVi

Re: Re: Why you should not override isVisible

2009-01-16 Thread Michael Sparer
method? > > > The model object should be cached, and is isPositive() so expensive? > > > Sven > > > > > > > - Ursprüngliche Nachricht - > Von: Scott Swank > Gesendet: 16.01.09 02:06 Uhr > An: users@wicket.apache.org > Betreff: Re: Why yo

Re: Why you should not override isVisible

2009-01-16 Thread Erik van Oosten
Thanks DVD, we already established that it is called more then once. This discussion is talking about: 1. caching the "visible" value during the render phase to prevent the potentially large performance hit of multiple invocations to isVisible 2. preventing calls to isVisible during or

Re: Why you should not override isVisible

2009-01-16 Thread DVD
. Sorry I could not provide sample to reproduce this. but I did see it happening. - Original Message - From: "Erik van Oosten" To: Sent: Friday, January 16, 2009 4:18 AM Subject: Re: Why you should not override isVisible Martijn, I just went through the source (1.4-rc1)

Re: Why you should not override isVisible

2009-01-16 Thread Erik van Oosten
Martijn, I just went through the source (1.4-rc1) to trace a detach manually and find suspect callers of isVisible. I found none during detach, but I did find one call to isVisible /after/ detach. A simple run confirmed this. The call to isVisible /after/ detach can be found in method Compon

Re: Re: Why you should not override isVisible

2009-01-16 Thread sven
What's taking so long in your isVisible() method? The model object should be cached, and is isPositive() so expensive? Sven - Ursprüngliche Nachricht - Von: Scott Swank Gesendet: 16.01.09 02:06 Uhr An: users@wicket.apache.org Betreff: Re: Why you should not override isVi

Re: Why you should not override isVisible

2009-01-15 Thread Martin Makundi
I consider the current behavior a perfect default. It just might be useful to have a built in caching mechanism that you can turn on on a per-component-instance -basis. Whether it is worth the debate is another question, because it is pretty low-cost to implement one for your particular need. ** M

Re: Why you should not override isVisible

2009-01-15 Thread Scott Swank
We have implemented this, perhaps a dozen times or more across our application. For example, there are several payment options whose relevance is determined by whether the customer owes any money on their purchase (e.g. as opposed to using a gift card). These "total the order and determine visibi

Re: Why you should not override isVisible

2009-01-15 Thread Jonathan Locke
oh i suppose you also need to reset the value in onBeforeRender(). it's a small pain, but how often does this really become a quantifiable problem and not just a worry? Jonathan Locke wrote: > > > sure, that's the clean way to do it, but it comes at the expense of > possibly breaking user cod

Re: Why you should not override isVisible

2009-01-15 Thread Jonathan Locke
sure, that's the clean way to do it, but it comes at the expense of possibly breaking user code by surprise. i'm not sure how big of a deal this is. i've heard people talk about it, but i'd be interested in some examples of how performance of this method has been a problem for people. i've neve

Re: Why you should not override isVisible

2009-01-15 Thread Scott Swank
My idea what an inversion of that one: Add a method to Component, such as isVisibleInternal() [no I don't love the name] that would cache the results of isVisible(). Then all code that currently calls isVisible() would be changed to call isVisibleInternal() instead. Someone who really wanted non

Re: Why you should not override isVisible

2009-01-15 Thread Jonathan Locke
well, one simple design that would avoid the reuse problem is: Boolean Component#isCachedVisible() { return null; } then override to use visibility caching and return true or false. if you don't override you get the current functionality. of course you need two more bits in Component to suppor

Re: Why you should not override isVisible

2009-01-15 Thread Martijn Dashorst
A short test didn't uncover any calls to isVisible during detach phase (1.4). Might need more extensive tests though... Martijn On Thu, Jan 15, 2009 at 10:12 PM, Jonathan Locke wrote: > > > this is a really good point. i think i'm -1 on the idea too unless there's a > good proposal for how to av

Re: Why you should not override isVisible

2009-01-15 Thread Jonathan Locke
this is a really good point. i think i'm -1 on the idea too unless there's a good proposal for how to avoid that problem. igor.vaynberg wrote: > > if we do this then as a reusable component developer you have to > always be aware of this default and code with it in mind because it is > outside

Re: Why you should not override isVisible

2009-01-15 Thread Igor Vaynberg
if we do this then as a reusable component developer you have to always be aware of this default and code with it in mind because it is outside your control yet impacts the behaviors of components. -1 from me. -igor On Thu, Jan 15, 2009 at 12:25 PM, Jonathan Locke wrote: > > > yeah, for full fle

Re: Why you should not override isVisible

2009-01-15 Thread Martijn Dashorst
On Thu, Jan 15, 2009 at 9:38 PM, Erik van Oosten wrote: > I actually thought they were in English, but I now see that the first few > are in Dutch. Not sure why I did that. They are not that important, so just > read on... You can probably have a good laugh at google translate (or babelfish) and

Re: Why you should not override isVisible

2009-01-15 Thread Erik van Oosten
Indeed. If this would no longer be the case, overriding isVisible would be no problem (though caching would be nice). Regards, Erik. Martijn Dashorst wrote: What is strange is that isvisible is being checked during detach (I seriously doubt that). That shouldn't be happening: *all* compon

Re: Why you should not override isVisible

2009-01-15 Thread Martijn Dashorst
What is strange is that isvisible is being checked during detach (I seriously doubt that). That shouldn't be happening: *all* components should be detached regardless of their visibility. Martijn On Thu, Jan 15, 2009 at 9:29 PM, Jonathan Locke wrote: > > > i think the next step is to see what th

Re: Why you should not override isVisible

2009-01-15 Thread Erik van Oosten
Hi Pierre, I actually thought they were in English, but I now see that the first few are in Dutch. Not sure why I did that. They are not that important, so just read on... Regards, Erik. Pierre Goupil wrote: Good evening, I'm sorry to bug you, but I'be read the presentation you're talk

Re: Why you should not override isVisible

2009-01-15 Thread Jonathan Locke
i think the next step is to see what the core team thinks about this. filing a ticket couldn't hurt. Jonathan Locke wrote: > > > yeah, for full flexibility, you might just link a method in Component to a > default in IRequestCycleSettings: > > boolean getCacheComponentVisibility() { return >

Re: Why you should not override isVisible

2009-01-15 Thread Jonathan Locke
yeah, for full flexibility, you might just link a method in Component to a default in IRequestCycleSettings: boolean getCacheComponentVisibility() { return getApplication().getRequestCycleSettings().getCacheComponentVisibility(); } then you can set a default and override for any individual comp

Re: Why you should not override isVisible

2009-01-15 Thread Martin Makundi
> you could always work it the other way and have no caching by default but > allow people who know what they're doing to enable it for an application > with something like > IRequestCycleSettings#setCacheComponentVisibility(boolean). This sounds more robust, so long as it can be controlled on a p

Re: Why you should not override isVisible

2009-01-15 Thread Jonathan Locke
well yes, it might break existing behavior, that's why i suggested having a workaround for that. although i would think that well-designed isVisible methods shouldn't generally have this problem. you could always work it the other way and have no caching by default but allow people who know wha

Re: Why you should not override isVisible

2009-01-15 Thread Martin Makundi
Well... I would not build in the cache because different components could interact in a way that if someone prematurely calls the isVisible method, it freezes in the wrong state. It is known, that the isVisible is called multiple times for various reasons. ** Martin 2009/1/15 Jonathan Locke : > >

Re: Why you should not override isVisible

2009-01-15 Thread Jonathan Locke
not sure. it /ought/ to make sense to cache during the render cycle. but i have the eerie sense that this came up before, so maybe there's some reason why it can't be cached? (although even if there are some odd cases where it can't be, it seems like you could make a workaround for those cases ..

Re: Why you should not override isVisible

2009-01-15 Thread Scott Swank
Is there a reason why the default behavior is not to cache the result of isVisible()? Are there cases where the result of isVisible() is expected to change over the course of rendering? Would a JIRA w/ code be welcome, or is the current behavior required? Scott On Thu, Jan 15, 2009 at 10:23 AM,

Re: Why you should not override isVisible

2009-01-15 Thread Jonathan Locke
I would be careful not to throw the baby out with the bath water here. The design decision you're making is push versus pull and implementing isVisible is often going to be a better design decision because it may be more clear that visibility state stays consistent for a given problem. If the per

Re: Why you should not override isVisible

2009-01-15 Thread Scott Swank
We have done exact that -- a lot. Driving isVisible() from the state of the model is very clean, but the performance is potentially awful. I have wondered whether this should be the default implementation for Component.isVisible(). Is IVisitor.beforeRender() an appropriate place in the rendering

Re: Why you should not override isVisible

2009-01-15 Thread Erik van Oosten
Martin Makundi wrote: -1- isVisible is called a lot. It is easily called ten times within 1 request If you need to optimize, you can use lazy initialization of a boolean variable here and reset it in onBeforeRender? Yes, you could use lazy evaluation. (You should reset the boolean in a

Re: Why you should not override isVisible

2009-01-15 Thread Martin Makundi
> -1- isVisible is called a lot. It is easily called ten times within 1 > request If you need to optimize, you can use lazy initialization of a boolean variable here and reset it in onBeforeRender? > -2- isVisible can make your model be reloaded multiple times within 1 > request If you need to o