Uploaded image for project: 'Wicket'
  1. Wicket
  2. WICKET-6347

IChoiceRenderer implements IDetachable

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Trivial
    • Resolution: Fixed
    • Affects Version/s: 8.0.0-M4
    • Fix Version/s: 8.0.0-M6
    • Component/s: wicket
    • Labels:
      None

      Description

      IChoiceRenderer (and other renderers ) don't implement IDetachable currently.

      I've came across situations where this would have been handy, and apparently other users would also welcome such an improvement:

      http://stackoverflow.com/questions/42933128/automatically-detaching-choice-renderer-in-wicket

      I've made the required changes locally and there's no negative impact as far as I can see.

        Issue Links

          Activity

          Hide
          svenmeier Sven Meier added a comment -

          Choice renderers now take part in detachment as other Wicket concepts like components and models. The owning component is responsible to detach it.

          Show
          svenmeier Sven Meier added a comment - Choice renderers now take part in detachment as other Wicket concepts like components and models. The owning component is responsible to detach it.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 44a20c5df85e636a49f62c87be7377b44e5008ee in wicket's branch refs/heads/master from Sven Meier
          [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=44a20c5 ]

          WICKET-6347 renderer might be null

          if #newEditor() doesn't create AbstractChoice, the renderer might actually be null

          Show
          jira-bot ASF subversion and git services added a comment - Commit 44a20c5df85e636a49f62c87be7377b44e5008ee in wicket's branch refs/heads/master from Sven Meier [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=44a20c5 ] WICKET-6347 renderer might be null if #newEditor() doesn't create AbstractChoice, the renderer might actually be null
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 775c40c77226517636ba3905dff8b4cc7dd86e60 in wicket's branch refs/heads/master from Sven Meier
          [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=775c40c ]

          WICKET-6347 added #detach() to all renderers

          Show
          jira-bot ASF subversion and git services added a comment - Commit 775c40c77226517636ba3905dff8b4cc7dd86e60 in wicket's branch refs/heads/master from Sven Meier [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=775c40c ] WICKET-6347 added #detach() to all renderers
          Hide
          svenmeier Sven Meier added a comment -

          I'll wait with this change until we settled what to do about IModel's semantic error.

          Show
          svenmeier Sven Meier added a comment - I'll wait with this change until we settled what to do about IModel's semantic error.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 24da5e48b3410ab46dced342cdd08a15831069bc in wicket's branch refs/heads/WICKET-6347-detachable-renderers from Sven Meier
          [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=24da5e4 ]

          WICKET-6347 renderer might be null

          if #newEditor() doesn't create AbstractChoice, the renderer might actually be null

          Show
          jira-bot ASF subversion and git services added a comment - Commit 24da5e48b3410ab46dced342cdd08a15831069bc in wicket's branch refs/heads/ WICKET-6347 -detachable-renderers from Sven Meier [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=24da5e4 ] WICKET-6347 renderer might be null if #newEditor() doesn't create AbstractChoice, the renderer might actually be null
          Hide
          pedrosans Pedro Santos added a comment -

          Because we have another interface doing it (IModel, and I also pointed the semantic error in Martjin's mail thread earlier today) does not actually counter my point that it's a semantic error. But you are invited to show your technical point instead of to plain point that are not convinced.

          The alternative implementation I proposed for the components that could detach renderers should cover your use case and adds no semantic error to the code.

          Show
          pedrosans Pedro Santos added a comment - Because we have another interface doing it (IModel, and I also pointed the semantic error in Martjin's mail thread earlier today) does not actually counter my point that it's a semantic error. But you are invited to show your technical point instead of to plain point that are not convinced. The alternative implementation I proposed for the components that could detach renderers should cover your use case and adds no semantic error to the code.
          Hide
          svenmeier Sven Meier added a comment -

          I'm trying to improve Wicket by letting renderers implement IDetachable - that's neither sexy nor anything foreign to Wicket.
          As long as IModel extends IDetachable, I see no reason why IChoiceRenderer shouldn't be able to implement this interface too. Your objections didn't convince me of the opposite.

          Show
          svenmeier Sven Meier added a comment - I'm trying to improve Wicket by letting renderers implement IDetachable - that's neither sexy nor anything foreign to Wicket. As long as IModel extends IDetachable, I see no reason why IChoiceRenderer shouldn't be able to implement this interface too. Your objections didn't convince me of the opposite.
          Hide
          pedrosans Pedro Santos added a comment -

          Hi Sven,

          it does look that you are eager to shove this Java 8 new sexy feature regardless if you are adding a default implementation. It isn't because you can reach something by breaking contracts/interfaces with this new feature (a lot of renderers won't be detachable) that it suits Wicket well for renderes (we can discuss models in another thread).

          Show
          pedrosans Pedro Santos added a comment - Hi Sven, it does look that you are eager to shove this Java 8 new sexy feature regardless if you are adding a default implementation. It isn't because you can reach something by breaking contracts/interfaces with this new feature (a lot of renderers won't be detachable) that it suits Wicket well for renderes (we can discuss models in another thread).
          Hide
          pedrosans Pedro Santos added a comment -

          Again (I pointed the semantic error in your point earlier today), there's no temporary value being detached in this implementation. This empty method is rather a placeholder than a default implementation.

          Yes it's forcing all renderers to communicate that they fulfill the contract: "I can be detached", regardless if they really do, which is another semantic error.

          Show
          pedrosans Pedro Santos added a comment - Again (I pointed the semantic error in your point earlier today), there's no temporary value being detached in this implementation. This empty method is rather a placeholder than a default implementation. Yes it's forcing all renderers to communicate that they fulfill the contract: "I can be detached", regardless if they really do, which is another semantic error.
          Hide
          svenmeier Sven Meier added a comment -

          Hi Pedro,

          it seems you're not too keen on the new default methods
          IMHO they suit Wicket rather well - for models or for renderers.

          Sven

          Show
          svenmeier Sven Meier added a comment - Hi Pedro, it seems you're not too keen on the new default methods IMHO they suit Wicket rather well - for models or for renderers. Sven
          Hide
          mgrigorov Martin Grigorov added a comment -

          With the "default" impl of the method I don't think this is "forcing" anything.
          It is just one way to use Java 8 default methods in interfaces.

          Show
          mgrigorov Martin Grigorov added a comment - With the "default" impl of the method I don't think this is "forcing" anything. It is just one way to use Java 8 default methods in interfaces.
          Hide
          pedrosans Pedro Santos added a comment -

          Hi Sven, why don't:

          if(renderer instanceof IDetachable) ((IDetachable)renderer).detach();

          instead of to force this interface to all renderes?

          Show
          pedrosans Pedro Santos added a comment - Hi Sven, why don't: if(renderer instanceof IDetachable) ((IDetachable)renderer).detach(); instead of to force this interface to all renderes?
          Hide
          svenmeier Sven Meier added a comment -

          Not much to it with Java 8 default methods.

          Show
          svenmeier Sven Meier added a comment - Not much to it with Java 8 default methods.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 0bc929d71865f052e895fd0918ea297acc7086fa in wicket's branch refs/heads/WICKET-6347-detachable-renderers from Sven Meier
          [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=0bc929d ]

          WICKET-6347 added #detach() to all renderers

          Show
          jira-bot ASF subversion and git services added a comment - Commit 0bc929d71865f052e895fd0918ea297acc7086fa in wicket's branch refs/heads/ WICKET-6347 -detachable-renderers from Sven Meier [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=0bc929d ] WICKET-6347 added #detach() to all renderers
          Hide
          mgrigorov Martin Grigorov added a comment -

          I don't imagine any issues, but I can tell more once I see the diff.

          Show
          mgrigorov Martin Grigorov added a comment - I don't imagine any issues, but I can tell more once I see the diff.
          Hide
          svenmeier Sven Meier added a comment -

          Any objections? I see no drawbacks.

          Show
          svenmeier Sven Meier added a comment - Any objections? I see no drawbacks.

            People

            • Assignee:
              svenmeier Sven Meier
              Reporter:
              svenmeier Sven Meier
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development