Wicket
  1. Wicket
  2. WICKET-2111

Ability to generate markup ids in alternate fashion

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 1.3.5, 1.3.6, 1.4-RC3
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      In the attempts to setup integrated testing one particular piece of wicket code isn't quite extendible enough for our needs, which is the generation of markup ids by the wicket session class. The ability to extend this functionality is not limited to the particular use case, I'd like to propose a small change.

      The issue is the following; when a Component has no explicit markup-id set, the markup id is generated by the Session which has an internal counter and uses an increment of this to generate a mark-id. The flaw IMHO is that a Component requests the Session to generate an id, without passing it any context. Especially the most logical context, i.e. "please session, generate a markup id for ME" is missing. Therefore I'd propose that the Session.getMarkupId() is passes the Component object for which the markup id is to be generated.

      By default, the operation should remain as is and the Session object falls back to the default getMarkupId() without parameters, which is already overrideable. But now you can override the getMarkupId() and generate more useful markup ids.

      In our case, we are able to generate markup ids which contain part of the hierarchy and in this manner generate stable Ids, namely those which do not change after several requests. This particular usage may just work for our case (one page application, no back-button support, etc), but the fundamental overrideable method to generate more useful IDs is more widely applicable, hence this change request.

      1. patch
        2 kB
        Berry van Halderen
      2. wicket-2111.patch
        37 kB
        Juergen Donnerstag
      3. wicket-2111.patch
        25 kB
        Juergen Donnerstag
      4. wicket-2111.patch
        26 kB
        Juergen Donnerstag
      5. wicket-2111.patch
        26 kB
        Juergen Donnerstag

        Issue Links

          Activity

          Hide
          Berry van Halderen added a comment -

          This is the change I propose as a patch to the released 1.3.5 distribution. This patch has been created in such a way that it is a drop in add-on functionality and allows all previous extensions and overrides to work correctly.

          If you look at the patch, its a little bit more than just the new method, that's because the patch is fashioned in such a way that the change is 100% compatible with previous workings of wicket in casu markup ids. Also, the getMarkupId used to return just an integer, while a string as return value is a more useful return value in a more context aware environment. To be sure the behavior is completely non-intrusive and compatible there is a little bit of glue coded needed.

          I can elaborate further if needed

          Show
          Berry van Halderen added a comment - This is the change I propose as a patch to the released 1.3.5 distribution. This patch has been created in such a way that it is a drop in add-on functionality and allows all previous extensions and overrides to work correctly. If you look at the patch, its a little bit more than just the new method, that's because the patch is fashioned in such a way that the change is 100% compatible with previous workings of wicket in casu markup ids. Also, the getMarkupId used to return just an integer, while a string as return value is a more useful return value in a more context aware environment. To be sure the behavior is completely non-intrusive and compatible there is a little bit of glue coded needed. I can elaborate further if needed
          Hide
          Igor Vaynberg added a comment -

          i am thinking perhaps we should have a IMarkupIdStrategy

          { String getMarkupId(Component); }

          instead of baking this into the session and forcing subclassing on that level.

          Show
          Igor Vaynberg added a comment - i am thinking perhaps we should have a IMarkupIdStrategy { String getMarkupId(Component); } instead of baking this into the session and forcing subclassing on that level.
          Hide
          Juergen Donnerstag added a comment -

          I would go even one step further and introduce a IMarkupId type. Our current implementation not only lacks the flexibility mentioned above, the source code - for historic reasons - is rather awkward as well:

          • two places where markupId are stored: Component.generatedMarkupId for integer and a MetaData entry for string markupIds
          • the source code is scattered all over with "if (markupId instanceof Integer)" and "if (markupId instanceof String)"
          • the API is using Object for the same reason
          • set/getMarkupId and set/getMarkupIdImpl are rather confusing

          The attached patch - which changes the API and hence can only be applied to 1.5 - is an attempt to improve all that.

          • removed generatedMarkupId and the MetaDataKey from Component
          • added private IMarkupId markupId
          • cleaned up set/getMarkupId and getMarkupIdImpl. Removed setMarkupIdImpl
          • created an IMarkupId interface
          • created a base class AbstractMarkupId
          • created a DefaultMarkupId which uses an int. Can be subclassed to provide your own means to get to the int id.
          • added Application.newMarkupId() to allow application wide to use your own IMarkupId implementation. Component.getMarkupId can be replaced by a subclass to provide a component specific IMarkupId implementation
          • StringMarkupId is an string based markupId

          I also updated ComponentSourceEntry and this is where I'm not 100% sure. Each IMarkupId must implement String getShortValue which is what is needed to automatically reconstruct the markupId. But different to our current implementation when the Component is reconstructed via ComponentSourceEntry.applyComponentInfo a StringMarkupId will be created irrespective of what type markupId was before. Since the markupId will not be modified it should just be fine.

          Some test cases fail but that is only because sequence.nextValue() is invoked in a slightly different order. All other tests are successful.

          Show
          Juergen Donnerstag added a comment - I would go even one step further and introduce a IMarkupId type. Our current implementation not only lacks the flexibility mentioned above, the source code - for historic reasons - is rather awkward as well: two places where markupId are stored: Component.generatedMarkupId for integer and a MetaData entry for string markupIds the source code is scattered all over with "if (markupId instanceof Integer)" and "if (markupId instanceof String)" the API is using Object for the same reason set/getMarkupId and set/getMarkupIdImpl are rather confusing The attached patch - which changes the API and hence can only be applied to 1.5 - is an attempt to improve all that. removed generatedMarkupId and the MetaDataKey from Component added private IMarkupId markupId cleaned up set/getMarkupId and getMarkupIdImpl. Removed setMarkupIdImpl created an IMarkupId interface created a base class AbstractMarkupId created a DefaultMarkupId which uses an int. Can be subclassed to provide your own means to get to the int id. added Application.newMarkupId() to allow application wide to use your own IMarkupId implementation. Component.getMarkupId can be replaced by a subclass to provide a component specific IMarkupId implementation StringMarkupId is an string based markupId I also updated ComponentSourceEntry and this is where I'm not 100% sure. Each IMarkupId must implement String getShortValue which is what is needed to automatically reconstruct the markupId. But different to our current implementation when the Component is reconstructed via ComponentSourceEntry.applyComponentInfo a StringMarkupId will be created irrespective of what type markupId was before. Since the markupId will not be modified it should just be fine. Some test cases fail but that is only because sequence.nextValue() is invoked in a slightly different order. All other tests are successful.
          Hide
          Johan Compagner added a comment -

          hmm do we really need 3 public none final method get starts with getMarkupXXXXX()?

          so they can override everything AND also have Application:
          public IMarkupId newMarkupId(final Component component)

          and besides that, the method that is removed i want to keep...

          setMarkupId(String)

          Thats what uses get, now they have to wrap and know about StringMarkupId??

          Why do we need to have:

          public final Component setMarkupId(final IMarkupId markupId)

          Because now we can set it but we also have a getter
          why not have something like this:

          public final String getMarkupId()
          {
          if (markupId == null)

          { markupId = createMarkupId(); }

          return markupId.toString();
          }
          public final Component setMarkupId(String)

          { markupId = new StringMarkupId(string); }

          public IMarkupId createMarkupId()

          { markupId = getApplication().newMarkupId(this); }

          Who uses by the way the should create boolean? I dont see any reference to it in our methods.?

          for example the patch has a bug:

          public String getMarkupId(boolean createIfDoesNotExist)

          { return getMarkupIdImpl(createIfDoesNotExist).toString(this); }

          that bombs out big time now with a null pointer if the boolean is false.

          But users who needs the create boolean can overwrite the createMarkupId and dont let it happen?

          Show
          Johan Compagner added a comment - hmm do we really need 3 public none final method get starts with getMarkupXXXXX()? so they can override everything AND also have Application: public IMarkupId newMarkupId(final Component component) and besides that, the method that is removed i want to keep... setMarkupId(String) Thats what uses get, now they have to wrap and know about StringMarkupId?? Why do we need to have: public final Component setMarkupId(final IMarkupId markupId) Because now we can set it but we also have a getter why not have something like this: public final String getMarkupId() { if (markupId == null) { markupId = createMarkupId(); } return markupId.toString(); } public final Component setMarkupId(String) { markupId = new StringMarkupId(string); } public IMarkupId createMarkupId() { markupId = getApplication().newMarkupId(this); } Who uses by the way the should create boolean? I dont see any reference to it in our methods.? for example the patch has a bug: public String getMarkupId(boolean createIfDoesNotExist) { return getMarkupIdImpl(createIfDoesNotExist).toString(this); } that bombs out big time now with a null pointer if the boolean is false. But users who needs the create boolean can overwrite the createMarkupId and dont let it happen?
          Hide
          Juergen Donnerstag added a comment -

          IMO we don't need all the getXXX. Intend was to simplify / cleanup the internals, keeping the API close to how it is today, assuming it is as is for good reasons.

          Show
          Juergen Donnerstag added a comment - IMO we don't need all the getXXX. Intend was to simplify / cleanup the internals, keeping the API close to how it is today, assuming it is as is for good reasons.
          Hide
          Johan Compagner added a comment -

          but with the additions of the IMarkupId it is becoming a mess.
          Then we should really try to clean it up, in a nice getter/setter and a factory (2 could have 2 getters if that boolean is important)

          Is it for the outside world also really important to have access to the instanceof IMarkupId ?

          Show
          Johan Compagner added a comment - but with the additions of the IMarkupId it is becoming a mess. Then we should really try to clean it up, in a nice getter/setter and a factory (2 could have 2 getters if that boolean is important) Is it for the outside world also really important to have access to the instanceof IMarkupId ?
          Hide
          Juergen Donnerstag added a comment -

          IMO the mess starts with both Component.generatedMarkupId and a Component MetaDataKey used for markupIds. Two variables for doing almost the same; and several places in the source code with "if (markupId instanceof Integer)" and "if (markupId instanceof String)".

          The solution to that is introducing a IMarkuId which can either be Integer or String or whatever. At least IMarkupId has a meaining, whereas Object has not.

          I think in general we are not too far.

          Show
          Juergen Donnerstag added a comment - IMO the mess starts with both Component.generatedMarkupId and a Component MetaDataKey used for markupIds. Two variables for doing almost the same; and several places in the source code with "if (markupId instanceof Integer)" and "if (markupId instanceof String)". The solution to that is introducing a IMarkuId which can either be Integer or String or whatever. At least IMarkupId has a meaining, whereas Object has not. I think in general we are not too far.
          Hide
          Johan Compagner added a comment -

          yeah i dont get that either.

          Why store it as an integer anyway?

          Why not do Integer.toString(generatedValue) instead of new Integer(generatedValue)

          So it should be always just a string.

          But i am not against IMarkupId and a factory method in component that defaults to Application

          We only should clean up that we only have 2 or 3 (if we really need the boolean) method to get/set it.

          Show
          Johan Compagner added a comment - yeah i dont get that either. Why store it as an integer anyway? Why not do Integer.toString(generatedValue) instead of new Integer(generatedValue) So it should be always just a string. But i am not against IMarkupId and a factory method in component that defaults to Application We only should clean up that we only have 2 or 3 (if we really need the boolean) method to get/set it.
          Hide
          Matej Knopp added a comment -

          There is good reason why markup id is stored as integer by default! In 99.99% cases the integer is only thing necessary to get component's markup id. There is no reason to store it as string. We did that and the runtime penalty was really noticeable. In case someone wants to override it, we store it as metadata. Thus we don't need String slot for markup id.

          Show
          Matej Knopp added a comment - There is good reason why markup id is stored as integer by default! In 99.99% cases the integer is only thing necessary to get component's markup id. There is no reason to store it as string. We did that and the runtime penalty was really noticeable. In case someone wants to override it, we store it as metadata. Thus we don't need String slot for markup id.
          Hide
          Matej Knopp added a comment - - edited

          Guys, before you throw around ideas like IMarkupId and markup ids as strings please consider that the Component itself has been profiled a lot and there is very good reason why the internals look as they do (even though the code is not pretty). In one project (rather ajax heavy) storing component data as Object and having used only int slot for markup id has reduced the memory consumed by wicket components by 30-40% on certain pages.

          If you have lot of ajax enabled components a string field for markup id is a huge waste of memory.

          Show
          Matej Knopp added a comment - - edited Guys, before you throw around ideas like IMarkupId and markup ids as strings please consider that the Component itself has been profiled a lot and there is very good reason why the internals look as they do (even though the code is not pretty). In one project (rather ajax heavy) storing component data as Object and having used only int slot for markup id has reduced the memory consumed by wicket components by 30-40% on certain pages. If you have lot of ajax enabled components a string field for markup id is a huge waste of memory.
          Hide
          Johan Compagner added a comment -

          Better would be if we just would go through the page instead of the session
          But i guess this cant be done because the markupid is already ask for before the component hierarchy is complete?
          Because on a page would be way better for people because then the id is already pretty stable by default..

          I agree with matej that would should look out for a big memory usage hit.

          This can also be solved by having something like IMarkupId but then the instances of IMarkupId should be "interned" so they should be immutable and pooled
          Problem is that we then also have to take care of deserialization so that the interned value is returned. But this is something IMarkupId implementations can do.

          Show
          Johan Compagner added a comment - Better would be if we just would go through the page instead of the session But i guess this cant be done because the markupid is already ask for before the component hierarchy is complete? Because on a page would be way better for people because then the id is already pretty stable by default.. I agree with matej that would should look out for a big memory usage hit. This can also be solved by having something like IMarkupId but then the instances of IMarkupId should be "interned" so they should be immutable and pooled Problem is that we then also have to take care of deserialization so that the interned value is returned. But this is something IMarkupId implementations can do.
          Hide
          Antony Stubbs added a comment -

          +1 for this issue being addressed - stable generated id's are highly desirable for tools such has selenium-ide which build automated test cases based on finding components from their id.

          Show
          Antony Stubbs added a comment - +1 for this issue being addressed - stable generated id's are highly desirable for tools such has selenium-ide which build automated test cases based on finding components from their id.
          Hide
          Igor Vaynberg added a comment -

          i am curious to see a strategy that will help with generating stabler ids and yet making sure they are unique for each component on the page.

          for testing tools we have the option to output the page-relative path of the component in a wicket:path attribute which can be used to identify components on the page and is more stable then the auto-generated id.

          Show
          Igor Vaynberg added a comment - i am curious to see a strategy that will help with generating stabler ids and yet making sure they are unique for each component on the page. for testing tools we have the option to output the page-relative path of the component in a wicket:path attribute which can be used to identify components on the page and is more stable then the auto-generated id.
          Hide
          Carlo M. Camerino added a comment -

          hi,

          +1 for this. wicket component paths do work but on one hand, selenium is always having a hard time finding the component making it harder to find it in the page. maybe it can be included as a setting to make this work. what i do is i use another tool to generate an xpath expression. if it could be simpler and selenium ide can find it , it will be easier to use. thanks a lot

          Show
          Carlo M. Camerino added a comment - hi, +1 for this. wicket component paths do work but on one hand, selenium is always having a hard time finding the component making it harder to find it in the page. maybe it can be included as a setting to make this work. what i do is i use another tool to generate an xpath expression. if it could be simpler and selenium ide can find it , it will be easier to use. thanks a lot
          Hide
          Igor Vaynberg added a comment -

          hrm, an interesting idea maybe that getmarkupid() returns an opaque object: class MarkupId() { } and the id itself is obtained by calling a .tostring(). this is basically an indirection that would allow us to defer id generation until render time. this way, even if component is not yet added to the page we can regurn an id representation that can be passed on to other components.

          Show
          Igor Vaynberg added a comment - hrm, an interesting idea maybe that getmarkupid() returns an opaque object: class MarkupId() { } and the id itself is obtained by calling a .tostring(). this is basically an indirection that would allow us to defer id generation until render time. this way, even if component is not yet added to the page we can regurn an id representation that can be passed on to other components.
          Hide
          Matej Knopp added a comment -

          Just keep in mind one thing. We need to remember the markup id after it's rendered. For "standard" markup Id we only need one integer (4 bytes, not a reference). Otherwise it costs lot of state for ajax heavy apps.

          Show
          Matej Knopp added a comment - Just keep in mind one thing. We need to remember the markup id after it's rendered. For "standard" markup Id we only need one integer (4 bytes, not a reference). Otherwise it costs lot of state for ajax heavy apps.
          Hide
          Igor Vaynberg added a comment -

          yeah, and that is fine. the MarkupId can fix the componentid when tostring() is called. but this extra level of indirection should allow us to work around current hierarchy limitations.

          Show
          Igor Vaynberg added a comment - yeah, and that is fine. the MarkupId can fix the componentid when tostring() is called. but this extra level of indirection should allow us to work around current hierarchy limitations.
          Hide
          Juergen Donnerstag added a comment -

          A new attempt on the topic:

          • generated markup ID (int) and the meta data are still there and maintained by the Component => no change. We still have min overhead for ajax heavy pages with generated IDs
          • Setter: No more "Object" and "instanceof String" or "instanceof Integer", and no more public setMarkupIdImpl(). Users will only see one method setMarkupId(String).
            // what users will use. A thin wrapper only
            public final Component setMarkupId(final String markupId)

          // Used to set generated ID. No need for users to use directly. Not private, but hidden, since required by ComponentSourceEntry. A thin wrapper
          final Component setMarkupId(final int markupId)

          // copies markupID settings from "comp". Required by MarkupContainer.replace(). hidden. A thin wrapper
          final Component setMarkupId(final Component comp)

          // private; low level work; set generatedMarkupId and metakey
          private final void setMarkupId(final int iId, final String sId)

          • Getter: similar API but at least IMO clearer with respect to what they do and thus simpler to understand
            // public; non-final (can be replaced by user). Find the markup starting with the markup itself, than check the generated Id, than meta data, and optionally a global strategy.
            // In case of the generated ID, the "int" will be converted into a W3C compliant string etc. => getMarkupImpl() is responsible to convert the internal representation into an external one (return value is a String)
            public String getMarkupIdImpl()

          // A little helper: make sure the ID is W3C compliant
          private String makeMarkupIdCompliant(String markupId)

          // Some of the logik around generated ID has been moved into getMarkupIdImpl() as explained above which IMO assigns clear responsibilities to each and makes it easier to read. This also allowed to make getMarkupId() final (all methods are final except getMarkupIdImpl)
          public final String getMarkupId(boolean createIfDoesNotExist)

          // As before. no change
          public final String getMarkupId()

          • Introduces a global strategy with a simple interface which is invoked during getMarkupIdImpl() if no other ID was found. This way you can register a strategy during development and simplify testing with the various tools around. In prod you'd use generated ID enjoying optimized performance
            String getMarkupId(Component component);

          feedback and comments are welcome

          Show
          Juergen Donnerstag added a comment - A new attempt on the topic: generated markup ID (int) and the meta data are still there and maintained by the Component => no change. We still have min overhead for ajax heavy pages with generated IDs Setter: No more "Object" and "instanceof String" or "instanceof Integer", and no more public setMarkupIdImpl(). Users will only see one method setMarkupId(String). // what users will use. A thin wrapper only public final Component setMarkupId(final String markupId) // Used to set generated ID. No need for users to use directly. Not private, but hidden, since required by ComponentSourceEntry. A thin wrapper final Component setMarkupId(final int markupId) // copies markupID settings from "comp". Required by MarkupContainer.replace(). hidden. A thin wrapper final Component setMarkupId(final Component comp) // private; low level work; set generatedMarkupId and metakey private final void setMarkupId(final int iId, final String sId) Getter: similar API but at least IMO clearer with respect to what they do and thus simpler to understand // public; non-final (can be replaced by user). Find the markup starting with the markup itself, than check the generated Id, than meta data, and optionally a global strategy. // In case of the generated ID, the "int" will be converted into a W3C compliant string etc. => getMarkupImpl() is responsible to convert the internal representation into an external one (return value is a String) public String getMarkupIdImpl() // A little helper: make sure the ID is W3C compliant private String makeMarkupIdCompliant(String markupId) // Some of the logik around generated ID has been moved into getMarkupIdImpl() as explained above which IMO assigns clear responsibilities to each and makes it easier to read. This also allowed to make getMarkupId() final (all methods are final except getMarkupIdImpl) public final String getMarkupId(boolean createIfDoesNotExist) // As before. no change public final String getMarkupId() Introduces a global strategy with a simple interface which is invoked during getMarkupIdImpl() if no other ID was found. This way you can register a strategy during development and simplify testing with the various tools around. In prod you'd use generated ID enjoying optimized performance String getMarkupId(Component component); feedback and comments are welcome
          Hide
          Martin Grigorov added a comment -

          I have no much experience in that area in Wicket, but I think the patch covers all the issues mentioned in the comments above.

          Show
          Martin Grigorov added a comment - I have no much experience in that area in Wicket, but I think the patch covers all the issues mentioned in the comments above.
          Hide
          Igor Vaynberg added a comment -

          do we need private transient String markupId; its a big penalty to add a runtime slot to all components when it will mostly go unused.

          ive been thinking about how to fix this mess we have with multiple setters, etc.

          what if:

          we generate the integer id like we do now, automatically. but, on output we encode it into the [a-zA-Z0-9]+ alphabet. then we have a single setter that takes a string, and if it can be parsed back into an integer we set the generated int id to the parsed integer. the internals will be much simpler as well if we do this.

          a nice sideeffect is that ids will become much shorter. instead of "id53" it can be "idZ" and instead of "id1000" it will be "idyQ" or whatever the encoded version will be.

          Show
          Igor Vaynberg added a comment - do we need private transient String markupId; its a big penalty to add a runtime slot to all components when it will mostly go unused. ive been thinking about how to fix this mess we have with multiple setters, etc. what if: we generate the integer id like we do now, automatically. but, on output we encode it into the [a-zA-Z0-9] + alphabet. then we have a single setter that takes a string, and if it can be parsed back into an integer we set the generated int id to the parsed integer. the internals will be much simpler as well if we do this. a nice sideeffect is that ids will become much shorter. instead of "id53" it can be "idZ" and instead of "id1000" it will be "idyQ" or whatever the encoded version will be.
          Hide
          Juergen Donnerstag added a comment -

          I removed the transient String markupId, since users may still add it by subclassing getMarkupIdImpl().

          > we generate the integer id like we do now, automatically. but, on output we encode it into the [a-zA-Z0-9]+ alphabet.
          > a nice sideeffect is that ids will become much shorter. instead of "id53" it can be "idZ" and instead of "id1000" it will be "idyQ" or whatever the encoded version will be.
          we encode the ID already for some time. In development mode we also add the component ID to make it human readable

          > then we have a single setter that takes a string, and if it can be parsed back into an integer we set the generated int id to the parsed integer. the internals will be much simpler as well if we do this.
          The "int" setter is necessary because of ComponentSourceEntry to retrieve a page from storage.
          The "copy" setter is necessary to support addOrReplace()
          All setter are hidden except setMarkupId(String). That's the only one visible to the user.

          Show
          Juergen Donnerstag added a comment - I removed the transient String markupId, since users may still add it by subclassing getMarkupIdImpl(). > we generate the integer id like we do now, automatically. but, on output we encode it into the [a-zA-Z0-9] + alphabet. > a nice sideeffect is that ids will become much shorter. instead of "id53" it can be "idZ" and instead of "id1000" it will be "idyQ" or whatever the encoded version will be. we encode the ID already for some time. In development mode we also add the component ID to make it human readable > then we have a single setter that takes a string, and if it can be parsed back into an integer we set the generated int id to the parsed integer. the internals will be much simpler as well if we do this. The "int" setter is necessary because of ComponentSourceEntry to retrieve a page from storage. The "copy" setter is necessary to support addOrReplace() All setter are hidden except setMarkupId(String). That's the only one visible to the user.
          Hide
          Juergen Donnerstag added a comment -

          updated version

          Show
          Juergen Donnerstag added a comment - updated version
          Hide
          Juergen Donnerstag added a comment -

          updated version

          Show
          Juergen Donnerstag added a comment - updated version
          Hide
          Martin Grigorov added a comment -

          No action here for the last 3 years. And no one asked for something like this in the mailing lists.
          Closing as "Won't fix".

          Show
          Martin Grigorov added a comment - No action here for the last 3 years. And no one asked for something like this in the mailing lists. Closing as "Won't fix".

            People

            • Assignee:
              Unassigned
              Reporter:
              Berry van Halderen
            • Votes:
              4 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development