Probably our model should cache the result of cart.getTotal() since that's the expensive bit.
That said, I see value in introducing visibility caching into Wicket core. Others do not. I make my case, the core developers decide, we move on. :) Scott On Fri, Jan 16, 2009 at 9:05 AM, Sven Meier <[email protected]> wrote: > But the model's value is accessed by many other methods, not just > isVisible(). > If you want to save the reflection overhead why don't you put a caching > model between your component and the CompoundPropertyModel? > > Or access the model of the containing component if this is applicable: > > public void CartPanel(IModel cart) { > super(new CompoundPropertyModel(cart)); > > add(new Label("total") { > public boolean isVisible() { > ((Cart)CartPanel.this.getModelObject()).getTotal().isPositive(); > } > }); > } > > I still don't see the need to introduce visibility caching into Wicket core. > > Sven > > > Scott Swank schrieb: >> >> 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] >> >> >> > > > --------------------------------------------------------------------- > 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]
