On Fri, Mar 30, 2012 at 4:45 PM, Juergen Donnerstag <juergen.donners...@gmail.com> wrote: > but with setOutputMarkupId() we make sure the most appropriate ID is > used, if one hasn't already been assigned. And in case wicket:id is
You mean if "id" attribute is present. 'wicket:id' should be there to be a Wicket Component. > present, it'll be used. And IDs have to be page-unique. That is no tag > on the same page must have the same ID. > > Juergen > > On Fri, Mar 30, 2012 at 4:33 PM, Sven Meier <s...@meiers.net> wrote: >> Javascript ids are session unique anyway - just prefixed with the component >> id in non-deployment config. >> >> Sven >> >> >> On 03/30/2012 04:11 PM, Juergen Donnerstag wrote: >>> >>> On Fri, Mar 30, 2012 at 3:57 PM, Sven Meier<s...@meiers.net> wrote: >>>> >>>> Hi Juergen and Jesse, >>>> >>>> I've debugged WicketLinkTagHandler and everything worked as I have >>>> expected >>>> it: >>>> >>>> The WicketTag is always the same instance, coming from the cached markup. >>>> Changing its id doesn't make sense to me: This is global state which is >>>> accessed for *each* rendering of the container. >>>> >>>> >>>>> An id which is unique with markup file is not guranteed to be unique in >>>>> the >>>>> page markup. >>>>> That's why resolvers are using the page.autoIndex to create a page >>>>> unique >>>>> id >>>> >>>> Yes, the id has to be unique for components inside their container. That >>>> hasn't changed with the patch. >>>> >>> From a component resolution point of view you are right, but I think >>> you are ignoring Javascript here. wicket-ids may become become ids and >>> they need to be page unique, correct? >>> >>> Juergen >>> >>>> Back to debugging >>>> Sven >>>> >>>> >>>> >>>> On 03/30/2012 12:40 PM, Juergen Donnerstag wrote: >>>>> >>>>> On Fri, Mar 30, 2012 at 3:01 AM, Jesse Long<je...@virtualpostman.co.za> >>>>> wrote: >>>>>> >>>>>> Hi Juergen, Sven, >>>>>> >>>>>> We are not doing away with adding the "auto index" unique number to the >>>>>> resultant component id, we are only doing away with modifying the id in >>>>>> the >>>>>> cached markup tag. >>>>> >>>>> The patched changed the resolver and not the filter. Hence the markup >>>>> loaded into the cache is still the same, but the id presented at >>>>> render time is now a different. >>>>> >>>>>> The uniqueness of the id of the component returned by the resolve() >>>>>> method >>>>>> is still being guaranteed (as far as I understand) because it takes the >>>>>> original id from the tag and append the current page's next auto index >>>>>> number. The component (TransparentWebMarkupContainer) is then created >>>>>> with >>>>>> this modified id. >>>>> >>>>> sorry but this is also not correct. The markup of a page is composed >>>>> of potentially many markup files (fragments, panels, inheritance, >>>>> etc.). An id which is unique with markup file is not guranteed to be >>>>> unique in the page markup. That's why resolvers are using the >>>>> page.autoIndex to create a page unique id >>>>> >>>>>> All I'm asking in this patch is to not modify the original id stored in >>>>>> the >>>>>> markup tag, because its not necessary. (AFAIK). >>>>>> >>>>>> Anyway, the resultant component id is now (after the patch) as unique >>>>>> as >>>>>> it >>>>>> is on the first render without the patch... >>>>> >>>>> sorry, but this is not true. >>>>> >>>>>> This problem is actually very serious. I have a base page which 90% of >>>>>> my >>>>>> pages extend. Without the patch, on every single render, the id stored >>>>>> in >>>>>> the tag has a few bytes appended, causing the id of the resultant >>>>>> component >>>>>> to be a few bytes longer (for every render of every page in any >>>>>> session). >>>>>> I'm not sure if these ids get stored in memory after the render (I dont >>>>>> know >>>>>> wicket internals well enough to tell) or stored in the page store. If >>>>>> they >>>>>> do, its a big memory leak. >>>>>> >>>>>> At the very least its spews out huge amounts of data in the debug >>>>>> logging. >>>>> >>>>> I very well understand your pain point, but lets fix the problem >>>>> properly and not introduce a bug with an enhancement. >>>>> >>>>> Juergen >>>>> >>>>>> Cheers, >>>>>> Jesse >>>>>> >>>>>> >>>>>> >>>>>> On 29/03/2012 22:39, Sven Meier wrote: >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>>> An artifical markup which creates duplicate IDs can be constructed. >>>>>>> >>>>>>> all auto labels' tags have the same id already (its class name) so no >>>>>>> need >>>>>>> to create some artificial markup. >>>>>>> >>>>>>>> To me the applied patch leaves us with a bigger problem than it >>>>>>>> fixes. >>>>>>> >>>>>>> If the id of the tag in the cached markup is altered on each render, >>>>>>> the >>>>>>> id can get quite long pretty fast. As the reporter of the issues has >>>>>>> stated, >>>>>>> this is a 'big' problem ;). >>>>>>> >>>>>>>> we should fix it properly >>>>>>> >>>>>>> Agreed. Obviously I'm missing a key concept here. >>>>>>> I'll try to debug this issue tomorrow to understand the problem you've >>>>>>> identified. >>>>>>> >>>>>>> Sven >>>>>>> >>>>>>> On 03/29/2012 09:44 PM, Juergen Donnerstag wrote: >>>>>>>> >>>>>>>> Hi Sven >>>>>>>> >>>>>>>>> I double checked with how other filters/resolvers are doing it and I >>>>>>>>> found >>>>>>>>> AutoLabelTextResolver to leave the tag id untouched too. >>>>>>>> >>>>>>>> which is a fault as well. Same risk. >>>>>>>> >>>>>>>>>> May be a Component's flag bit can be used for that purpose. >>>>>>>>> >>>>>>>>> Shouldn't that be a flag on the markup tag instead then? >>>>>>>> >>>>>>>> No, because the Pages are made of potentially many markups. Since the >>>>>>>> ID has no info about hierarchy, the number appended is the only thing >>>>>>>> that makes it page unique. Hence it can not be per markup. Since >>>>>>>> every >>>>>>>> tag gets associated with a Component, the Component is the right >>>>>>>> place >>>>>>>> for the flag. >>>>>>>> >>>>>>>>>> I don't know how likely it is and I know we have no tests for it. >>>>>>>>> >>>>>>>>> Yeah, I didn't find one either. I'm not sure I understand where's a >>>>>>>>> possible >>>>>>>>> problem here. It's hard to discuss this issues without an actual >>>>>>>>> case. >>>>>>>> >>>>>>>> An artifical markup which creates duplicate IDs can be constructed. >>>>>>>> I've not tried it but I don't expect it to be too difficult. Knowing >>>>>>>> there is a potential risk which will be very hard to find by a normal >>>>>>>> user, makes me belief we should fix it properly, To me the applied >>>>>>>> patch leaves us with a bigger problem than it fixes. >>>>>>>> >>>>>>>> Juergen >>>>>>>> >>>>>>>>> Sven >>>>>>>>> >>>>>>>>> >>>>>>>>> On 03/29/2012 08:42 PM, Juergen Donnerstag wrote: >>>>>>>>>> >>>>>>>>>> Hi Sven, >>>>>>>>>> >>>>>>>>>> the change introduces a theoretical risk of duplicate ids. I don't >>>>>>>>>> know how likely it is and I know we have no tests for it. But >>>>>>>>>> Filters >>>>>>>>>> are created per markup file and modify the markup upon loading, >>>>>>>>>> prior >>>>>>>>>> to caching. Which also means that the Page is not available. In >>>>>>>>>> case >>>>>>>>>> of inheritance it might not even be the same Page. To be absolutely >>>>>>>>>> sure the ID is page-unique, it gets update in the Resolvers where >>>>>>>>>> the >>>>>>>>>> Page instance is available. >>>>>>>>>> >>>>>>>>>> The root cause is that we don't validate if the ID has been updated >>>>>>>>>> already and avoid subsequent updates. We have no means to do that >>>>>>>>>> check. A number at the end of the ID would not be sufficient. May >>>>>>>>>> be >>>>>>>>>> a >>>>>>>>>> Component's flag bit can be used for that purpose. >>>>>>>>>> >>>>>>>>>> I understand the problem in your application, but users will have >>>>>>>>>> an >>>>>>>>>> extremly hard time to identify the issue if it occurs. I'm not sure >>>>>>>>>> it's worth it. May be the approach outlined above solved your >>>>>>>>>> problem >>>>>>>>>> as well, but in a safe manner. >>>>>>>>>> >>>>>>>>>> Juergen >>>>>>>>>> >>>>>>>>>> On Thu, Mar 29, 2012 at 6:43 PM,<svenme...@apache.org> >>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> Updated Branches: >>>>>>>>>>> refs/heads/wicket-1.5.x c6c45a510 -> d7e0d8845 >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> WICKET-4484 do not change tag id when resolving component >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Project: http://git-wip-us.apache.org/repos/asf/wicket/repo >>>>>>>>>>> Commit: >>>>>>>>>>> http://git-wip-us.apache.org/repos/asf/wicket/commit/d7e0d884 >>>>>>>>>>> Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/d7e0d884 >>>>>>>>>>> Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/d7e0d884 >>>>>>>>>>> >>>>>>>>>>> Branch: refs/heads/wicket-1.5.x >>>>>>>>>>> Commit: d7e0d8845dd509110e48ba965f0e11a3aae6a1f5 >>>>>>>>>>> Parents: c6c45a5 >>>>>>>>>>> Author: Sven Meier<svenme...@apache.org> >>>>>>>>>>> Authored: Thu Mar 29 18:43:07 2012 +0200 >>>>>>>>>>> Committer: Sven Meier<svenme...@apache.org> >>>>>>>>>>> Committed: Thu Mar 29 18:43:07 2012 +0200 >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>>>> .../markup/parser/filter/WicketLinkTagHandler.java | 1 - >>>>>>>>>>> 1 files changed, 0 insertions(+), 1 deletions(-) >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> http://git-wip-us.apache.org/repos/asf/wicket/blob/d7e0d884/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>>>> diff --git >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>>> index 689133e..8779b6f 100644 >>>>>>>>>>> --- >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>>> +++ >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>>> @@ -208,7 +208,6 @@ public class WicketLinkTagHandler extends >>>>>>>>>>> AbstractMarkupFilter implements ICompo >>>>>>>>>>> if (wtag.isLinkTag()&& >>>>>>>>>>> (wtag.getNamespace() >>>>>>>>>>> != >>>>>>>>>>> null)) >>>>>>>>>>> { >>>>>>>>>>> String id = tag.getId() + "-" + >>>>>>>>>>> container.getPage().getAutoIndex(); >>>>>>>>>>> - tag.setId(id); >>>>>>>>>>> >>>>>>>>>>> return new >>>>>>>>>>> TransparentWebMarkupContainer(id); >>>>>>>>>>> } >>>>>>>>>>> >> -- Martin Grigorov jWeekend Training, Consulting, Development http://jWeekend.com
