Re: overriding isVisible bad?
Component can continue having the public isVisible/Enabled method always returning its correct state avaliable to use. Just the core request code will use an safe version that can be: isVisible/EnabledOnRequest. I really value an request cycle protected against volatile component states. I like the idea of using composition. We can refactor all the Component state code to an StateManager object. Its main goal can be: - maintain the components state: all its flags, the data object and respective API - define how close the visible/enabledOnRequest state will be to the user logic: we can define the check points, by default will be only the onConfigure. Perhaps user will can even override the SateManager, coding specific component logic (can solve some security restrictions). On Fri, Dec 3, 2010 at 11:12 PM, Eelco Hillenius eelco.hillen...@gmail.comwrote: That's actually also an example of why I prefer overriding isVisible: I can have that button and other widgets (maybe completely unrelated) widgets that depend on a particular state (like whether a record exists), and a function (override of isVisible) will always yield the correct result. In contrast, relying on the internal component state (set/isVisible) puts the responsibility of updating the relevant components on the handler (or worse, it may have to be triggered from the business layer). Eelco On Fri, Dec 3, 2010 at 10:46 AM, Igor Vaynberg igor.vaynb...@gmail.com wrote: ldm also doesnt always work nicely. suppose you have a delete button that is only visible if the record exists. the page renders, the user clicks the button. the onclick handler invoked, and isvisible is checked - at which point it is true. the record is deleted in the onclick handler, the page is rerendered - delete button is still visible because the ldm has cached the visibility value from before onclick handler is invoked. to make it work correctly the ldm would have to be detached manually after onclick handler. -igor On Thu, Dec 2, 2010 at 6:36 PM, Clint Checketts checke...@gmail.com wrote: Yesterday a friend following this thread pointed out that we should rethink our overriding of onVisible and use onConfigure. I've used LoadabledDetachableModels to cache the value used in my isVisible/isEnabled overriding so changing values mid request aren't a problem. That is its whole purpose. Also calling .detach() on that model isn't hacky, that is its design. While I appreciate having onConfigure as an option it seems like overriding isVisible is still the cleaner and clearer way. Folks just need to follow the rule that expensive calls should be contained in an LDM. Am I stuck in the past in holding this view? -Clint On Thu, Dec 2, 2010 at 4:03 AM, Martin Makundi martin.maku...@koodaripalvelut.com wrote: What about using onconfigure to replace loadabledetachablemodel ? We have had some trouble with loadabledetachablemodels when their state is frozen before a dependent model has been initialized (for example) and we need to call model.detach() from within our code, which seems bit hacky. Initializing also models at a specific known requestcycle moment might be beneficial. Ofcourse it is not so straightforward as with enable/visible state. ** Martin 2010/12/1 Igor Vaynberg igor.vaynb...@gmail.com: i would be happy if that was good enough. in the past it hasnt been, thats why we have the current solution. maybe we can try it again in 1.5 and see what happens. -igor On Tue, Nov 30, 2010 at 11:44 AM, Pedro Santos pedros...@gmail.com wrote: I have a different point of view, the HTTP imposes us some limitations, we will hardly have an good synchronization between the component state on browser and server using only HTTP conversation. So it is mandatory the service layer to respect the described security restriction. On Tue, Nov 30, 2010 at 5:32 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: an easy example is security. a user views a page that allows them to delete another user meanwhile their permissions are tweaked and they can no longer delete other users half an hour later the user clicks the delete button - this should fail, but wont if we are using last-rendered state. -igor On Tue, Nov 30, 2010 at 11:18 AM, Pedro Santos pedros...@gmail.com wrote: I need to look better on which core components are relying on an updated visibility/enabled state at the process event time, and why the last rendered state wouldn't be enough to them to work nicely. On Tue, Nov 30, 2010 at 3:19 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: currently we only invoke configure before the render. this would mean we would have to invoke it before processing a listener, clearing the cache, and then invoking it again before render. i wonder if that is enough
Re: overriding isVisible bad?
Igor, I agree that there are places that using onConfigure / setVisible may be better than overriding isVisible. However, I often have the following type of scenario, and wonder how you would suggest doing it without overriding isVisible: Situation: one component depends on the visibility of another (sibling) component for its visibility. if using onConfigure / setVisible, wouldn't there be an order-of-operations problem? Example: final Link next = new Link(next) { onConfigure() { setVisible(someCalculatedState); } } Label nextLabel = new Label(nextLabel) { onConfigure() { setVisible(next.isVisible()); } } I could do that before by overriding isVisible (rather than onConfigure) for both. Then, when nextLabel#isVisible called next#isVisible to calculate its state, it was guaranteed that next would return the proper info. But now, you could have the following situation: Render 1: both are visible Render 2: something has changed and now someCalculatedState will be false nextLabel is configured first and sets visible to true since next has not yet reconfigured based on the new state next is configured, and sets visibility to false based on the new state I suppose the best way to accomplish this with the onConfigure/setVisible method is to do onConfigure() { setVisible(calculateSomeState()); } the problem is that it makes it more difficult to create generic components this way (a label that takes another component as constructor arg and uses that component to determine its visibility). Thoughts? -- Jeremy Thomerson http://wickettraining.com *Need a CMS for Wicket? Use Brix! http://brixcms.org*
Re: overriding isVisible bad?
afair this is already explained in the javadoc of onconfigure label { onconfigure() { link.onconfigure(); setvisible(link.isvisible()); }} -igor On Mon, Dec 6, 2010 at 7:35 AM, Jeremy Thomerson jer...@wickettraining.com wrote: Igor, I agree that there are places that using onConfigure / setVisible may be better than overriding isVisible. However, I often have the following type of scenario, and wonder how you would suggest doing it without overriding isVisible: Situation: one component depends on the visibility of another (sibling) component for its visibility. if using onConfigure / setVisible, wouldn't there be an order-of-operations problem? Example: final Link next = new Link(next) { onConfigure() { setVisible(someCalculatedState); } } Label nextLabel = new Label(nextLabel) { onConfigure() { setVisible(next.isVisible()); } } I could do that before by overriding isVisible (rather than onConfigure) for both. Then, when nextLabel#isVisible called next#isVisible to calculate its state, it was guaranteed that next would return the proper info. But now, you could have the following situation: Render 1: both are visible Render 2: something has changed and now someCalculatedState will be false nextLabel is configured first and sets visible to true since next has not yet reconfigured based on the new state next is configured, and sets visibility to false based on the new state I suppose the best way to accomplish this with the onConfigure/setVisible method is to do onConfigure() { setVisible(calculateSomeState()); } the problem is that it makes it more difficult to create generic components this way (a label that takes another component as constructor arg and uses that component to determine its visibility). Thoughts? -- Jeremy Thomerson http://wickettraining.com *Need a CMS for Wicket? Use Brix! http://brixcms.org*
Re: overriding isVisible bad?
ldm also doesnt always work nicely. suppose you have a delete button that is only visible if the record exists. the page renders, the user clicks the button. the onclick handler invoked, and isvisible is checked - at which point it is true. the record is deleted in the onclick handler, the page is rerendered - delete button is still visible because the ldm has cached the visibility value from before onclick handler is invoked. to make it work correctly the ldm would have to be detached manually after onclick handler. -igor On Thu, Dec 2, 2010 at 6:36 PM, Clint Checketts checke...@gmail.com wrote: Yesterday a friend following this thread pointed out that we should rethink our overriding of onVisible and use onConfigure. I've used LoadabledDetachableModels to cache the value used in my isVisible/isEnabled overriding so changing values mid request aren't a problem. That is its whole purpose. Also calling .detach() on that model isn't hacky, that is its design. While I appreciate having onConfigure as an option it seems like overriding isVisible is still the cleaner and clearer way. Folks just need to follow the rule that expensive calls should be contained in an LDM. Am I stuck in the past in holding this view? -Clint On Thu, Dec 2, 2010 at 4:03 AM, Martin Makundi martin.maku...@koodaripalvelut.com wrote: What about using onconfigure to replace loadabledetachablemodel ? We have had some trouble with loadabledetachablemodels when their state is frozen before a dependent model has been initialized (for example) and we need to call model.detach() from within our code, which seems bit hacky. Initializing also models at a specific known requestcycle moment might be beneficial. Ofcourse it is not so straightforward as with enable/visible state. ** Martin 2010/12/1 Igor Vaynberg igor.vaynb...@gmail.com: i would be happy if that was good enough. in the past it hasnt been, thats why we have the current solution. maybe we can try it again in 1.5 and see what happens. -igor On Tue, Nov 30, 2010 at 11:44 AM, Pedro Santos pedros...@gmail.com wrote: I have a different point of view, the HTTP imposes us some limitations, we will hardly have an good synchronization between the component state on browser and server using only HTTP conversation. So it is mandatory the service layer to respect the described security restriction. On Tue, Nov 30, 2010 at 5:32 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: an easy example is security. a user views a page that allows them to delete another user meanwhile their permissions are tweaked and they can no longer delete other users half an hour later the user clicks the delete button - this should fail, but wont if we are using last-rendered state. -igor On Tue, Nov 30, 2010 at 11:18 AM, Pedro Santos pedros...@gmail.com wrote: I need to look better on which core components are relying on an updated visibility/enabled state at the process event time, and why the last rendered state wouldn't be enough to them to work nicely. On Tue, Nov 30, 2010 at 3:19 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: currently we only invoke configure before the render. this would mean we would have to invoke it before processing a listener, clearing the cache, and then invoking it again before render. i wonder if that is enough places to invoke it -igor On Tue, Nov 30, 2010 at 9:15 AM, Pedro Santos pedros...@gmail.com wrote: If user click an link, it will change the value of some property at the process_event request cycle step. Then the processor will go to the respond step, will invoke every component before render method which will end up invoking the Component#configure and updating the visibility/enabled state (even if it changes, we are able to work with the updated state). So when the this component has the opportunity to render it self, it will be aware its update state. On Tue, Nov 30, 2010 at 2:39 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: there are other places that should be checked though. for example before we invoke a listener on the component we should check again to make sure that visibility hasnt changed. eg if visibility depends on some property of the user clicking the link that changed between render and clicking the link. -igor On Tue, Nov 30, 2010 at 8:30 AM, Pedro Santos pedros...@gmail.com wrote: An implementation idea: Component { public final void configure() { if (!getFlag(FLAG_CONFIGURED)) { setVisible_NoClientCode(isVisible()); //we only check the user isVisible in here onConfigure(); setFlag(FLAG_CONFIGURED, true); } } } On Tue, Nov 30, 2010 at 2:16 PM, Igor Vaynberg
Re: overriding isVisible bad?
That's actually also an example of why I prefer overriding isVisible: I can have that button and other widgets (maybe completely unrelated) widgets that depend on a particular state (like whether a record exists), and a function (override of isVisible) will always yield the correct result. In contrast, relying on the internal component state (set/isVisible) puts the responsibility of updating the relevant components on the handler (or worse, it may have to be triggered from the business layer). Eelco On Fri, Dec 3, 2010 at 10:46 AM, Igor Vaynberg igor.vaynb...@gmail.com wrote: ldm also doesnt always work nicely. suppose you have a delete button that is only visible if the record exists. the page renders, the user clicks the button. the onclick handler invoked, and isvisible is checked - at which point it is true. the record is deleted in the onclick handler, the page is rerendered - delete button is still visible because the ldm has cached the visibility value from before onclick handler is invoked. to make it work correctly the ldm would have to be detached manually after onclick handler. -igor On Thu, Dec 2, 2010 at 6:36 PM, Clint Checketts checke...@gmail.com wrote: Yesterday a friend following this thread pointed out that we should rethink our overriding of onVisible and use onConfigure. I've used LoadabledDetachableModels to cache the value used in my isVisible/isEnabled overriding so changing values mid request aren't a problem. That is its whole purpose. Also calling .detach() on that model isn't hacky, that is its design. While I appreciate having onConfigure as an option it seems like overriding isVisible is still the cleaner and clearer way. Folks just need to follow the rule that expensive calls should be contained in an LDM. Am I stuck in the past in holding this view? -Clint On Thu, Dec 2, 2010 at 4:03 AM, Martin Makundi martin.maku...@koodaripalvelut.com wrote: What about using onconfigure to replace loadabledetachablemodel ? We have had some trouble with loadabledetachablemodels when their state is frozen before a dependent model has been initialized (for example) and we need to call model.detach() from within our code, which seems bit hacky. Initializing also models at a specific known requestcycle moment might be beneficial. Ofcourse it is not so straightforward as with enable/visible state. ** Martin 2010/12/1 Igor Vaynberg igor.vaynb...@gmail.com: i would be happy if that was good enough. in the past it hasnt been, thats why we have the current solution. maybe we can try it again in 1.5 and see what happens. -igor On Tue, Nov 30, 2010 at 11:44 AM, Pedro Santos pedros...@gmail.com wrote: I have a different point of view, the HTTP imposes us some limitations, we will hardly have an good synchronization between the component state on browser and server using only HTTP conversation. So it is mandatory the service layer to respect the described security restriction. On Tue, Nov 30, 2010 at 5:32 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: an easy example is security. a user views a page that allows them to delete another user meanwhile their permissions are tweaked and they can no longer delete other users half an hour later the user clicks the delete button - this should fail, but wont if we are using last-rendered state. -igor On Tue, Nov 30, 2010 at 11:18 AM, Pedro Santos pedros...@gmail.com wrote: I need to look better on which core components are relying on an updated visibility/enabled state at the process event time, and why the last rendered state wouldn't be enough to them to work nicely. On Tue, Nov 30, 2010 at 3:19 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: currently we only invoke configure before the render. this would mean we would have to invoke it before processing a listener, clearing the cache, and then invoking it again before render. i wonder if that is enough places to invoke it -igor On Tue, Nov 30, 2010 at 9:15 AM, Pedro Santos pedros...@gmail.com wrote: If user click an link, it will change the value of some property at the process_event request cycle step. Then the processor will go to the respond step, will invoke every component before render method which will end up invoking the Component#configure and updating the visibility/enabled state (even if it changes, we are able to work with the updated state). So when the this component has the opportunity to render it self, it will be aware its update state. On Tue, Nov 30, 2010 at 2:39 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: there are other places that should be checked though. for example before we invoke a listener on the component we should check again to make sure that visibility hasnt changed. eg if visibility depends on some property of the user clicking
Re: overriding isVisible bad?
huh? that doesnt make any sense. the callbacks like onconfigure simply give you checkpoints for calculating and caching visibility rather then calculating every time. -igor On Fri, Dec 3, 2010 at 5:12 PM, Eelco Hillenius eelco.hillen...@gmail.com wrote: That's actually also an example of why I prefer overriding isVisible: I can have that button and other widgets (maybe completely unrelated) widgets that depend on a particular state (like whether a record exists), and a function (override of isVisible) will always yield the correct result. In contrast, relying on the internal component state (set/isVisible) puts the responsibility of updating the relevant components on the handler (or worse, it may have to be triggered from the business layer). Eelco On Fri, Dec 3, 2010 at 10:46 AM, Igor Vaynberg igor.vaynb...@gmail.com wrote: ldm also doesnt always work nicely. suppose you have a delete button that is only visible if the record exists. the page renders, the user clicks the button. the onclick handler invoked, and isvisible is checked - at which point it is true. the record is deleted in the onclick handler, the page is rerendered - delete button is still visible because the ldm has cached the visibility value from before onclick handler is invoked. to make it work correctly the ldm would have to be detached manually after onclick handler. -igor On Thu, Dec 2, 2010 at 6:36 PM, Clint Checketts checke...@gmail.com wrote: Yesterday a friend following this thread pointed out that we should rethink our overriding of onVisible and use onConfigure. I've used LoadabledDetachableModels to cache the value used in my isVisible/isEnabled overriding so changing values mid request aren't a problem. That is its whole purpose. Also calling .detach() on that model isn't hacky, that is its design. While I appreciate having onConfigure as an option it seems like overriding isVisible is still the cleaner and clearer way. Folks just need to follow the rule that expensive calls should be contained in an LDM. Am I stuck in the past in holding this view? -Clint On Thu, Dec 2, 2010 at 4:03 AM, Martin Makundi martin.maku...@koodaripalvelut.com wrote: What about using onconfigure to replace loadabledetachablemodel ? We have had some trouble with loadabledetachablemodels when their state is frozen before a dependent model has been initialized (for example) and we need to call model.detach() from within our code, which seems bit hacky. Initializing also models at a specific known requestcycle moment might be beneficial. Ofcourse it is not so straightforward as with enable/visible state. ** Martin 2010/12/1 Igor Vaynberg igor.vaynb...@gmail.com: i would be happy if that was good enough. in the past it hasnt been, thats why we have the current solution. maybe we can try it again in 1.5 and see what happens. -igor On Tue, Nov 30, 2010 at 11:44 AM, Pedro Santos pedros...@gmail.com wrote: I have a different point of view, the HTTP imposes us some limitations, we will hardly have an good synchronization between the component state on browser and server using only HTTP conversation. So it is mandatory the service layer to respect the described security restriction. On Tue, Nov 30, 2010 at 5:32 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: an easy example is security. a user views a page that allows them to delete another user meanwhile their permissions are tweaked and they can no longer delete other users half an hour later the user clicks the delete button - this should fail, but wont if we are using last-rendered state. -igor On Tue, Nov 30, 2010 at 11:18 AM, Pedro Santos pedros...@gmail.com wrote: I need to look better on which core components are relying on an updated visibility/enabled state at the process event time, and why the last rendered state wouldn't be enough to them to work nicely. On Tue, Nov 30, 2010 at 3:19 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: currently we only invoke configure before the render. this would mean we would have to invoke it before processing a listener, clearing the cache, and then invoking it again before render. i wonder if that is enough places to invoke it -igor On Tue, Nov 30, 2010 at 9:15 AM, Pedro Santos pedros...@gmail.com wrote: If user click an link, it will change the value of some property at the process_event request cycle step. Then the processor will go to the respond step, will invoke every component before render method which will end up invoking the Component#configure and updating the visibility/enabled state (even if it changes, we are able to work with the updated state). So when the this component has the opportunity to render it self, it will be aware its update state. On Tue, Nov 30, 2010 at 2:39 PM, Igor Vaynberg igor.vaynb...@gmail.com
Re: overriding isVisible bad?
huh? that doesnt make any sense. the callbacks like onconfigure simply give you checkpoints for calculating and caching visibility rather then calculating every time. I wasn't arguing against onConfigure (which is a fine trade-off) but saw an example of where relying on just setVisible would be problematic. Now the problem is that we have three ways of achieving the same thing. And yet another overridable method. It's probably too late in the game, but I would be more in favor of a delegation mechanism (e.g. interfaces for the functions of isVisible, isEnabled and whatever else makes sense) and a way to set those functions. So you'd do things more through composition. Might even consider making working with such functions a generic capability that also supports detaching (which then can also be used to let the component manage detachment of multiple models if there's more than one), etc. Would be a significant break though, so I'm not expecting a lot of cheering :-) Eelco
Re: overriding isVisible bad?
What about using onconfigure to replace loadabledetachablemodel ? We have had some trouble with loadabledetachablemodels when their state is frozen before a dependent model has been initialized (for example) and we need to call model.detach() from within our code, which seems bit hacky. Initializing also models at a specific known requestcycle moment might be beneficial. Ofcourse it is not so straightforward as with enable/visible state. ** Martin 2010/12/1 Igor Vaynberg igor.vaynb...@gmail.com: i would be happy if that was good enough. in the past it hasnt been, thats why we have the current solution. maybe we can try it again in 1.5 and see what happens. -igor On Tue, Nov 30, 2010 at 11:44 AM, Pedro Santos pedros...@gmail.com wrote: I have a different point of view, the HTTP imposes us some limitations, we will hardly have an good synchronization between the component state on browser and server using only HTTP conversation. So it is mandatory the service layer to respect the described security restriction. On Tue, Nov 30, 2010 at 5:32 PM, Igor Vaynberg igor.vaynb...@gmail.comwrote: an easy example is security. a user views a page that allows them to delete another user meanwhile their permissions are tweaked and they can no longer delete other users half an hour later the user clicks the delete button - this should fail, but wont if we are using last-rendered state. -igor On Tue, Nov 30, 2010 at 11:18 AM, Pedro Santos pedros...@gmail.com wrote: I need to look better on which core components are relying on an updated visibility/enabled state at the process event time, and why the last rendered state wouldn't be enough to them to work nicely. On Tue, Nov 30, 2010 at 3:19 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: currently we only invoke configure before the render. this would mean we would have to invoke it before processing a listener, clearing the cache, and then invoking it again before render. i wonder if that is enough places to invoke it -igor On Tue, Nov 30, 2010 at 9:15 AM, Pedro Santos pedros...@gmail.com wrote: If user click an link, it will change the value of some property at the process_event request cycle step. Then the processor will go to the respond step, will invoke every component before render method which will end up invoking the Component#configure and updating the visibility/enabled state (even if it changes, we are able to work with the updated state). So when the this component has the opportunity to render it self, it will be aware its update state. On Tue, Nov 30, 2010 at 2:39 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: there are other places that should be checked though. for example before we invoke a listener on the component we should check again to make sure that visibility hasnt changed. eg if visibility depends on some property of the user clicking the link that changed between render and clicking the link. -igor On Tue, Nov 30, 2010 at 8:30 AM, Pedro Santos pedros...@gmail.com wrote: An implementation idea: Component { public final void configure() { if (!getFlag(FLAG_CONFIGURED)) { setVisible_NoClientCode(isVisible()); //we only check the user isVisible in here onConfigure(); setFlag(FLAG_CONFIGURED, true); } } } On Tue, Nov 30, 2010 at 2:16 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: so how is it different if they can still override something that needs to be checked all the time? -igor On Tue, Nov 30, 2010 at 8:02 AM, Pedro Santos pedros...@gmail.com wrote: I understand the concern about possible isVisible implementations like isVisible(return currentlyTime 10:00:00;) //imagine this component being rendered at 09:59:59 isVisible(return dao.list().size() 0);// performance issues But maybe we can have the best from both approaches. This is an copy/paste from java.awt.Component: public boolean isVisible() { return isVisible_NoClientCode(); } final boolean isVisible_NoClientCode() { return visible; } There are some points in the awt framework were the isVisible method is not used in benefit of isVisible_NoClientCode I'm in favor of create an final isVisible/Enabled version and change the Wicket core to use it. Also maintain the hotspot to users provide their isVisible/Enable implementations that will serve to feed the core component state. On Mon, Nov 29, 2010 at 4:56 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: ive run into plenty of weird problems with overrides, but maybe because this was in a high concurrency app where data changed
Re: overriding isVisible bad?
Yesterday a friend following this thread pointed out that we should rethink our overriding of onVisible and use onConfigure. I've used LoadabledDetachableModels to cache the value used in my isVisible/isEnabled overriding so changing values mid request aren't a problem. That is its whole purpose. Also calling .detach() on that model isn't hacky, that is its design. While I appreciate having onConfigure as an option it seems like overriding isVisible is still the cleaner and clearer way. Folks just need to follow the rule that expensive calls should be contained in an LDM. Am I stuck in the past in holding this view? -Clint On Thu, Dec 2, 2010 at 4:03 AM, Martin Makundi martin.maku...@koodaripalvelut.com wrote: What about using onconfigure to replace loadabledetachablemodel ? We have had some trouble with loadabledetachablemodels when their state is frozen before a dependent model has been initialized (for example) and we need to call model.detach() from within our code, which seems bit hacky. Initializing also models at a specific known requestcycle moment might be beneficial. Ofcourse it is not so straightforward as with enable/visible state. ** Martin 2010/12/1 Igor Vaynberg igor.vaynb...@gmail.com: i would be happy if that was good enough. in the past it hasnt been, thats why we have the current solution. maybe we can try it again in 1.5 and see what happens. -igor On Tue, Nov 30, 2010 at 11:44 AM, Pedro Santos pedros...@gmail.com wrote: I have a different point of view, the HTTP imposes us some limitations, we will hardly have an good synchronization between the component state on browser and server using only HTTP conversation. So it is mandatory the service layer to respect the described security restriction. On Tue, Nov 30, 2010 at 5:32 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: an easy example is security. a user views a page that allows them to delete another user meanwhile their permissions are tweaked and they can no longer delete other users half an hour later the user clicks the delete button - this should fail, but wont if we are using last-rendered state. -igor On Tue, Nov 30, 2010 at 11:18 AM, Pedro Santos pedros...@gmail.com wrote: I need to look better on which core components are relying on an updated visibility/enabled state at the process event time, and why the last rendered state wouldn't be enough to them to work nicely. On Tue, Nov 30, 2010 at 3:19 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: currently we only invoke configure before the render. this would mean we would have to invoke it before processing a listener, clearing the cache, and then invoking it again before render. i wonder if that is enough places to invoke it -igor On Tue, Nov 30, 2010 at 9:15 AM, Pedro Santos pedros...@gmail.com wrote: If user click an link, it will change the value of some property at the process_event request cycle step. Then the processor will go to the respond step, will invoke every component before render method which will end up invoking the Component#configure and updating the visibility/enabled state (even if it changes, we are able to work with the updated state). So when the this component has the opportunity to render it self, it will be aware its update state. On Tue, Nov 30, 2010 at 2:39 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: there are other places that should be checked though. for example before we invoke a listener on the component we should check again to make sure that visibility hasnt changed. eg if visibility depends on some property of the user clicking the link that changed between render and clicking the link. -igor On Tue, Nov 30, 2010 at 8:30 AM, Pedro Santos pedros...@gmail.com wrote: An implementation idea: Component { public final void configure() { if (!getFlag(FLAG_CONFIGURED)) { setVisible_NoClientCode(isVisible()); //we only check the user isVisible in here onConfigure(); setFlag(FLAG_CONFIGURED, true); } } } On Tue, Nov 30, 2010 at 2:16 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: so how is it different if they can still override something that needs to be checked all the time? -igor On Tue, Nov 30, 2010 at 8:02 AM, Pedro Santos pedros...@gmail.com wrote: I understand the concern about possible isVisible implementations like isVisible(return currentlyTime 10:00:00;) //imagine this component being rendered at 09:59:59 isVisible(return dao.list().size() 0);// performance issues But maybe we can have the best from both approaches. This is an
Re: overriding isVisible bad?
On Thu, Dec 2, 2010 at 9:36 PM, Clint Checketts checke...@gmail.com wrote: While I appreciate having onConfigure as an option it seems like overriding isVisible is still the cleaner and clearer way. Folks just need to follow the rule that expensive calls should be contained in an LDM. The expense isn't the problem. It's the dynamic nature of the value returned that causes issues, IIUC.
Re: overriding isVisible bad?
On of the main problems I see with onConfigure() is that is a general 'configuration' method. In our code, we ofter create an anonymous inner-class, overriding the isVisible() method. These methods often look like 'return super.isVisible() someOtherCondition;'. This is a lot more difficult to implement in a onConfigure() method, especially when that method also configures a lot of other things. The only way to make that work, is by starting with a call to super.onConfigure(), and including super.isVisible() in the call to setVisible(). In this situation, I think overriding isVisible() is much cleaner. Emond On Monday 29 November 2010 19:56:39 Igor Vaynberg wrote: ive run into plenty of weird problems with overrides, but maybe because this was in a high concurrency app where data changed frequently. the problems arise from the fact that the value returned from isvisible() can change while we are doing traversals, etc. eg we run a traversal for all visible components and put them in a list. later we iterate over the list and try to render these components. the render function also checks their visibility and if they are no longer visible it throws an exception. if isvisible() override depends on some external factor like the database there is a small window where the value can change and now you can have a weird exception: such as tried to invoke a listener on a component that is not visible or not enabled. these are very intermittent and damn near impossible to reproduce. another problem is performance. isvisible() is called multiple times during the request and if it depends on the database it can be a performance problem. in fact a couple of users have complained about this on the list in the past. at least now we have an easy solution for them - use onconfigure(). so as of right now the developers have two choices: override isvisible() and potentially suffer the consequences. or, override onconfigure() and set visibility there in a more deterministic fashion. -igor On Mon, Nov 29, 2010 at 10:21 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: To expand, unless I'm missing something (new?), things are really only problematic when both the mutable value and the override are mixed. In a way, I think that using the override is 'more pure', as it's a simple function that is executed when needed, whereas mutable state can be harder to deal with when trying to figure out how it got to be in that state. So, sorry Igor, but we disagree on this one. Eelco On Mon, Nov 29, 2010 at 10:13 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: Niether is evil. It has potential pitfalls, which you should just be aware of. We use such overrides all over the place and never have problems with them either. :-) Avoiding it is safer, but also more verbose (in 1.3.x at least). Eelco On Mon, Nov 29, 2010 at 9:49 AM, Igor Vaynberg igor.vaynb...@gmail.com wrote: On Mon, Nov 29, 2010 at 9:35 AM, Sven Meier s...@meiers.net wrote: Hi Douglas, WICKET-3171 describes a problematic case, where visibility of a component changes while its form is being processed. In our projects we're overriding isVisible() where appropriate and never encountered a similar problem. I'd say WICKET-3171 is the rare 5% usecase. What's next, is overriding isEnabled() going to be declared evil too? ;) yes -igor Sven On Mon, 2010-11-29 at 11:22 -0600, Douglas Ferguson wrote: Can you explain why? We have done this all over the place. D/ On Nov 29, 2010, at 10:00 AM, Martin Grigorov wrote: The recommended way since a few 1.4 releases is to override onConfigure() and call setVisible(true|false) depending on your conditions. On Mon, Nov 29, 2010 at 4:49 PM, Douglas Ferguson doug...@douglasferguson.us wrote: Igor posted a comment to this bug saying that overriding isVisible() is evil https://issues.apache.org/jira/browse/WICKET-3171 I was surprised by this and am curious to hear more. D/
Re: overriding isVisible bad?
so how is it different if they can still override something that needs to be checked all the time? -igor On Tue, Nov 30, 2010 at 8:02 AM, Pedro Santos pedros...@gmail.com wrote: I understand the concern about possible isVisible implementations like isVisible(return currentlyTime 10:00:00;) //imagine this component being rendered at 09:59:59 isVisible(return dao.list().size() 0);// performance issues But maybe we can have the best from both approaches. This is an copy/paste from java.awt.Component: public boolean isVisible() { return isVisible_NoClientCode(); } final boolean isVisible_NoClientCode() { return visible; } There are some points in the awt framework were the isVisible method is not used in benefit of isVisible_NoClientCode I'm in favor of create an final isVisible/Enabled version and change the Wicket core to use it. Also maintain the hotspot to users provide their isVisible/Enable implementations that will serve to feed the core component state. On Mon, Nov 29, 2010 at 4:56 PM, Igor Vaynberg igor.vaynb...@gmail.comwrote: ive run into plenty of weird problems with overrides, but maybe because this was in a high concurrency app where data changed frequently. the problems arise from the fact that the value returned from isvisible() can change while we are doing traversals, etc. eg we run a traversal for all visible components and put them in a list. later we iterate over the list and try to render these components. the render function also checks their visibility and if they are no longer visible it throws an exception. if isvisible() override depends on some external factor like the database there is a small window where the value can change and now you can have a weird exception: such as tried to invoke a listener on a component that is not visible or not enabled. these are very intermittent and damn near impossible to reproduce. another problem is performance. isvisible() is called multiple times during the request and if it depends on the database it can be a performance problem. in fact a couple of users have complained about this on the list in the past. at least now we have an easy solution for them - use onconfigure(). so as of right now the developers have two choices: override isvisible() and potentially suffer the consequences. or, override onconfigure() and set visibility there in a more deterministic fashion. -igor On Mon, Nov 29, 2010 at 10:21 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: To expand, unless I'm missing something (new?), things are really only problematic when both the mutable value and the override are mixed. In a way, I think that using the override is 'more pure', as it's a simple function that is executed when needed, whereas mutable state can be harder to deal with when trying to figure out how it got to be in that state. So, sorry Igor, but we disagree on this one. Eelco On Mon, Nov 29, 2010 at 10:13 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: Niether is evil. It has potential pitfalls, which you should just be aware of. We use such overrides all over the place and never have problems with them either. :-) Avoiding it is safer, but also more verbose (in 1.3.x at least). Eelco On Mon, Nov 29, 2010 at 9:49 AM, Igor Vaynberg igor.vaynb...@gmail.com wrote: On Mon, Nov 29, 2010 at 9:35 AM, Sven Meier s...@meiers.net wrote: Hi Douglas, WICKET-3171 describes a problematic case, where visibility of a component changes while its form is being processed. In our projects we're overriding isVisible() where appropriate and never encountered a similar problem. I'd say WICKET-3171 is the rare 5% usecase. What's next, is overriding isEnabled() going to be declared evil too? ;) yes -igor Sven On Mon, 2010-11-29 at 11:22 -0600, Douglas Ferguson wrote: Can you explain why? We have done this all over the place. D/ On Nov 29, 2010, at 10:00 AM, Martin Grigorov wrote: The recommended way since a few 1.4 releases is to override onConfigure() and call setVisible(true|false) depending on your conditions. On Mon, Nov 29, 2010 at 4:49 PM, Douglas Ferguson doug...@douglasferguson.us wrote: Igor posted a comment to this bug saying that overriding isVisible() is evil https://issues.apache.org/jira/browse/WICKET-3171 I was surprised by this and am curious to hear more. D/ -- Pedro Henrique Oliveira dos Santos
Re: overriding isVisible bad?
An implementation idea: Component { public final void configure() { if (!getFlag(FLAG_CONFIGURED)) { setVisible_NoClientCode(isVisible()); //we only check the user isVisible in here onConfigure(); setFlag(FLAG_CONFIGURED, true); } } } On Tue, Nov 30, 2010 at 2:16 PM, Igor Vaynberg igor.vaynb...@gmail.comwrote: so how is it different if they can still override something that needs to be checked all the time? -igor On Tue, Nov 30, 2010 at 8:02 AM, Pedro Santos pedros...@gmail.com wrote: I understand the concern about possible isVisible implementations like isVisible(return currentlyTime 10:00:00;) //imagine this component being rendered at 09:59:59 isVisible(return dao.list().size() 0);// performance issues But maybe we can have the best from both approaches. This is an copy/paste from java.awt.Component: public boolean isVisible() { return isVisible_NoClientCode(); } final boolean isVisible_NoClientCode() { return visible; } There are some points in the awt framework were the isVisible method is not used in benefit of isVisible_NoClientCode I'm in favor of create an final isVisible/Enabled version and change the Wicket core to use it. Also maintain the hotspot to users provide their isVisible/Enable implementations that will serve to feed the core component state. On Mon, Nov 29, 2010 at 4:56 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: ive run into plenty of weird problems with overrides, but maybe because this was in a high concurrency app where data changed frequently. the problems arise from the fact that the value returned from isvisible() can change while we are doing traversals, etc. eg we run a traversal for all visible components and put them in a list. later we iterate over the list and try to render these components. the render function also checks their visibility and if they are no longer visible it throws an exception. if isvisible() override depends on some external factor like the database there is a small window where the value can change and now you can have a weird exception: such as tried to invoke a listener on a component that is not visible or not enabled. these are very intermittent and damn near impossible to reproduce. another problem is performance. isvisible() is called multiple times during the request and if it depends on the database it can be a performance problem. in fact a couple of users have complained about this on the list in the past. at least now we have an easy solution for them - use onconfigure(). so as of right now the developers have two choices: override isvisible() and potentially suffer the consequences. or, override onconfigure() and set visibility there in a more deterministic fashion. -igor On Mon, Nov 29, 2010 at 10:21 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: To expand, unless I'm missing something (new?), things are really only problematic when both the mutable value and the override are mixed. In a way, I think that using the override is 'more pure', as it's a simple function that is executed when needed, whereas mutable state can be harder to deal with when trying to figure out how it got to be in that state. So, sorry Igor, but we disagree on this one. Eelco On Mon, Nov 29, 2010 at 10:13 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: Niether is evil. It has potential pitfalls, which you should just be aware of. We use such overrides all over the place and never have problems with them either. :-) Avoiding it is safer, but also more verbose (in 1.3.x at least). Eelco On Mon, Nov 29, 2010 at 9:49 AM, Igor Vaynberg igor.vaynb...@gmail.com wrote: On Mon, Nov 29, 2010 at 9:35 AM, Sven Meier s...@meiers.net wrote: Hi Douglas, WICKET-3171 describes a problematic case, where visibility of a component changes while its form is being processed. In our projects we're overriding isVisible() where appropriate and never encountered a similar problem. I'd say WICKET-3171 is the rare 5% usecase. What's next, is overriding isEnabled() going to be declared evil too? ;) yes -igor Sven On Mon, 2010-11-29 at 11:22 -0600, Douglas Ferguson wrote: Can you explain why? We have done this all over the place. D/ On Nov 29, 2010, at 10:00 AM, Martin Grigorov wrote: The recommended way since a few 1.4 releases is to override onConfigure() and call setVisible(true|false) depending on your conditions. On Mon, Nov 29, 2010 at 4:49 PM, Douglas Ferguson doug...@douglasferguson.us wrote: Igor posted a comment to this bug saying that overriding isVisible() is evil https://issues.apache.org/jira/browse/WICKET-3171 I was surprised by this
Re: overriding isVisible bad?
Jus to get it right ... is the following statement correct? isVisible() will work as expected when the criteria for visibility does not change during render Am 30.11.2010 um 17:02 schrieb Pedro Santos: I understand the concern about possible isVisible implementations like isVisible(return currentlyTime 10:00:00;) //imagine this component being rendered at 09:59:59 isVisible(return dao.list().size() 0);// performance issues But maybe we can have the best from both approaches. This is an copy/paste from java.awt.Component: public boolean isVisible() { return isVisible_NoClientCode(); } final boolean isVisible_NoClientCode() { return visible; } There are some points in the awt framework were the isVisible method is not used in benefit of isVisible_NoClientCode I'm in favor of create an final isVisible/Enabled version and change the Wicket core to use it. Also maintain the hotspot to users provide their isVisible/Enable implementations that will serve to feed the core component state. On Mon, Nov 29, 2010 at 4:56 PM, Igor Vaynberg igor.vaynb...@gmail.comwrote: ive run into plenty of weird problems with overrides, but maybe because this was in a high concurrency app where data changed frequently. the problems arise from the fact that the value returned from isvisible() can change while we are doing traversals, etc. eg we run a traversal for all visible components and put them in a list. later we iterate over the list and try to render these components. the render function also checks their visibility and if they are no longer visible it throws an exception. if isvisible() override depends on some external factor like the database there is a small window where the value can change and now you can have a weird exception: such as tried to invoke a listener on a component that is not visible or not enabled. these are very intermittent and damn near impossible to reproduce. another problem is performance. isvisible() is called multiple times during the request and if it depends on the database it can be a performance problem. in fact a couple of users have complained about this on the list in the past. at least now we have an easy solution for them - use onconfigure(). so as of right now the developers have two choices: override isvisible() and potentially suffer the consequences. or, override onconfigure() and set visibility there in a more deterministic fashion. -igor On Mon, Nov 29, 2010 at 10:21 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: To expand, unless I'm missing something (new?), things are really only problematic when both the mutable value and the override are mixed. In a way, I think that using the override is 'more pure', as it's a simple function that is executed when needed, whereas mutable state can be harder to deal with when trying to figure out how it got to be in that state. So, sorry Igor, but we disagree on this one. Eelco On Mon, Nov 29, 2010 at 10:13 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: Niether is evil. It has potential pitfalls, which you should just be aware of. We use such overrides all over the place and never have problems with them either. :-) Avoiding it is safer, but also more verbose (in 1.3.x at least). Eelco On Mon, Nov 29, 2010 at 9:49 AM, Igor Vaynberg igor.vaynb...@gmail.com wrote: On Mon, Nov 29, 2010 at 9:35 AM, Sven Meier s...@meiers.net wrote: Hi Douglas, WICKET-3171 describes a problematic case, where visibility of a component changes while its form is being processed. In our projects we're overriding isVisible() where appropriate and never encountered a similar problem. I'd say WICKET-3171 is the rare 5% usecase. What's next, is overriding isEnabled() going to be declared evil too? ;) yes -igor Sven On Mon, 2010-11-29 at 11:22 -0600, Douglas Ferguson wrote: Can you explain why? We have done this all over the place. D/ On Nov 29, 2010, at 10:00 AM, Martin Grigorov wrote: The recommended way since a few 1.4 releases is to override onConfigure() and call setVisible(true|false) depending on your conditions. On Mon, Nov 29, 2010 at 4:49 PM, Douglas Ferguson doug...@douglasferguson.us wrote: Igor posted a comment to this bug saying that overriding isVisible() is evil https://issues.apache.org/jira/browse/WICKET-3171 I was surprised by this and am curious to hear more. D/ -- Pedro Henrique Oliveira dos Santos
Re: overriding isVisible bad?
there are other places that should be checked though. for example before we invoke a listener on the component we should check again to make sure that visibility hasnt changed. eg if visibility depends on some property of the user clicking the link that changed between render and clicking the link. -igor On Tue, Nov 30, 2010 at 8:30 AM, Pedro Santos pedros...@gmail.com wrote: An implementation idea: Component { public final void configure() { if (!getFlag(FLAG_CONFIGURED)) { setVisible_NoClientCode(isVisible()); //we only check the user isVisible in here onConfigure(); setFlag(FLAG_CONFIGURED, true); } } } On Tue, Nov 30, 2010 at 2:16 PM, Igor Vaynberg igor.vaynb...@gmail.comwrote: so how is it different if they can still override something that needs to be checked all the time? -igor On Tue, Nov 30, 2010 at 8:02 AM, Pedro Santos pedros...@gmail.com wrote: I understand the concern about possible isVisible implementations like isVisible(return currentlyTime 10:00:00;) //imagine this component being rendered at 09:59:59 isVisible(return dao.list().size() 0);// performance issues But maybe we can have the best from both approaches. This is an copy/paste from java.awt.Component: public boolean isVisible() { return isVisible_NoClientCode(); } final boolean isVisible_NoClientCode() { return visible; } There are some points in the awt framework were the isVisible method is not used in benefit of isVisible_NoClientCode I'm in favor of create an final isVisible/Enabled version and change the Wicket core to use it. Also maintain the hotspot to users provide their isVisible/Enable implementations that will serve to feed the core component state. On Mon, Nov 29, 2010 at 4:56 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: ive run into plenty of weird problems with overrides, but maybe because this was in a high concurrency app where data changed frequently. the problems arise from the fact that the value returned from isvisible() can change while we are doing traversals, etc. eg we run a traversal for all visible components and put them in a list. later we iterate over the list and try to render these components. the render function also checks their visibility and if they are no longer visible it throws an exception. if isvisible() override depends on some external factor like the database there is a small window where the value can change and now you can have a weird exception: such as tried to invoke a listener on a component that is not visible or not enabled. these are very intermittent and damn near impossible to reproduce. another problem is performance. isvisible() is called multiple times during the request and if it depends on the database it can be a performance problem. in fact a couple of users have complained about this on the list in the past. at least now we have an easy solution for them - use onconfigure(). so as of right now the developers have two choices: override isvisible() and potentially suffer the consequences. or, override onconfigure() and set visibility there in a more deterministic fashion. -igor On Mon, Nov 29, 2010 at 10:21 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: To expand, unless I'm missing something (new?), things are really only problematic when both the mutable value and the override are mixed. In a way, I think that using the override is 'more pure', as it's a simple function that is executed when needed, whereas mutable state can be harder to deal with when trying to figure out how it got to be in that state. So, sorry Igor, but we disagree on this one. Eelco On Mon, Nov 29, 2010 at 10:13 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: Niether is evil. It has potential pitfalls, which you should just be aware of. We use such overrides all over the place and never have problems with them either. :-) Avoiding it is safer, but also more verbose (in 1.3.x at least). Eelco On Mon, Nov 29, 2010 at 9:49 AM, Igor Vaynberg igor.vaynb...@gmail.com wrote: On Mon, Nov 29, 2010 at 9:35 AM, Sven Meier s...@meiers.net wrote: Hi Douglas, WICKET-3171 describes a problematic case, where visibility of a component changes while its form is being processed. In our projects we're overriding isVisible() where appropriate and never encountered a similar problem. I'd say WICKET-3171 is the rare 5% usecase. What's next, is overriding isEnabled() going to be declared evil too? ;) yes -igor Sven On Mon, 2010-11-29 at 11:22 -0600, Douglas Ferguson wrote: Can you explain why? We have done this all over the place. D/ On Nov 29, 2010, at 10:00 AM, Martin Grigorov wrote: The recommended way since a few 1.4
Re: overriding isVisible bad?
Yes, for instance if an component contribute to the HTML header with an CSS resource, and if this component return false to the isVisible test when the HtmlHeaderContainer is traversing the component hierarchy requesting visible components to contribute to header, and later return true when the Component#render method offer the MarkupStream to it render it self at the response: we will see the components not working as expected (rendered with its proper CSS) On Tue, Nov 30, 2010 at 2:30 PM, Peter Ertl pe...@gmx.org wrote: Jus to get it right ... is the following statement correct? isVisible() will work as expected when the criteria for visibility does not change during render Am 30.11.2010 um 17:02 schrieb Pedro Santos: I understand the concern about possible isVisible implementations like isVisible(return currentlyTime 10:00:00;) //imagine this component being rendered at 09:59:59 isVisible(return dao.list().size() 0);// performance issues But maybe we can have the best from both approaches. This is an copy/paste from java.awt.Component: public boolean isVisible() { return isVisible_NoClientCode(); } final boolean isVisible_NoClientCode() { return visible; } There are some points in the awt framework were the isVisible method is not used in benefit of isVisible_NoClientCode I'm in favor of create an final isVisible/Enabled version and change the Wicket core to use it. Also maintain the hotspot to users provide their isVisible/Enable implementations that will serve to feed the core component state. On Mon, Nov 29, 2010 at 4:56 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: ive run into plenty of weird problems with overrides, but maybe because this was in a high concurrency app where data changed frequently. the problems arise from the fact that the value returned from isvisible() can change while we are doing traversals, etc. eg we run a traversal for all visible components and put them in a list. later we iterate over the list and try to render these components. the render function also checks their visibility and if they are no longer visible it throws an exception. if isvisible() override depends on some external factor like the database there is a small window where the value can change and now you can have a weird exception: such as tried to invoke a listener on a component that is not visible or not enabled. these are very intermittent and damn near impossible to reproduce. another problem is performance. isvisible() is called multiple times during the request and if it depends on the database it can be a performance problem. in fact a couple of users have complained about this on the list in the past. at least now we have an easy solution for them - use onconfigure(). so as of right now the developers have two choices: override isvisible() and potentially suffer the consequences. or, override onconfigure() and set visibility there in a more deterministic fashion. -igor On Mon, Nov 29, 2010 at 10:21 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: To expand, unless I'm missing something (new?), things are really only problematic when both the mutable value and the override are mixed. In a way, I think that using the override is 'more pure', as it's a simple function that is executed when needed, whereas mutable state can be harder to deal with when trying to figure out how it got to be in that state. So, sorry Igor, but we disagree on this one. Eelco On Mon, Nov 29, 2010 at 10:13 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: Niether is evil. It has potential pitfalls, which you should just be aware of. We use such overrides all over the place and never have problems with them either. :-) Avoiding it is safer, but also more verbose (in 1.3.x at least). Eelco On Mon, Nov 29, 2010 at 9:49 AM, Igor Vaynberg igor.vaynb...@gmail.com wrote: On Mon, Nov 29, 2010 at 9:35 AM, Sven Meier s...@meiers.net wrote: Hi Douglas, WICKET-3171 describes a problematic case, where visibility of a component changes while its form is being processed. In our projects we're overriding isVisible() where appropriate and never encountered a similar problem. I'd say WICKET-3171 is the rare 5% usecase. What's next, is overriding isEnabled() going to be declared evil too? ;) yes -igor Sven On Mon, 2010-11-29 at 11:22 -0600, Douglas Ferguson wrote: Can you explain why? We have done this all over the place. D/ On Nov 29, 2010, at 10:00 AM, Martin Grigorov wrote: The recommended way since a few 1.4 releases is to override onConfigure() and call setVisible(true|false) depending on your conditions. On Mon, Nov 29, 2010 at 4:49 PM, Douglas Ferguson doug...@douglasferguson.us wrote: Igor posted a comment to this bug saying that overriding isVisible() is
Re: overriding isVisible bad?
If user click an link, it will change the value of some property at the process_event request cycle step. Then the processor will go to the respond step, will invoke every component before render method which will end up invoking the Component#configure and updating the visibility/enabled state (even if it changes, we are able to work with the updated state). So when the this component has the opportunity to render it self, it will be aware its update state. On Tue, Nov 30, 2010 at 2:39 PM, Igor Vaynberg igor.vaynb...@gmail.comwrote: there are other places that should be checked though. for example before we invoke a listener on the component we should check again to make sure that visibility hasnt changed. eg if visibility depends on some property of the user clicking the link that changed between render and clicking the link. -igor On Tue, Nov 30, 2010 at 8:30 AM, Pedro Santos pedros...@gmail.com wrote: An implementation idea: Component { public final void configure() { if (!getFlag(FLAG_CONFIGURED)) { setVisible_NoClientCode(isVisible()); //we only check the user isVisible in here onConfigure(); setFlag(FLAG_CONFIGURED, true); } } } On Tue, Nov 30, 2010 at 2:16 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: so how is it different if they can still override something that needs to be checked all the time? -igor On Tue, Nov 30, 2010 at 8:02 AM, Pedro Santos pedros...@gmail.com wrote: I understand the concern about possible isVisible implementations like isVisible(return currentlyTime 10:00:00;) //imagine this component being rendered at 09:59:59 isVisible(return dao.list().size() 0);// performance issues But maybe we can have the best from both approaches. This is an copy/paste from java.awt.Component: public boolean isVisible() { return isVisible_NoClientCode(); } final boolean isVisible_NoClientCode() { return visible; } There are some points in the awt framework were the isVisible method is not used in benefit of isVisible_NoClientCode I'm in favor of create an final isVisible/Enabled version and change the Wicket core to use it. Also maintain the hotspot to users provide their isVisible/Enable implementations that will serve to feed the core component state. On Mon, Nov 29, 2010 at 4:56 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: ive run into plenty of weird problems with overrides, but maybe because this was in a high concurrency app where data changed frequently. the problems arise from the fact that the value returned from isvisible() can change while we are doing traversals, etc. eg we run a traversal for all visible components and put them in a list. later we iterate over the list and try to render these components. the render function also checks their visibility and if they are no longer visible it throws an exception. if isvisible() override depends on some external factor like the database there is a small window where the value can change and now you can have a weird exception: such as tried to invoke a listener on a component that is not visible or not enabled. these are very intermittent and damn near impossible to reproduce. another problem is performance. isvisible() is called multiple times during the request and if it depends on the database it can be a performance problem. in fact a couple of users have complained about this on the list in the past. at least now we have an easy solution for them - use onconfigure(). so as of right now the developers have two choices: override isvisible() and potentially suffer the consequences. or, override onconfigure() and set visibility there in a more deterministic fashion. -igor On Mon, Nov 29, 2010 at 10:21 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: To expand, unless I'm missing something (new?), things are really only problematic when both the mutable value and the override are mixed. In a way, I think that using the override is 'more pure', as it's a simple function that is executed when needed, whereas mutable state can be harder to deal with when trying to figure out how it got to be in that state. So, sorry Igor, but we disagree on this one. Eelco On Mon, Nov 29, 2010 at 10:13 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: Niether is evil. It has potential pitfalls, which you should just be aware of. We use such overrides all over the place and never have problems with them either. :-) Avoiding it is safer, but also more verbose (in 1.3.x at least). Eelco On Mon, Nov 29, 2010 at 9:49 AM, Igor Vaynberg igor.vaynb...@gmail.com wrote: On Mon, Nov 29, 2010 at 9:35 AM, Sven Meier s...@meiers.net
Re: overriding isVisible bad?
currently we only invoke configure before the render. this would mean we would have to invoke it before processing a listener, clearing the cache, and then invoking it again before render. i wonder if that is enough places to invoke it -igor On Tue, Nov 30, 2010 at 9:15 AM, Pedro Santos pedros...@gmail.com wrote: If user click an link, it will change the value of some property at the process_event request cycle step. Then the processor will go to the respond step, will invoke every component before render method which will end up invoking the Component#configure and updating the visibility/enabled state (even if it changes, we are able to work with the updated state). So when the this component has the opportunity to render it self, it will be aware its update state. On Tue, Nov 30, 2010 at 2:39 PM, Igor Vaynberg igor.vaynb...@gmail.comwrote: there are other places that should be checked though. for example before we invoke a listener on the component we should check again to make sure that visibility hasnt changed. eg if visibility depends on some property of the user clicking the link that changed between render and clicking the link. -igor On Tue, Nov 30, 2010 at 8:30 AM, Pedro Santos pedros...@gmail.com wrote: An implementation idea: Component { public final void configure() { if (!getFlag(FLAG_CONFIGURED)) { setVisible_NoClientCode(isVisible()); //we only check the user isVisible in here onConfigure(); setFlag(FLAG_CONFIGURED, true); } } } On Tue, Nov 30, 2010 at 2:16 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: so how is it different if they can still override something that needs to be checked all the time? -igor On Tue, Nov 30, 2010 at 8:02 AM, Pedro Santos pedros...@gmail.com wrote: I understand the concern about possible isVisible implementations like isVisible(return currentlyTime 10:00:00;) //imagine this component being rendered at 09:59:59 isVisible(return dao.list().size() 0);// performance issues But maybe we can have the best from both approaches. This is an copy/paste from java.awt.Component: public boolean isVisible() { return isVisible_NoClientCode(); } final boolean isVisible_NoClientCode() { return visible; } There are some points in the awt framework were the isVisible method is not used in benefit of isVisible_NoClientCode I'm in favor of create an final isVisible/Enabled version and change the Wicket core to use it. Also maintain the hotspot to users provide their isVisible/Enable implementations that will serve to feed the core component state. On Mon, Nov 29, 2010 at 4:56 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: ive run into plenty of weird problems with overrides, but maybe because this was in a high concurrency app where data changed frequently. the problems arise from the fact that the value returned from isvisible() can change while we are doing traversals, etc. eg we run a traversal for all visible components and put them in a list. later we iterate over the list and try to render these components. the render function also checks their visibility and if they are no longer visible it throws an exception. if isvisible() override depends on some external factor like the database there is a small window where the value can change and now you can have a weird exception: such as tried to invoke a listener on a component that is not visible or not enabled. these are very intermittent and damn near impossible to reproduce. another problem is performance. isvisible() is called multiple times during the request and if it depends on the database it can be a performance problem. in fact a couple of users have complained about this on the list in the past. at least now we have an easy solution for them - use onconfigure(). so as of right now the developers have two choices: override isvisible() and potentially suffer the consequences. or, override onconfigure() and set visibility there in a more deterministic fashion. -igor On Mon, Nov 29, 2010 at 10:21 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: To expand, unless I'm missing something (new?), things are really only problematic when both the mutable value and the override are mixed. In a way, I think that using the override is 'more pure', as it's a simple function that is executed when needed, whereas mutable state can be harder to deal with when trying to figure out how it got to be in that state. So, sorry Igor, but we disagree on this one. Eelco On Mon, Nov 29, 2010 at 10:13 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: Niether is evil. It has potential pitfalls, which you should just be aware of.
Re: overriding isVisible bad?
On Tue, Nov 30, 2010 at 12:55 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: On Mon, Nov 29, 2010 at 11:51 PM, Juergen Donnerstag juergen.donners...@gmail.com wrote: I'm curious. Which ideas would you steal from SiteBricks and JaxRS? There are also many interesting ideas in Apache Sling. How it uses OSGi for modularization, comes with a 'launchpad' and has a pretty flexible bootstrapping mechanism and looks like a good power-to-weight ratio.
Re: overriding isVisible bad?
I need to look better on which core components are relying on an updated visibility/enabled state at the process event time, and why the last rendered state wouldn't be enough to them to work nicely. On Tue, Nov 30, 2010 at 3:19 PM, Igor Vaynberg igor.vaynb...@gmail.comwrote: currently we only invoke configure before the render. this would mean we would have to invoke it before processing a listener, clearing the cache, and then invoking it again before render. i wonder if that is enough places to invoke it -igor On Tue, Nov 30, 2010 at 9:15 AM, Pedro Santos pedros...@gmail.com wrote: If user click an link, it will change the value of some property at the process_event request cycle step. Then the processor will go to the respond step, will invoke every component before render method which will end up invoking the Component#configure and updating the visibility/enabled state (even if it changes, we are able to work with the updated state). So when the this component has the opportunity to render it self, it will be aware its update state. On Tue, Nov 30, 2010 at 2:39 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: there are other places that should be checked though. for example before we invoke a listener on the component we should check again to make sure that visibility hasnt changed. eg if visibility depends on some property of the user clicking the link that changed between render and clicking the link. -igor On Tue, Nov 30, 2010 at 8:30 AM, Pedro Santos pedros...@gmail.com wrote: An implementation idea: Component { public final void configure() { if (!getFlag(FLAG_CONFIGURED)) { setVisible_NoClientCode(isVisible()); //we only check the user isVisible in here onConfigure(); setFlag(FLAG_CONFIGURED, true); } } } On Tue, Nov 30, 2010 at 2:16 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: so how is it different if they can still override something that needs to be checked all the time? -igor On Tue, Nov 30, 2010 at 8:02 AM, Pedro Santos pedros...@gmail.com wrote: I understand the concern about possible isVisible implementations like isVisible(return currentlyTime 10:00:00;) //imagine this component being rendered at 09:59:59 isVisible(return dao.list().size() 0);// performance issues But maybe we can have the best from both approaches. This is an copy/paste from java.awt.Component: public boolean isVisible() { return isVisible_NoClientCode(); } final boolean isVisible_NoClientCode() { return visible; } There are some points in the awt framework were the isVisible method is not used in benefit of isVisible_NoClientCode I'm in favor of create an final isVisible/Enabled version and change the Wicket core to use it. Also maintain the hotspot to users provide their isVisible/Enable implementations that will serve to feed the core component state. On Mon, Nov 29, 2010 at 4:56 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: ive run into plenty of weird problems with overrides, but maybe because this was in a high concurrency app where data changed frequently. the problems arise from the fact that the value returned from isvisible() can change while we are doing traversals, etc. eg we run a traversal for all visible components and put them in a list. later we iterate over the list and try to render these components. the render function also checks their visibility and if they are no longer visible it throws an exception. if isvisible() override depends on some external factor like the database there is a small window where the value can change and now you can have a weird exception: such as tried to invoke a listener on a component that is not visible or not enabled. these are very intermittent and damn near impossible to reproduce. another problem is performance. isvisible() is called multiple times during the request and if it depends on the database it can be a performance problem. in fact a couple of users have complained about this on the list in the past. at least now we have an easy solution for them - use onconfigure(). so as of right now the developers have two choices: override isvisible() and potentially suffer the consequences. or, override onconfigure() and set visibility there in a more deterministic fashion. -igor On Mon, Nov 29, 2010 at 10:21 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: To expand, unless I'm missing something (new?), things are really only problematic when both the mutable value and the override are mixed. In a way, I think that using the override is 'more
Re: overriding isVisible bad?
an easy example is security. a user views a page that allows them to delete another user meanwhile their permissions are tweaked and they can no longer delete other users half an hour later the user clicks the delete button - this should fail, but wont if we are using last-rendered state. -igor On Tue, Nov 30, 2010 at 11:18 AM, Pedro Santos pedros...@gmail.com wrote: I need to look better on which core components are relying on an updated visibility/enabled state at the process event time, and why the last rendered state wouldn't be enough to them to work nicely. On Tue, Nov 30, 2010 at 3:19 PM, Igor Vaynberg igor.vaynb...@gmail.comwrote: currently we only invoke configure before the render. this would mean we would have to invoke it before processing a listener, clearing the cache, and then invoking it again before render. i wonder if that is enough places to invoke it -igor On Tue, Nov 30, 2010 at 9:15 AM, Pedro Santos pedros...@gmail.com wrote: If user click an link, it will change the value of some property at the process_event request cycle step. Then the processor will go to the respond step, will invoke every component before render method which will end up invoking the Component#configure and updating the visibility/enabled state (even if it changes, we are able to work with the updated state). So when the this component has the opportunity to render it self, it will be aware its update state. On Tue, Nov 30, 2010 at 2:39 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: there are other places that should be checked though. for example before we invoke a listener on the component we should check again to make sure that visibility hasnt changed. eg if visibility depends on some property of the user clicking the link that changed between render and clicking the link. -igor On Tue, Nov 30, 2010 at 8:30 AM, Pedro Santos pedros...@gmail.com wrote: An implementation idea: Component { public final void configure() { if (!getFlag(FLAG_CONFIGURED)) { setVisible_NoClientCode(isVisible()); //we only check the user isVisible in here onConfigure(); setFlag(FLAG_CONFIGURED, true); } } } On Tue, Nov 30, 2010 at 2:16 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: so how is it different if they can still override something that needs to be checked all the time? -igor On Tue, Nov 30, 2010 at 8:02 AM, Pedro Santos pedros...@gmail.com wrote: I understand the concern about possible isVisible implementations like isVisible(return currentlyTime 10:00:00;) //imagine this component being rendered at 09:59:59 isVisible(return dao.list().size() 0);// performance issues But maybe we can have the best from both approaches. This is an copy/paste from java.awt.Component: public boolean isVisible() { return isVisible_NoClientCode(); } final boolean isVisible_NoClientCode() { return visible; } There are some points in the awt framework were the isVisible method is not used in benefit of isVisible_NoClientCode I'm in favor of create an final isVisible/Enabled version and change the Wicket core to use it. Also maintain the hotspot to users provide their isVisible/Enable implementations that will serve to feed the core component state. On Mon, Nov 29, 2010 at 4:56 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: ive run into plenty of weird problems with overrides, but maybe because this was in a high concurrency app where data changed frequently. the problems arise from the fact that the value returned from isvisible() can change while we are doing traversals, etc. eg we run a traversal for all visible components and put them in a list. later we iterate over the list and try to render these components. the render function also checks their visibility and if they are no longer visible it throws an exception. if isvisible() override depends on some external factor like the database there is a small window where the value can change and now you can have a weird exception: such as tried to invoke a listener on a component that is not visible or not enabled. these are very intermittent and damn near impossible to reproduce. another problem is performance. isvisible() is called multiple times during the request and if it depends on the database it can be a performance problem. in fact a couple of users have complained about this on the list in the past. at least now we have an easy solution for them - use onconfigure(). so as of right now the developers have two choices: override isvisible() and potentially suffer the consequences. or, override onconfigure() and set visibility
Re: overriding isVisible bad?
I have a different point of view, the HTTP imposes us some limitations, we will hardly have an good synchronization between the component state on browser and server using only HTTP conversation. So it is mandatory the service layer to respect the described security restriction. On Tue, Nov 30, 2010 at 5:32 PM, Igor Vaynberg igor.vaynb...@gmail.comwrote: an easy example is security. a user views a page that allows them to delete another user meanwhile their permissions are tweaked and they can no longer delete other users half an hour later the user clicks the delete button - this should fail, but wont if we are using last-rendered state. -igor On Tue, Nov 30, 2010 at 11:18 AM, Pedro Santos pedros...@gmail.com wrote: I need to look better on which core components are relying on an updated visibility/enabled state at the process event time, and why the last rendered state wouldn't be enough to them to work nicely. On Tue, Nov 30, 2010 at 3:19 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: currently we only invoke configure before the render. this would mean we would have to invoke it before processing a listener, clearing the cache, and then invoking it again before render. i wonder if that is enough places to invoke it -igor On Tue, Nov 30, 2010 at 9:15 AM, Pedro Santos pedros...@gmail.com wrote: If user click an link, it will change the value of some property at the process_event request cycle step. Then the processor will go to the respond step, will invoke every component before render method which will end up invoking the Component#configure and updating the visibility/enabled state (even if it changes, we are able to work with the updated state). So when the this component has the opportunity to render it self, it will be aware its update state. On Tue, Nov 30, 2010 at 2:39 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: there are other places that should be checked though. for example before we invoke a listener on the component we should check again to make sure that visibility hasnt changed. eg if visibility depends on some property of the user clicking the link that changed between render and clicking the link. -igor On Tue, Nov 30, 2010 at 8:30 AM, Pedro Santos pedros...@gmail.com wrote: An implementation idea: Component { public final void configure() { if (!getFlag(FLAG_CONFIGURED)) { setVisible_NoClientCode(isVisible()); //we only check the user isVisible in here onConfigure(); setFlag(FLAG_CONFIGURED, true); } } } On Tue, Nov 30, 2010 at 2:16 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: so how is it different if they can still override something that needs to be checked all the time? -igor On Tue, Nov 30, 2010 at 8:02 AM, Pedro Santos pedros...@gmail.com wrote: I understand the concern about possible isVisible implementations like isVisible(return currentlyTime 10:00:00;) //imagine this component being rendered at 09:59:59 isVisible(return dao.list().size() 0);// performance issues But maybe we can have the best from both approaches. This is an copy/paste from java.awt.Component: public boolean isVisible() { return isVisible_NoClientCode(); } final boolean isVisible_NoClientCode() { return visible; } There are some points in the awt framework were the isVisible method is not used in benefit of isVisible_NoClientCode I'm in favor of create an final isVisible/Enabled version and change the Wicket core to use it. Also maintain the hotspot to users provide their isVisible/Enable implementations that will serve to feed the core component state. On Mon, Nov 29, 2010 at 4:56 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: ive run into plenty of weird problems with overrides, but maybe because this was in a high concurrency app where data changed frequently. the problems arise from the fact that the value returned from isvisible() can change while we are doing traversals, etc. eg we run a traversal for all visible components and put them in a list. later we iterate over the list and try to render these components. the render function also checks their visibility and if they are no longer visible it throws an exception. if isvisible() override depends on some external factor like the database there is a small window where the value can change and now you can have a weird exception: such as tried to invoke a listener on a component that is not visible or not enabled. these are very intermittent and damn near impossible to
Re: overriding isVisible bad?
i would be happy if that was good enough. in the past it hasnt been, thats why we have the current solution. maybe we can try it again in 1.5 and see what happens. -igor On Tue, Nov 30, 2010 at 11:44 AM, Pedro Santos pedros...@gmail.com wrote: I have a different point of view, the HTTP imposes us some limitations, we will hardly have an good synchronization between the component state on browser and server using only HTTP conversation. So it is mandatory the service layer to respect the described security restriction. On Tue, Nov 30, 2010 at 5:32 PM, Igor Vaynberg igor.vaynb...@gmail.comwrote: an easy example is security. a user views a page that allows them to delete another user meanwhile their permissions are tweaked and they can no longer delete other users half an hour later the user clicks the delete button - this should fail, but wont if we are using last-rendered state. -igor On Tue, Nov 30, 2010 at 11:18 AM, Pedro Santos pedros...@gmail.com wrote: I need to look better on which core components are relying on an updated visibility/enabled state at the process event time, and why the last rendered state wouldn't be enough to them to work nicely. On Tue, Nov 30, 2010 at 3:19 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: currently we only invoke configure before the render. this would mean we would have to invoke it before processing a listener, clearing the cache, and then invoking it again before render. i wonder if that is enough places to invoke it -igor On Tue, Nov 30, 2010 at 9:15 AM, Pedro Santos pedros...@gmail.com wrote: If user click an link, it will change the value of some property at the process_event request cycle step. Then the processor will go to the respond step, will invoke every component before render method which will end up invoking the Component#configure and updating the visibility/enabled state (even if it changes, we are able to work with the updated state). So when the this component has the opportunity to render it self, it will be aware its update state. On Tue, Nov 30, 2010 at 2:39 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: there are other places that should be checked though. for example before we invoke a listener on the component we should check again to make sure that visibility hasnt changed. eg if visibility depends on some property of the user clicking the link that changed between render and clicking the link. -igor On Tue, Nov 30, 2010 at 8:30 AM, Pedro Santos pedros...@gmail.com wrote: An implementation idea: Component { public final void configure() { if (!getFlag(FLAG_CONFIGURED)) { setVisible_NoClientCode(isVisible()); //we only check the user isVisible in here onConfigure(); setFlag(FLAG_CONFIGURED, true); } } } On Tue, Nov 30, 2010 at 2:16 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: so how is it different if they can still override something that needs to be checked all the time? -igor On Tue, Nov 30, 2010 at 8:02 AM, Pedro Santos pedros...@gmail.com wrote: I understand the concern about possible isVisible implementations like isVisible(return currentlyTime 10:00:00;) //imagine this component being rendered at 09:59:59 isVisible(return dao.list().size() 0);// performance issues But maybe we can have the best from both approaches. This is an copy/paste from java.awt.Component: public boolean isVisible() { return isVisible_NoClientCode(); } final boolean isVisible_NoClientCode() { return visible; } There are some points in the awt framework were the isVisible method is not used in benefit of isVisible_NoClientCode I'm in favor of create an final isVisible/Enabled version and change the Wicket core to use it. Also maintain the hotspot to users provide their isVisible/Enable implementations that will serve to feed the core component state. On Mon, Nov 29, 2010 at 4:56 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: ive run into plenty of weird problems with overrides, but maybe because this was in a high concurrency app where data changed frequently. the problems arise from the fact that the value returned from isvisible() can change while we are doing traversals, etc. eg we run a traversal for all visible components and put them in a list. later we iterate over the list and try to render these components. the render function also checks their visibility and if they are no longer visible it throws an exception. if isvisible() override depends on some external factor like the database there is
overriding isVisible bad?
Igor posted a comment to this bug saying that overriding isVisible() is evil https://issues.apache.org/jira/browse/WICKET-3171 I was surprised by this and am curious to hear more. D/
Re: overriding isVisible bad?
The recommended way since a few 1.4 releases is to override onConfigure() and call setVisible(true|false) depending on your conditions. On Mon, Nov 29, 2010 at 4:49 PM, Douglas Ferguson doug...@douglasferguson.us wrote: Igor posted a comment to this bug saying that overriding isVisible() is evil https://issues.apache.org/jira/browse/WICKET-3171 I was surprised by this and am curious to hear more. D/
Re: overriding isVisible bad?
Can you explain why? We have done this all over the place. D/ On Nov 29, 2010, at 10:00 AM, Martin Grigorov wrote: The recommended way since a few 1.4 releases is to override onConfigure() and call setVisible(true|false) depending on your conditions. On Mon, Nov 29, 2010 at 4:49 PM, Douglas Ferguson doug...@douglasferguson.us wrote: Igor posted a comment to this bug saying that overriding isVisible() is evil https://issues.apache.org/jira/browse/WICKET-3171 I was surprised by this and am curious to hear more. D/
Re: overriding isVisible bad?
Hi Douglas, WICKET-3171 describes a problematic case, where visibility of a component changes while its form is being processed. In our projects we're overriding isVisible() where appropriate and never encountered a similar problem. I'd say WICKET-3171 is the rare 5% usecase. What's next, is overriding isEnabled() going to be declared evil too? ;) Sven On Mon, 2010-11-29 at 11:22 -0600, Douglas Ferguson wrote: Can you explain why? We have done this all over the place. D/ On Nov 29, 2010, at 10:00 AM, Martin Grigorov wrote: The recommended way since a few 1.4 releases is to override onConfigure() and call setVisible(true|false) depending on your conditions. On Mon, Nov 29, 2010 at 4:49 PM, Douglas Ferguson doug...@douglasferguson.us wrote: Igor posted a comment to this bug saying that overriding isVisible() is evil https://issues.apache.org/jira/browse/WICKET-3171 I was surprised by this and am curious to hear more. D/
Re: overriding isVisible bad?
Niether is evil. It has potential pitfalls, which you should just be aware of. We use such overrides all over the place and never have problems with them either. :-) Avoiding it is safer, but also more verbose (in 1.3.x at least). Eelco On Mon, Nov 29, 2010 at 9:49 AM, Igor Vaynberg igor.vaynb...@gmail.com wrote: On Mon, Nov 29, 2010 at 9:35 AM, Sven Meier s...@meiers.net wrote: Hi Douglas, WICKET-3171 describes a problematic case, where visibility of a component changes while its form is being processed. In our projects we're overriding isVisible() where appropriate and never encountered a similar problem. I'd say WICKET-3171 is the rare 5% usecase. What's next, is overriding isEnabled() going to be declared evil too? ;) yes -igor Sven On Mon, 2010-11-29 at 11:22 -0600, Douglas Ferguson wrote: Can you explain why? We have done this all over the place. D/ On Nov 29, 2010, at 10:00 AM, Martin Grigorov wrote: The recommended way since a few 1.4 releases is to override onConfigure() and call setVisible(true|false) depending on your conditions. On Mon, Nov 29, 2010 at 4:49 PM, Douglas Ferguson doug...@douglasferguson.us wrote: Igor posted a comment to this bug saying that overriding isVisible() is evil https://issues.apache.org/jira/browse/WICKET-3171 I was surprised by this and am curious to hear more. D/
Re: overriding isVisible bad?
To expand, unless I'm missing something (new?), things are really only problematic when both the mutable value and the override are mixed. In a way, I think that using the override is 'more pure', as it's a simple function that is executed when needed, whereas mutable state can be harder to deal with when trying to figure out how it got to be in that state. So, sorry Igor, but we disagree on this one. Eelco On Mon, Nov 29, 2010 at 10:13 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: Niether is evil. It has potential pitfalls, which you should just be aware of. We use such overrides all over the place and never have problems with them either. :-) Avoiding it is safer, but also more verbose (in 1.3.x at least). Eelco On Mon, Nov 29, 2010 at 9:49 AM, Igor Vaynberg igor.vaynb...@gmail.com wrote: On Mon, Nov 29, 2010 at 9:35 AM, Sven Meier s...@meiers.net wrote: Hi Douglas, WICKET-3171 describes a problematic case, where visibility of a component changes while its form is being processed. In our projects we're overriding isVisible() where appropriate and never encountered a similar problem. I'd say WICKET-3171 is the rare 5% usecase. What's next, is overriding isEnabled() going to be declared evil too? ;) yes -igor Sven On Mon, 2010-11-29 at 11:22 -0600, Douglas Ferguson wrote: Can you explain why? We have done this all over the place. D/ On Nov 29, 2010, at 10:00 AM, Martin Grigorov wrote: The recommended way since a few 1.4 releases is to override onConfigure() and call setVisible(true|false) depending on your conditions. On Mon, Nov 29, 2010 at 4:49 PM, Douglas Ferguson doug...@douglasferguson.us wrote: Igor posted a comment to this bug saying that overriding isVisible() is evil https://issues.apache.org/jira/browse/WICKET-3171 I was surprised by this and am curious to hear more. D/
Re: overriding isVisible bad?
ive run into plenty of weird problems with overrides, but maybe because this was in a high concurrency app where data changed frequently. the problems arise from the fact that the value returned from isvisible() can change while we are doing traversals, etc. eg we run a traversal for all visible components and put them in a list. later we iterate over the list and try to render these components. the render function also checks their visibility and if they are no longer visible it throws an exception. if isvisible() override depends on some external factor like the database there is a small window where the value can change and now you can have a weird exception: such as tried to invoke a listener on a component that is not visible or not enabled. these are very intermittent and damn near impossible to reproduce. another problem is performance. isvisible() is called multiple times during the request and if it depends on the database it can be a performance problem. in fact a couple of users have complained about this on the list in the past. at least now we have an easy solution for them - use onconfigure(). so as of right now the developers have two choices: override isvisible() and potentially suffer the consequences. or, override onconfigure() and set visibility there in a more deterministic fashion. -igor On Mon, Nov 29, 2010 at 10:21 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: To expand, unless I'm missing something (new?), things are really only problematic when both the mutable value and the override are mixed. In a way, I think that using the override is 'more pure', as it's a simple function that is executed when needed, whereas mutable state can be harder to deal with when trying to figure out how it got to be in that state. So, sorry Igor, but we disagree on this one. Eelco On Mon, Nov 29, 2010 at 10:13 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: Niether is evil. It has potential pitfalls, which you should just be aware of. We use such overrides all over the place and never have problems with them either. :-) Avoiding it is safer, but also more verbose (in 1.3.x at least). Eelco On Mon, Nov 29, 2010 at 9:49 AM, Igor Vaynberg igor.vaynb...@gmail.com wrote: On Mon, Nov 29, 2010 at 9:35 AM, Sven Meier s...@meiers.net wrote: Hi Douglas, WICKET-3171 describes a problematic case, where visibility of a component changes while its form is being processed. In our projects we're overriding isVisible() where appropriate and never encountered a similar problem. I'd say WICKET-3171 is the rare 5% usecase. What's next, is overriding isEnabled() going to be declared evil too? ;) yes -igor Sven On Mon, 2010-11-29 at 11:22 -0600, Douglas Ferguson wrote: Can you explain why? We have done this all over the place. D/ On Nov 29, 2010, at 10:00 AM, Martin Grigorov wrote: The recommended way since a few 1.4 releases is to override onConfigure() and call setVisible(true|false) depending on your conditions. On Mon, Nov 29, 2010 at 4:49 PM, Douglas Ferguson doug...@douglasferguson.us wrote: Igor posted a comment to this bug saying that overriding isVisible() is evil https://issues.apache.org/jira/browse/WICKET-3171 I was surprised by this and am curious to hear more. D/
Re: overriding isVisible bad?
how so? we added something new that we think will work better. -igor On Mon, Nov 29, 2010 at 12:45 PM, James Carman ja...@carmanconsulting.com wrote: On Mon, Nov 29, 2010 at 1:13 PM, Eelco Hillenius eelco.hillen...@gmail.com wrote: Niether is evil. It has potential pitfalls, which you should just be aware of. We use such overrides all over the place and never have problems with them either. :-) Avoiding it is safer, but also more verbose (in 1.3.x at least). Well, in the past, the canned answer was override isEnabled/isVisible. Changing that paradigm and doing a complete 180 is troubling.
Re: overriding isVisible bad?
I am glad we have something new that's better, but going from do this to this is evil is the troubling part. A lot of us have a lot of code that is based on the previous advice. Now declaring that code is evil is kind of scary, especially in the middle of a major version. If something is evil, then we should probably try to avoid it, so what do we do, go clean up all of our existing code (and re-test it)? On Mon, Nov 29, 2010 at 3:52 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: how so? we added something new that we think will work better. -igor On Mon, Nov 29, 2010 at 12:45 PM, James Carman ja...@carmanconsulting.com wrote: On Mon, Nov 29, 2010 at 1:13 PM, Eelco Hillenius eelco.hillen...@gmail.com wrote: Niether is evil. It has potential pitfalls, which you should just be aware of. We use such overrides all over the place and never have problems with them either. :-) Avoiding it is safer, but also more verbose (in 1.3.x at least). Well, in the past, the canned answer was override isEnabled/isVisible. Changing that paradigm and doing a complete 180 is troubling.
Re: overriding isVisible bad?
if it works for you keep it. all we did was give you a better/safer alternative. -igor On Mon, Nov 29, 2010 at 12:59 PM, James Carman ja...@carmanconsulting.com wrote: I am glad we have something new that's better, but going from do this to this is evil is the troubling part. A lot of us have a lot of code that is based on the previous advice. Now declaring that code is evil is kind of scary, especially in the middle of a major version. If something is evil, then we should probably try to avoid it, so what do we do, go clean up all of our existing code (and re-test it)? On Mon, Nov 29, 2010 at 3:52 PM, Igor Vaynberg igor.vaynb...@gmail.com wrote: how so? we added something new that we think will work better. -igor On Mon, Nov 29, 2010 at 12:45 PM, James Carman ja...@carmanconsulting.com wrote: On Mon, Nov 29, 2010 at 1:13 PM, Eelco Hillenius eelco.hillen...@gmail.com wrote: Niether is evil. It has potential pitfalls, which you should just be aware of. We use such overrides all over the place and never have problems with them either. :-) Avoiding it is safer, but also more verbose (in 1.3.x at least). Well, in the past, the canned answer was override isEnabled/isVisible. Changing that paradigm and doing a complete 180 is troubling.
Re: overriding isVisible bad?
Well, in the past, the canned answer was override isEnabled/isVisible. Changing that paradigm and doing a complete 180 is troubling. I don't think that's the case though. We've had many discussions on this list (and in private even), and we've always felt uneasy about supporting two rather than one way of achieving the same thing. Ultimately we decided to support both, but there have been several times where we were near making isVisible final. Eelco
Re: overriding isVisible bad?
If wicket was going to be coded over again, would you make isEnabled and/or isVisible final methods? Might the non-finality of the methods be deprecated in some future release? The majority of the isXXX methods in Component are final. Richard -- Quis custodiet ipsos custodes
Re: overriding isVisible bad?
it has nothing to do with requiring a function to be set. the problem is that the function is free to change its mind at any moment, but we rely on it returning the same value during some fixed periods of time. if we truly want to support isvisible() we would need to cache/memoize the value for the period we assume it to be constant. Yes, that's what I'm saying. but, defining these caching regions is very difficult, and maintaining them as we work on core we be even more difficult still. I don't think it would be that difficult, but certainly not something worth doing at this stage. Eelco
Re: overriding isVisible bad?
On Mon, Nov 29, 2010 at 7:45 PM, richard emberson richard.ember...@gmail.com wrote: If wicket was going to be coded over again, would you make isEnabled and/or isVisible final methods? If *I* would do it, I'd probably write it for Scala and lean more heavily on functions rather than mutable state. I'd also steal a few ideas from other frameworks like SiteBricks and JaxRS, have better 'native' JavaScript support (but definitively not going the GWT route), make choosing between stateful and stateless more explicit and up-front, support client-side state (but not like JSF or ASP.NET do it), etc. I gathered plenty of ideas of how I'd do a new framework, but I'm not even going to attempt that anyway :-) After having done a few projects with alternatives (GWT, pure JS/ JaxRS), I'm totally sold on Wicket again. It's not a perfect framework, but sure beats the hell out of most other frameworks when it comes to productivity and maintainability. :-) Eelco
Re: overriding isVisible bad?
I'm curious. Which ideas would you steal from SiteBricks and JaxRS? Juergen On Tue, Nov 30, 2010 at 5:51 AM, Eelco Hillenius eelco.hillen...@gmail.com wrote: On Mon, Nov 29, 2010 at 7:45 PM, richard emberson richard.ember...@gmail.com wrote: If wicket was going to be coded over again, would you make isEnabled and/or isVisible final methods? If *I* would do it, I'd probably write it for Scala and lean more heavily on functions rather than mutable state. I'd also steal a few ideas from other frameworks like SiteBricks and JaxRS, have better 'native' JavaScript support (but definitively not going the GWT route), make choosing between stateful and stateless more explicit and up-front, support client-side state (but not like JSF or ASP.NET do it), etc. I gathered plenty of ideas of how I'd do a new framework, but I'm not even going to attempt that anyway :-) After having done a few projects with alternatives (GWT, pure JS/ JaxRS), I'm totally sold on Wicket again. It's not a perfect framework, but sure beats the hell out of most other frameworks when it comes to productivity and maintainability. :-) Eelco