Wicket
  1. Wicket
  2. WICKET-4754

make bookmarkablepagelink more flexible/reusable

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 1.5.8, 6.0.0
    • Fix Version/s: None
    • Component/s: wicket
    • Labels:
      None

      Description

      bookmarkablepagelink currently only takes page class from constructor which makes it less flexible/reusable in some situations ,it would be good to make it flexible and more reusable.

      i am providing the patch for the same .

      thanks !

        Activity

        Hide
        Martin Grigorov added a comment -

        Closing as "Won't fix" - I think the idea is not backed up by a good use case.
        You can roll your own link component, BPL is quite simple.

        Show
        Martin Grigorov added a comment - Closing as "Won't fix" - I think the idea is not backed up by a good use case. You can roll your own link component, BPL is quite simple.
        Hide
        vineet semwal added a comment -

        user should be responsible for his model implementation which is usually the case in wicket .the most i would do if i go by this approach is provide javadoc which says if your model is not detachable pageclass will get serialized and this might lead to problem etc.

        Show
        vineet semwal added a comment - user should be responsible for his model implementation which is usually the case in wicket .the most i would do if i go by this approach is provide javadoc which says if your model is not detachable pageclass will get serialized and this might lead to problem etc.
        Hide
        Martin Grigorov added a comment -

        BPL#detach() will call IModel<Class<Page>>'s #detach() but this will work only if the user uses your LDM. But since it is possible to use any IModel<Class<Page>> there is no way Wicket can assure that there wont be a hard ref to a Class.

        Show
        Martin Grigorov added a comment - BPL#detach() will call IModel<Class<Page>>'s #detach() but this will work only if the user uses your LDM. But since it is possible to use any IModel<Class<Page>> there is no way Wicket can assure that there wont be a hard ref to a Class.
        Hide
        vineet semwal added a comment -

        hmm i did that patch in a hurry, pageclassmodel#detach() can be called in bookmarkablepagelink.detach() ,if you see my loadabledetachablemodel implementation ,it doesn't keep any reference to pageclass ,LDM itself doesn't serialize anything.

        my usecase is simple i want the bookmarkablepagelink to be dynamic that is when class changes ,the url should change.currently this is not possible.removing final from getpageclass() will solve this but you will still have to resolve the pageclass from the the classname probably you will do that in some other method or may be will provide someother method which user can override.i consider passing a pageclassmodel to be a more elegant solution.

        Show
        vineet semwal added a comment - hmm i did that patch in a hurry, pageclassmodel#detach() can be called in bookmarkablepagelink.detach() ,if you see my loadabledetachablemodel implementation ,it doesn't keep any reference to pageclass ,LDM itself doesn't serialize anything. my usecase is simple i want the bookmarkablepagelink to be dynamic that is when class changes ,the url should change.currently this is not possible.removing final from getpageclass() will solve this but you will still have to resolve the pageclass from the the classname probably you will do that in some other method or may be will provide someother method which user can override.i consider passing a pageclassmodel to be a more elegant solution.
        Hide
        Martin Grigorov added a comment -

        What is your use case ?Your description is too generic.
        Currently Wicket keeps a ref to the class name. This is so to not keep a hard reference to a Class which may lead to ClassLoader memory leaks. With your approach this is broken because you keep a reference to a model which keeps the page itself. Your LDM impl can solve this but you don't call its #detach() method.

        All this can be solved by removing 'final' from org.apache.wicket.markup.html.link.BookmarkablePageLink#getPageClass() but as I said your description is too short to understand what flexibilty is missing.

        Show
        Martin Grigorov added a comment - What is your use case ?Your description is too generic. Currently Wicket keeps a ref to the class name. This is so to not keep a hard reference to a Class which may lead to ClassLoader memory leaks. With your approach this is broken because you keep a reference to a model which keeps the page itself. Your LDM impl can solve this but you don't call its #detach() method. All this can be solved by removing 'final' from org.apache.wicket.markup.html.link.BookmarkablePageLink#getPageClass() but as I said your description is too short to understand what flexibilty is missing.
        Hide
        vineet semwal added a comment -

        new patch (BookmarkablePageLinkWithPageClassModel.patch) lets user pass a page class model ,some other code changes done to clean bookmarkablepagelink.

        Show
        vineet semwal added a comment - new patch (BookmarkablePageLinkWithPageClassModel.patch) lets user pass a page class model ,some other code changes done to clean bookmarkablepagelink.
        Hide
        vineet semwal added a comment -

        another patch which does a bit more changes but makes bookmarkablepagelink more flexible .

        Show
        vineet semwal added a comment - another patch which does a bit more changes but makes bookmarkablepagelink more flexible .
        Hide
        vineet semwal added a comment -

        the provided patch is for 6.x ..

        Show
        vineet semwal added a comment - the provided patch is for 6.x ..
        Hide
        vineet semwal added a comment -

        patch added

        Show
        vineet semwal added a comment - patch added

          People

          • Assignee:
            Unassigned
            Reporter:
            vineet semwal
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development