wicket
wicket copied to clipboard
apache
Improve AjaxLazyLoadPanel's functionality.
There's a couple of changes here, I'll see if I can list them all:
-
ALLPs can now be terminated, which skips the component replacing part. (This is useful when you use an external service, and don't want it to keep the behaviour around just because some task timed out.)
-
They can now be provided with an indicator id. (Yeah, technically this was possibly before, but because the behaviour was attached to the page, the only place you could install the IAjaxIndicatorAware was on the page directly, meaning ALL ajax behaviours started using that indicator. I can think of a couple of issues with my solution, so I'm definitely hoping someone can come up with a better one. )
-
#getUpdateInterval() will also work for the first request, rather than just the subsequent requests. (This was a bit surprising when I found it, but it doesn't consult the panels on the page for the interval time when constructing the timer itself, meaning, regardless what I want the update interval to be, for the first request, it would always be 1 second because that's the value that passed in the constructor.)
I don't see a need for State#TERMINATED: If a component wants to end polling, it can just return contentReady=true and return something appropriate ... e.g. a timed-out label or even an identical loading component that was shown before.
Hm, while I can see the reasoning, it's rarely as simple as that.
In my opinion, it's about not running code that doesn't need to be run.
If there was only a boolean to represent the state, then you're left with checking at each step (getLazyLoadComponent/onContentLoaded) whether or not something properly loaded.
That kind of defeats the purpose to have a single place that's used to check the state of the content (contentState).
I could pull out the contents of contentState into a method, but then now I'm just working around AjaxLazyLoadPanel.
Even more annoying if the check itself isn't cheap.
Granted, I could cache the check, and all of that jazz, but then I've just implemented a chunk of the book keeping that AjazLazyLoadPanel could do already at no cost.
Not every one will end up using this functionality, but I dare say a lot of the people who are doing proper parallel stuff with this panel might find it easier to deal with things that can have a third state.
I actually just noticed that this improvement fixes a bug in the original.
You can't nest LLPs. If you have a panel, it loads and builds the content, that's fine. But if it contains another LLP then the visit will attempt to load that one as well. Which could be fine, but the problem is is when the LLP tries to replace the panel. Because the nested panel's onBeforeRender hasn't fired yet, the loading component was never added.
This solves that by just inserting the loading component immediately.
IMHO most people will want to show different contents depending on the result of lazy loading - I doubt that anyone would want to give up polling but keep the loading anymation - so they will need to transfer the result from isContentReady() to getLazyLoadComponent() anyways. I don't like to use an enum for that, as this will lead to even more enum values. It's just simpler to keep this information as a member variable. We could have joined isContentReady() and getLazyLoadComponent() into a single method, but I tried to keep the API somewhat similar to 7.x.
Regarding the initial update interval: yes, I agree that this could be improved. Note however that you are calling getUpdateInterval() too early in bind() - at that moment not all AjaxLazyLoadPanel might exist yet. Furthermore I made sure that at least one request is fired, to keep the class backwards-compatible to 7.x with isContentReady() returning true by default.
I mean, I can think of valid use case, but I'm not going to force it, because I definitely think we have a very specific edge case... We have a generic lazy load supplier panel. It accepts a supplier, and a factory that accepts a component id, and the supplier's result. The reason the terminated is handy is because there's now a way to tell the panel itself that it didn't finish normally, but as I said, I'm not gonna force it, because I'm definitely in the minority.
About calling getUpdateInterval too soon, I'm assuming you mean from onInitialise.
That is a bit too soon, so I'm fine with moving it into the onConfigure where the loaded check is.
Thinking back, I can't remember why I did that...
Ah, wait, yeah, the check itself shouldn't be added in onConfigure due to how the request cycle works.
It'll fire it on anything...
Side-note: This is actually how I noticed that the update interval wasn't taken into account on the first request. All of these seemingly fast lazy load panels were taking a second to load. Exactly one second.
I think it's probably a good idea to remove the getUpdateInterval() call and rely on the behaviour itself to update itself. That way the method isn't called too soon.
I'm not sure I like keeping the behaviour of always firing a request. An early escape makes sense, as in our case we have a cache that gets hit, which is why we load it in a LLP, as it's not there the first time. It does a lot for user experience especially when the user is on a poor connection.
As explained above we won't apply this pull request. Please close it and open separate requests for specific issues you see with the current implementation and don't force an API break.
