Hi Jesse, I've applied your patches for WICKET-5569 and WICKET-5580. Please create a new ticket with this improvement + a test.
Martin Grigorov Wicket Training and Consulting On Mon, Apr 28, 2014 at 11:07 PM, Martin Grigorov <[email protected]>wrote: > 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 >> > >
