Be Careful With Apache Wicket’s isVisible

Geek7 Comments

I see this all the time:


class MyPage extends WebPage

{

MyPage()

{

MyComponent myComponent = new MyComponent("myComponent");

add(myComponent);

myComponent.setVisible(isMyConditionMet());

}

}

Looks harmless I suppose but issues develop as the code does. A new link gets added to the page. Then the author finds out that they need to re-calculate the visibility of myComponent, so they call it there too.


public void onClick()

{

myComponent.setVisible(isMyConditionMet());

// Do other link stuff

}

Or maybe worse, there is additional logic based on the click itself!


public void onClick()

{

myComponent.setVisible(clickedTwice || isMyConditionMet());

// Do other link stuff

}

Now, we have the visibility of this component being determined all over the code. That’s not good. It is hard to read and hard to understand.

New idea! What if we just override the isVisible() implementation and calculate it on the fly. Simply overriding isVisible() on my component in Wicket to ensure nice encapsulation of my logic into the component.


@Override

public boolean isVisible()

{

return isMyConditionMet();

}

That is pretty! And easy to figure out what the logic for the visibility on this component does. Problem solved. But another one (or 3) is introduced!

There are 3 problems with doing this in Wicket that I have seen in practice:

1. isVisible isn’t meant to be overridden really. It probably should have been made a final to help us all from using it incorrectly, but it is not. The correct way to have the visibility set on a component is through setVisibilityAllowed(). This method is sort of a partner to the other setVisible method. They both get to decide the overall visibility of the component. However, the corresponding isVisibilityAllowed() method is a final so no one (including sub-classers) can override it to subvert its intent.

2. isVisible() itself gets called a lot per request. Trace it! I have seen 50-100 calls before. If you are doing complex calculations or hitting a database, each time it is called you are slowing your app down. You need to optimize here and find a way to set visibility appropriately and only recalculate when needed. The solution that comes to mind most is the following:


class MyComponent extends Component

{

private Boolean visible = null;

@Override

public boolean isVisible()

{

if (visible == null)

{

visible = isMyConditionMet();

}

return visible;

}

@Override

public void onDetach()

{

super.onDetach();

visible = null;

}

}

Now, you set every page render. Sounds good. It works in many cases — but not all of them. Issue #3!

3. isVisible() gets called before you expect it to. So say you do #2 there. That’s great. But what if the visibility of this component changes during processing. Here is an example of how this happens. Say you are on a page and you click a link. By clicking that link, the visibility of this component should change to hidden. Sounds simple enough. However, when forms are involved (and other cases too I suspect!), the visibility of various components on the page is checked prior to invoking your link’s onClick() code. So with the code from number 2 above, your isVisible() is going to be calculated and cached before your link code even executes. So then your link code set the state so that isMyConditionMet() now returns false. However, isVisible is now caching your value and will not recalculate until the next page refresh. This leaves your user staring at some stale data or state on the page. Oops.

How do we fix all that? Good question. It seems their isn’t a perfect solution. But I found a decent on that works for now. We know we want to sort of cache our visibility state so those pre-click isVisible() calls get the old value. And then we want to recalculate before we render. I know a method that gets called before render!


@Override

public void onBeforeRender()

{

setVisibilityAllowed(isMyConditionMet());

super.onBeforeRender();

}

Here I use the setVisibilityAllowed() as the way to control the visibility. Then we call the super’s onBeforeRender because we want our work to be done first and in case they look at the visibility. But there is something else missing. onBeforeRender doesn’t get called when a component is not visible. So if you start out invisible or switch to that, you can’t ever come back because onBeforeRender never fires. How do we fix that? There is a method that controls this and we can override it. That is a hint that this solution is a decent way to control the framework in my opinion. Somebody has been here before!


@Override

protected boolean callOnBeforeRenderIfNotVisible()

{

return true;

}

Once you add that to your component, onBeforeRender will get called every rendering opportunity. The only downside to this (and the reason why I said there is no perfect solution) is that your component may have children and onBeforeRender will call down into each of them even if the parent component is invisible. Given the Wicket design as of 1.3.5, there is no way to subvert that. In fact, Wicket throws an exception if you don’t called super.onBeforeRender() because it thinks you are doing something wrong. Maybe someday there will be a way to prevent that but for now, it is the side-effect of a proper visibility calculation.

I don’t think you have to go off and retool every isVisible() implementation right away. But you should squint at those isVisible overrides and wonder if they are doing the right thing.