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 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); >>>>>>>>>> } >>>>>>>>>> >
