You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by Jezza <gi...@git.apache.org> on 2018/10/16 19:30:57 UTC
[GitHub] wicket pull request #297: Improve AjaxLazyLoadPanel's functionality.
GitHub user Jezza opened a pull request:
https://github.com/apache/wicket/pull/297
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.)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Jezza/wicket async-lazy-load
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/wicket/pull/297.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #297
----
commit a66abe8cf104e577377eb0300e3a25451d50ce13
Author: Jezza <je...@...>
Date: 2018-10-16T19:22:25Z
Extend AjaxLazyLoadPanel's functionality.
Allow lazy panels to be terminated.
They can now be provided with an indicator id.
#getUpdateInterval() will also work for the first request, rather than just the subsequent requests.
----
---
[GitHub] wicket issue #297: Improve AjaxLazyLoadPanel's functionality.
Posted by Jezza <gi...@git.apache.org>.
Github user Jezza commented on the issue:
https://github.com/apache/wicket/pull/297
I actually just that this 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.
---
[GitHub] wicket issue #297: Improve AjaxLazyLoadPanel's functionality.
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on the issue:
https://github.com/apache/wicket/pull/297
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.
---
[GitHub] wicket issue #297: Improve AjaxLazyLoadPanel's functionality.
Posted by Jezza <gi...@git.apache.org>.
Github user Jezza commented on the issue:
https://github.com/apache/wicket/pull/297
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.
---
[GitHub] wicket issue #297: Improve AjaxLazyLoadPanel's functionality.
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on the issue:
https://github.com/apache/wicket/pull/297
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.
---
[GitHub] wicket issue #297: Improve AjaxLazyLoadPanel's functionality.
Posted by Jezza <gi...@git.apache.org>.
Github user Jezza commented on the issue:
https://github.com/apache/wicket/pull/297
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 to me seems like extra work that doesn't need to be managed.
If I have one place that checks all of the loading (`contentState`), I don't want to have to duplicate the state checking code.
I could pull it out into a method, but isn't that entirely what `contentState` is for?
To check the state of the content?
It's to separate the concerns.
You wouldn't be in favour of the other extreme, and want to get rid of `contentState` entire, and just return null from `getLazyLoadComponent` isn't ready.
I say that not every one will end up using this functionality, but I dare say a lot of the people who are doing parallel stuff with this panel might find it easier to deal with things that can have a third state.
---
[GitHub] wicket issue #297: Improve AjaxLazyLoadPanel's functionality.
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on the issue:
https://github.com/apache/wicket/pull/297
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.
---