Hi Jesse, The improvement sounds good ! I won't be able to work on it this week but I'm sure you will create a patch with a test by the time I'm back ;)
Martin Grigorov Wicket Training and Consulting On Sun, Apr 27, 2014 at 10:50 PM, Jesse Long <[email protected]> wrote: > Hi Wicket Devs, > > ComponentResolvers#resolve() first tries ComponentResolvers# > resolveByComponentHierarchy(). > > This tries the immediate container going all the way up the hierarchy > attempting to resolve the component is the container is also a > IComponentResolver. > > I'm worried that the wrong problem is being fixed in the wrong place here. > > Basically, I understand the need for this code as: > TransparentWebMarkupContainer inside TransparentWebMarkupContainer would > not work, because TransparentWebMarkupContainer just calls get(String) on > its parent. If its parent is also transparent, the get(String) is not going > to return the component, instead, resolve() should be called. > > Therefore, we must try the first one, and if that fails, continue up the > tree in case the parent is also a TransparentWebMarkupContainer. > > This smells like the ComponentResolvers class fixing > TransparentWebMarkupContainer bugs. > > Rather, we could just make TransparentWebMarkupContainer first try > get(String) on the parent, if that fails, check if the parent is a > IComponentResolver and if so, call resolve() on it. > > Like: > @Override > public Component resolve(MarkupContainer container, MarkupStream > markupStream, ComponentTag tag) > { > MarkupContainer parent = getParent(); > Component resolvedComponent = parent.get(tag.getId()); > > /* > * If get(String didn't work, parent may be a IComponentResolver, > in which case we must call > * resolve() on it. > */ > if (resolvedComponent == null && parent instanceof > IComponentResolver) > { > resolvedComponent = > ((IComponentResolver)parent).resolve(container, > markupStream, tag); > } > > if (resolvedComponent != null && getPage().wasRendered( > resolvedComponent)) > { > /* > * Means that parent container has an associated homonymous > tag to this grandchildren > * tag in markup. The parent container wants render it and it > should be not resolved to > * their grandchildren. > */ > return null; > } > return resolvedComponent; > } > > > When using a TransparentWebMarkupContainer, it is reasonable to expect the > component to be resolved from the component bound to the grandparent of the > component tag in the markup hierarchy, but with the current plan, the > component could be resolved from the great great great grandparent. > > Current behavior probably also leads to some confusing error messages when > a component cannot be resolved, where a component deep in the hierarchy > could "steal" a component which has not yet been rendered from an ancestor, > and the error message would be about how the ancestor is missing a > component or it was already rendered etc. User would have a hard time > figuring out that the problem is actually deeper inside the hierarchy. > > Thoughts? > > Thanks, > Jesse >
