Wicket
  1. Wicket
  2. WICKET-5265

FencedFeedbackPanel is broken with RefreshingView(and it's implementations)

    Details

      Description

      FencedFeedbackPanel doesn't work correctly if inner form(s) are in RefreshingView(or it's implementations)..
      in this case outerform feedbackpanel just starts including messages meant for inner feedbackpanel.
      with ListView FencedFeedbackPanel works correctly..
      actually one user(Mike Dundee) created this issue in quickview https://github.com/vineetsemwal/quickview/issues/19
      so in that link he has described his problem and pasted the code you can use to reproduce ...
      there i have also explained why it's broken with RefreshingView and it's implementations currently(it's a little complex so i am trying to avoid explaining all again ,also english is not my first language )

      thank you !

      1. WICKET-5265.patch
        5 kB
        vineet semwal
      2. second.patch
        5 kB
        vineet semwal
      3. fencedfeedbacktest.tar.gz
        21 kB
        Carl-Eric Menzel
      4. 0001-WICKET-5265-recreate-fence-mark-when-needed.patch
        16 kB
        Carl-Eric Menzel

        Issue Links

          Activity

          vineet semwal created issue -
          Hide
          vineet semwal added a comment - - edited

          in short it's broken when child (inner FencedFeedbackPanel) is removed and then added back later and this is the reason it's broken with RefreshingView ..

          Show
          vineet semwal added a comment - - edited in short it's broken when child (inner FencedFeedbackPanel) is removed and then added back later and this is the reason it's broken with RefreshingView ..
          vineet semwal made changes -
          Field Original Value New Value
          Attachment WICKET-5265.patch [ 12591141 ]
          Hide
          vineet semwal added a comment - - edited

          i have provided the patch that fixes the problem ,all current tests pass and i added one more case for current problem,
          i have also posted code in gist in case if someone just wants to review FencedFeedbackPanel
          https://gist.github.com/vineetsemwal/5944773

          Show
          vineet semwal added a comment - - edited i have provided the patch that fixes the problem ,all current tests pass and i added one more case for current problem, i have also posted code in gist in case if someone just wants to review FencedFeedbackPanel https://gist.github.com/vineetsemwal/5944773
          Hide
          Sven Meier added a comment -

          With your patch it is no longer possible to use multiple feedbacks for the same fence, right?

          Show
          Sven Meier added a comment - With your patch it is no longer possible to use multiple feedbacks for the same fence, right?
          Hide
          vineet semwal added a comment - - edited

          if those multiple feedbacks for same fence are on same level then it will work ,say 2 inner feedbacks on same fence ,then some outer feedback will find one or the other from metadata when it will try to recurse into component.. is this the case you are asking?

          Show
          vineet semwal added a comment - - edited if those multiple feedbacks for same fence are on same level then it will work ,say 2 inner feedbacks on same fence ,then some outer feedback will find one or the other from metadata when it will try to recurse into component.. is this the case you are asking?
          vineet semwal made changes -
          Attachment second.patch [ 12591399 ]
          Hide
          vineet semwal added a comment -

          i have tried another approach and uploaded (second.patch ) .this keeps set of fencedfeedbacks for same fence as metadata so the set is iterated and if any one is found then false is returned ie. (it tells do not recurse into component) else true.

          Show
          vineet semwal added a comment - i have tried another approach and uploaded (second.patch ) .this keeps set of fencedfeedbacks for same fence as metadata so the set is iterated and if any one is found then false is returned ie. (it tells do not recurse into component) else true.
          Hide
          Sven Meier added a comment -

          The problem I see with both of your patches is that you're introducing a possible memory leak: If FencedFeedbackPanels are removed, a component might still keep a reference to it in its metaData.
          If we follow that path, we might equally well keep the current solution with a counter and just remove #onRemove(), so users will have to do the housekeeping by themselves.

          Show
          Sven Meier added a comment - The problem I see with both of your patches is that you're introducing a possible memory leak: If FencedFeedbackPanels are removed, a component might still keep a reference to it in its metaData. If we follow that path, we might equally well keep the current solution with a counter and just remove #onRemove(), so users will have to do the housekeeping by themselves.
          Hide
          vineet semwal added a comment -

          i was thinking of using weakreference/weakhashmap when i created those patches,are you ok if those are used now?
          if you are ok with that,i will create one more patch (will improve second patch) ..
          i have no problem with your suggested solution too in fact that was the first thing i did when i noticed this bug...

          Show
          vineet semwal added a comment - i was thinking of using weakreference/weakhashmap when i created those patches,are you ok if those are used now? if you are ok with that,i will create one more patch (will improve second patch) .. i have no problem with your suggested solution too in fact that was the first thing i did when i noticed this bug...
          Hide
          Carl-Eric Menzel added a comment -

          We encountered the same problem, except rather than with RefreshingView, we saw it with Wizard components. The underlying issue is the same: In onRemove the FencedFeedbackPanel removes the fence mark from its fence component, but if the FFP is added back later, the fence mark is not restored.

          I'll attach a much simpler patch that simply verifies whether the fence mark is still there and recreates it if necessary. All tests run, including one checking for the bad behavior.

          Sven Meier, if you have no objections to this patch, I'll commit it for 6.x and 7.

          Show
          Carl-Eric Menzel added a comment - We encountered the same problem, except rather than with RefreshingView, we saw it with Wizard components. The underlying issue is the same: In onRemove the FencedFeedbackPanel removes the fence mark from its fence component, but if the FFP is added back later, the fence mark is not restored. I'll attach a much simpler patch that simply verifies whether the fence mark is still there and recreates it if necessary. All tests run, including one checking for the bad behavior. Sven Meier , if you have no objections to this patch, I'll commit it for 6.x and 7.
          Hide
          Carl-Eric Menzel added a comment -

          A simpler fix.

          Show
          Carl-Eric Menzel added a comment - A simpler fix.
          Carl-Eric Menzel made changes -
          Carl-Eric Menzel made changes -
          Assignee Carl-Eric Menzel [ cmenzel ]
          Hide
          Martin Grigorov added a comment -

          Looks good to me!

          Show
          Martin Grigorov added a comment - Looks good to me!
          Hide
          Carl-Eric Menzel added a comment -

          It fixes the easy case, but in our application I'm still seeing an issue when using FencedFeedbackPanels together with Wizards. It seems that the onbeforerender/onconfigure thing of the FencedFeedbackPanel inside the WizardStep comes too late in restoring its fence. By that time the outer one in the Wizard itself has already grabbed all the messages.

          I'm investigating this. Unfortunately we do not have an onAdd like we have an onRemove. I'm not certain yet, but I have a feeling that I'll need an onAdd to fix this (and onAdd might be useful in general). I'll update here once I have found out more.

          Show
          Carl-Eric Menzel added a comment - It fixes the easy case, but in our application I'm still seeing an issue when using FencedFeedbackPanels together with Wizards. It seems that the onbeforerender/onconfigure thing of the FencedFeedbackPanel inside the WizardStep comes too late in restoring its fence. By that time the outer one in the Wizard itself has already grabbed all the messages. I'm investigating this. Unfortunately we do not have an onAdd like we have an onRemove. I'm not certain yet, but I have a feeling that I'll need an onAdd to fix this (and onAdd might be useful in general). I'll update here once I have found out more.
          Hide
          Martin Grigorov added a comment -

          Can you use onInitialize() ?
          The idea is almost the same.
          It should be called really soon if the container/parent has a ref to the page.

          Show
          Martin Grigorov added a comment - Can you use onInitialize() ? The idea is almost the same. It should be called really soon if the container/parent has a ref to the page.
          Hide
          Carl-Eric Menzel added a comment - - edited

          Not really. onInitialize is a very different thing. It's only called exactly once and is basically a delayed constructor. I need something that will be called whenever a component is added or re-added to the page component tree. onRemove is called on the entire tree that is removed, we have no complement for that when something is added back.

          I've tried it, and it fixed the problem in this ticket. I'll clean it up a little and push it in a branch so you can have a look at it.

          Show
          Carl-Eric Menzel added a comment - - edited Not really. onInitialize is a very different thing. It's only called exactly once and is basically a delayed constructor. I need something that will be called whenever a component is added or re-added to the page component tree. onRemove is called on the entire tree that is removed, we have no complement for that when something is added back. I've tried it, and it fixed the problem in this ticket. I'll clean it up a little and push it in a branch so you can have a look at it.
          Hide
          Carl-Eric Menzel added a comment -

          The problem in the Wizard is when you go one step forward by calling next() and then come back with previous(). next() causes onRemove() in the WizardStep and all its children, so the FencedFeedbackPanel clears the fence marker. When you come back to that WizardStep with previous() it is re-added to the page but obviously not re-initialized. The component has no way of knowing that this happened, so it can't reliably recreate the fence.

          I have a quickstart for this that I'll upload, to illustrate.

          Show
          Carl-Eric Menzel added a comment - The problem in the Wizard is when you go one step forward by calling next() and then come back with previous(). next() causes onRemove() in the WizardStep and all its children, so the FencedFeedbackPanel clears the fence marker. When you come back to that WizardStep with previous() it is re-added to the page but obviously not re-initialized. The component has no way of knowing that this happened, so it can't reliably recreate the fence. I have a quickstart for this that I'll upload, to illustrate.
          Hide
          Martin Grigorov added a comment -

          I remember there was onAdd() in 1.2.x but it has been removed for some reason. Ask in dev@ and hopefully someone from the other devs will give more details.

          I think having onInitialize() and onAdd() will lead to more confusion.
          You can think of onInitialize() as a delayed constructor but I see it as a callback method which is called when the component has access to bot its parent and its markup. The extra bonus is that you can call overridable methods there, like AjaxLink's newAjaxEventBehavior(String event) and be sure that the AjaxLink component is fully initialized.

          By having onAdd() additionally the application developer will be confused because it won't be clear which callback is called first and which second and what semantics they provide.

          Better ask in dev@. Very few devs are subscribed to JIRA notifications.

          Show
          Martin Grigorov added a comment - I remember there was onAdd() in 1.2.x but it has been removed for some reason. Ask in dev@ and hopefully someone from the other devs will give more details. I think having onInitialize() and onAdd() will lead to more confusion. You can think of onInitialize() as a delayed constructor but I see it as a callback method which is called when the component has access to bot its parent and its markup. The extra bonus is that you can call overridable methods there, like AjaxLink's newAjaxEventBehavior(String event) and be sure that the AjaxLink component is fully initialized. By having onAdd() additionally the application developer will be confused because it won't be clear which callback is called first and which second and what semantics they provide. Better ask in dev@. Very few devs are subscribed to JIRA notifications.
          Carl-Eric Menzel made changes -
          Link This issue is blocked by WICKET-5677 [ WICKET-5677 ]
          Hide
          Carl-Eric Menzel added a comment -

          I created WICKET-5677 to track the improvement I think we need for this.

          Show
          Carl-Eric Menzel added a comment - I created WICKET-5677 to track the improvement I think we need for this.
          Hide
          Carl-Eric Menzel added a comment -

          A quickstart that demonstrates the problem. A single info message is generated and should be shown only in the "container feedback" panel. After using "next" and "previous" to step backwards and forwards, the message shows up twice - once where it is supposed to be, and once in the FFP used by the wizard.

          Show
          Carl-Eric Menzel added a comment - A quickstart that demonstrates the problem. A single info message is generated and should be shown only in the "container feedback" panel. After using "next" and "previous" to step backwards and forwards, the message shows up twice - once where it is supposed to be, and once in the FFP used by the wizard.
          Carl-Eric Menzel made changes -
          Attachment fencedfeedbacktest.tar.gz [ 12662457 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 30a2849ed6276ac9eb91287a467a77cc0e36dc80 in wicket's branch refs/heads/WICKET-5265 from Carl-Eric Menzel
          [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=30a2849 ]

          Merge branch 'WICKET-5677' into WICKET-5265

          Show
          ASF subversion and git services added a comment - Commit 30a2849ed6276ac9eb91287a467a77cc0e36dc80 in wicket's branch refs/heads/ WICKET-5265 from Carl-Eric Menzel [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=30a2849 ] Merge branch ' WICKET-5677 ' into WICKET-5265
          Hide
          ASF subversion and git services added a comment -

          Commit 19af28e9c506433316500a0d56151bc49551acfa in wicket's branch refs/heads/WICKET-5265 from Carl-Eric Menzel
          [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=19af28e ]

          WICKET-5265 FencedFeedbackPanel uses onReAdd to recreate fence mark

          Show
          ASF subversion and git services added a comment - Commit 19af28e9c506433316500a0d56151bc49551acfa in wicket's branch refs/heads/ WICKET-5265 from Carl-Eric Menzel [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=19af28e ] WICKET-5265 FencedFeedbackPanel uses onReAdd to recreate fence mark
          Hide
          Carl-Eric Menzel added a comment -

          I pushed branch "WICKET-5265" with a fix based on WICKET-5677. (if nobody objects in the next couple days, i will do a squash merge to have one clean commit in the main branch later, including issue number, of course.)

          Show
          Carl-Eric Menzel added a comment - I pushed branch " WICKET-5265 " with a fix based on WICKET-5677 . (if nobody objects in the next couple days, i will do a squash merge to have one clean commit in the main branch later, including issue number, of course.)
          Hide
          ASF subversion and git services added a comment -

          Commit 0eb596df3003006cb3d99988fc2e1f65e16c14cb in wicket's branch refs/heads/master from Carl-Eric Menzel
          [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=0eb596d ]

          WICKET-5265 recreate fence mark when removing and readding a fencedfeedbackpanel

          Show
          ASF subversion and git services added a comment - Commit 0eb596df3003006cb3d99988fc2e1f65e16c14cb in wicket's branch refs/heads/master from Carl-Eric Menzel [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=0eb596d ] WICKET-5265 recreate fence mark when removing and readding a fencedfeedbackpanel
          Hide
          ASF subversion and git services added a comment -

          Commit 734ecfbe1bec8436d31248612aa1bfe868e32342 in wicket's branch refs/heads/wicket-6.x from Carl-Eric Menzel
          [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=734ecfb ]

          WICKET-5265 recreate fence mark when removing and readding a fencedfeedbackpanel

          Show
          ASF subversion and git services added a comment - Commit 734ecfbe1bec8436d31248612aa1bfe868e32342 in wicket's branch refs/heads/wicket-6.x from Carl-Eric Menzel [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=734ecfb ] WICKET-5265 recreate fence mark when removing and readding a fencedfeedbackpanel
          Martin Grigorov made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Fix Version/s 6.18.0 [ 12327142 ]
          Fix Version/s 7.0.0-M4 [ 12328547 ]
          Resolution Fixed [ 1 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          464d 21h 31m 1 Martin Grigorov 14/Oct/14 09:04

            People

            • Assignee:
              Carl-Eric Menzel
              Reporter:
              vineet semwal
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development