Wicket
  1. Wicket
  2. WICKET-5429

ResourceReference's properties are not preserved when using reference replacement

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 6.12.0
    • Fix Version/s: 6.13.0, 7.0.0-M1
    • Component/s: wicket
    • Labels:
      None

      Description

      Media property of link (css) is not set if addResourceReplacement is set on WebApplication.init :

      I set a css file on page via renderHead(IHeaderResponse response) with screen media :

      @Override
      public void renderHead(IHeaderResponse response)

      { response.render(CssHeaderItem.forReference(OriginalResourceReference.get(), "screen")); }

      and after if I defined a resourceReplacement on application init :

      @Override
      public void init()

      { super.init(); addResourceReplacement(OriginalResourceReference.get(), new CssResourceReference(OriginalResourceReference.class, "overwrite.css")); }

      I don't have the property media="screen" when wicket generated page.

      Duto

      1. Sample.zip
        24 kB
        Duto
      2. fix-WICKET-5429.patch
        1 kB
        Cedric Gatay
      3. WICKET-5429-RRBR.patch
        9 kB
        Martin Grigorov

        Activity

        Hide
        Cedric Gatay added a comment -

        Patch for the described use case. I did not find a better way than double checking if we are manipulating CssReferenceHeaderItems to copy the media / pageParameters / condition to the replacement bundle.

        The patch was made against wicket-6.x branch.

        Show
        Cedric Gatay added a comment - Patch for the described use case. I did not find a better way than double checking if we are manipulating CssReferenceHeaderItems to copy the media / pageParameters / condition to the replacement bundle. The patch was made against wicket-6.x branch.
        Hide
        Martin Grigorov added a comment -

        Attaching an extended version of Cedric's patch.

        There were few problems with his version:

        1) it was fixing only the case with CSS resources, but not for JS ones

        2) his code was used for normal resource bundles too. I.e. the bundle will be recreated with the details of each of its resources and it will use the ones from the last resource reference.
        I've added a special ReplacementResourceBundleReference to be able to decide when to preserve the details and when not.

        Show
        Martin Grigorov added a comment - Attaching an extended version of Cedric's patch. There were few problems with his version: 1) it was fixing only the case with CSS resources, but not for JS ones 2) his code was used for normal resource bundles too. I.e. the bundle will be recreated with the details of each of its resources and it will use the ones from the last resource reference. I've added a special ReplacementResourceBundleReference to be able to decide when to preserve the details and when not.
        Hide
        Martin Grigorov added a comment -

        Emond Papegaaij Do you have a better solution for this problem ?

        Show
        Martin Grigorov added a comment - Emond Papegaaij Do you have a better solution for this problem ?
        Hide
        Emond Papegaaij added a comment -

        addResourceReplacement is a shortcut for registering a bundle with only 1 item. The 3 lines of code can also be executed manually, with a minor change to the last line (creating the item for the css reference). I don't think we need to extend the api for a usecase that's rarely used and can be solved with 3 lines of code already.

        Show
        Emond Papegaaij added a comment - addResourceReplacement is a shortcut for registering a bundle with only 1 item. The 3 lines of code can also be executed manually, with a minor change to the last line (creating the item for the css reference). I don't think we need to extend the api for a usecase that's rarely used and can be solved with 3 lines of code already.
        Hide
        Martin Grigorov added a comment -

        The problem is:

        1) it is not automatic.
        the developer will have to do this manually by copy/paste-ing code from Wicket

        2) the developer will duplicate the details once where the original header item is created and second time where the replacement header item is declared. It will be very easy to forget to update any of them

        I'm also not happy with the new ReplacementResourceBundleReference but I see no other way how to differentiate whether the current case is a replacement or a "normal" bundle.
        Another way is to add #isReplacement to ResourceBundleReference - not better :-/

        Show
        Martin Grigorov added a comment - The problem is: 1) it is not automatic. the developer will have to do this manually by copy/paste-ing code from Wicket 2) the developer will duplicate the details once where the original header item is created and second time where the replacement header item is declared. It will be very easy to forget to update any of them I'm also not happy with the new ReplacementResourceBundleReference but I see no other way how to differentiate whether the current case is a replacement or a "normal" bundle. Another way is to add #isReplacement to ResourceBundleReference - not better :-/
        Hide
        Cedric Gatay added a comment -

        That's the reason why I made the change only for the use case described in this Jira issue. It however lacks the ability to tell if the resource really needs to be recreated as Martin stated in his first answer.

        Show
        Cedric Gatay added a comment - That's the reason why I made the change only for the use case described in this Jira issue. It however lacks the ability to tell if the resource really needs to be recreated as Martin stated in his first answer.
        Hide
        Emond Papegaaij added a comment -

        Ok, I see the problem now. I think this should be solved with a dedicated HeaderItemWrapper. Bundles are rewrapped prior being rendered. This should allow copying the properties without explicit class checks. I'll see if I can create a patch on my way home (in an hour).

        Show
        Emond Papegaaij added a comment - Ok, I see the problem now. I think this should be solved with a dedicated HeaderItemWrapper. Bundles are rewrapped prior being rendered. This should allow copying the properties without explicit class checks. I'll see if I can create a patch on my way home (in an hour).
        Hide
        Emond Papegaaij added a comment -

        I've looked at it some more, and bundles are rewrapped, but that's not going to help, because the wrap method is invoked on the original item, not on the bundle. We could do another interface check, but I don't think the result will be any better than your patch. So, you can apply your patch.

        Show
        Emond Papegaaij added a comment - I've looked at it some more, and bundles are rewrapped, but that's not going to help, because the wrap method is invoked on the original item, not on the bundle. We could do another interface check, but I don't think the result will be any better than your patch. So, you can apply your patch.

          People

          • Assignee:
            Martin Grigorov
            Reporter:
            Duto
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development