That comes from a CompoundPropertyModel<Cart> and is bound to "total". So getModelObject() corresponds to cart.getTotal(). From there, isPositive() is quite cheap.
And we can, of course, keep implementing ad hoc caching of visibility. It's in no way complex, however it seems preferable to have this as the default behavior since only a very few components are likely to want to change their visibility over the course of rendering. Is this something that could be examined in 1.4 or 1.5 or is it simply inappropriate -- perhaps due to component details with which I'm unfamiliar? On Fri, Jan 16, 2009 at 12:53 AM, <[email protected]> wrote: > > > > 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: [email protected] > 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 > > their purchase (e.g. as opposed to using a gift card). These "total > > the order and determine visibility" methods were particular hot spots. > > > > @Override > > public boolean isVisible() { > > if (visible == null) > > visible = ((Money) getModelObject()).isPositive(); > > return visible; > > } > > > > While this is an idiosyncratic example, I can vouch for the fact that > > performance woes in isVisible() show up in profiling. > > > > On Thu, Jan 15, 2009 at 4:56 PM, Jonathan Locke > > <[email protected]> wrote: > >> > >> > >> 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 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 never run into it myself and if i did > >>> see it in a profiler, i'd probably just cache the value in a Boolean. it's > >>> literally just this little bit in your anonymous class: > >>> > >>> Boolean visible = null; > >>> public isVisible() { > >>> if (visible == null) { > >>> visible = // whatever boolean computation > >>> } > >>> return visible; > >>> } > >>> > >>> and then it disappears from the profiler and who cares about the rest. > >>> > >>> > >>> Scott Swank wrote: > >>>> > >>>> 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-cached > >>>> visibility (seemingly the 1% case) could override isVisibleInternal(), > >>>> but everyone else would get caching for free with their current code. > >>>> > >>>> On Thu, Jan 15, 2009 at 2:35 PM, Jonathan Locke > >>>> <[email protected]> wrote: > >>>>> > >>>>> > >>>>> 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 support this... > >>>>> one for whether isCachedVisible returned non-null and another > >>>>> for the value it returned. > >>>>> > >>>> > >>>> --------------------------------------------------------------------- > >>>> To unsubscribe, e-mail: [email protected] > >>>> For additional commands, e-mail: [email protected] > >>>> > >>>> > >>>> > >>> > >>> > >> > >> -- > >> View this message in context: >> http://www.nabble.com/Why-you-should-not-override-isVisible-tp21474995p21490479.html > >> Sent from the Wicket - User mailing list archive at Nabble.com. > >> > >> > >> --------------------------------------------------------------------- > >> To unsubscribe, e-mail: [email protected] > >> For additional commands, e-mail: [email protected] > >> > >> > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: [email protected] > > For additional commands, e-mail: [email protected] > > > > > > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
