javaserverfaces
  1. javaserverfaces
  2. JAVASERVERFACES-2373

Component moves - reparenting looses the component in the tree completely

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1.7
    • Fix Version/s: 2.1.8
    • Component/s: state saving
    • Labels:
      None
    • Environment:

      Any environment

      Description

      In Trinidad framework, we have a customization / changeManager feature, one of the subfeatures there is to be able to move components around in the tree. The usecase is a very common Design-time at Run-time feature where the components are moved by Drag-And-Drop in a canvas. This feature is broken when we recently tried to update to Mojarra 2.1.7, so this is a regression from Mojarra for which we are requesting a fix a.s.a.p.

      I have tried to scale down this feature in a simple managed bean code and attached the testcase. All I'm doing in the managed bean is reparenting the component to move, and expecting that 1. if the component tree state is restored by RI, the component is in its destination, or 2. if the jsf page is re-read and tags re-executed, then RI should re-instate the component is in its original position, and remove the component from its destination. The component being in its original position was the behavior from Mojarra until 2.1.7. From 2.1.7 onwards, the component that was moved is no more in the tree.

      To test it, run the facelets jsf page provided as testcase (note the components are from Trinidad, so you will need to include Trinidad library, you can yourself emulate the behavior in any other component library you wish, since the essense of defect is showcased). In the page, click on the first button, that will move the component inside PanelBox1. If you set breakpoint in the bean code, you will see reparenting successful.

      After this, when the page is re-rendered, the component is expected to be re-introduced in its original position (because it is in the page definition whose tags are processed), this was the behavior so far. However this behavior is no more 2.1.7 onwards, notice that the component is no more restored to the original location (this is the issue).

      1. ChangeBeanOne.java
        1 kB
        prakashudupa
      2. changebundle.txt
        69 kB
        Manfred Riem
      3. changebundle2.txt
        70 kB
        Manfred Riem
      4. changebundle3.txt
        1 kB
        Manfred Riem
      5. changebundle-fss.txt
        148 kB
        Manfred Riem
      6. testCase.jsf
        2 kB
        prakashudupa

        Issue Links

          Activity

          Hide
          Ed Burns added a comment -

          http://java.net/jira/browse/JAVASERVERFACES-2373

          These are Ed's notes when reviewing the 20120509 changebundle.
          Actionable comments addressed to Manfred start with mriem:.

          Please look through these suggestions and address my comments. I think I'd like to see another iteration.

          SECTION: test changes

          M jsf-ri/test/com/sun/faces/application/TestViewHandlerImpl.java
          M jsf-ri/test/com/sun/faces/context/TestStateContext.java
          M jsf-ri/test/com/sun/faces/component/visit/TestTreeWithUIDataVisit.java
          M test/agnostic/dynamic/src/test/java/com/sun/faces/test/agnostic/dynamic/Issue2373IT.java
          A test/agnostic/dynamic/src/main/java/com/sun/faces/test/agnostic/dynamic/RemoveComponentBean.java
          M test/agnostic/dynamic/src/main/webapp/index.xhtml
          A test/agnostic/dynamic/src/main/webapp/removeComponent.xhtml

          SECTION: State Management changes

          M jsf-ri/src/main/java/com/sun/faces/application/StateManagerImpl.java

          General comments on this class:

          • Overall, this class looks much cleaner, but I'm always suspicious of
            code that looks dramatically simpler, because stuff that looks dirty
            is often just bugfixes. Nonetheless, I'll trust that your changes
            simultaneously add clarity, remove complexity, and preserve bug fixes.
            That's a tall order, though!
          • mriem: Four or so methods and two inner classes have been removed.
            Where did these end up?
          • Remove apologies from class javadoc.
          • Remove ctor, moved to JspStateManagementStrategy.
          • Remove all ivars, moved to JspStateManagementStrategy.

          In method saveView():

          • mriem: on line 69, you access the context local variable, but check
            for its nullness on line 71. That's an NPE waiting to happen.
          • mriem: you've removed the id uniqueness check from this method. I
            assume it will show up somewhere else. Please verify that it does.

          In method restoreView():

          • mriem: you've removed the wordy comment about the need to clone the
            tree... Is that business handled somewhere else now? Yes: looks like
            it's in the new class JspStateManagementStrategy.

          A jsf-ri/src/main/java/com/sun/faces/application/view/JspStateManagementStrategy.java

          • Assume the responsibilities from the former StateManagerImpl.java

          M jsf-ri/src/main/java/com/sun/faces/application/view/StateManagementStrategyImpl.java

          • Removed. Functionality taken over by pair of classes:
            FaceletPartialStateManagementStrategy and
            FaceletFullStateManagementStrategy.
          • mriem: But I note that

          ./test/unit/src/main/java/com/sun/faces/application/view/StateManagementStrategyImpl.java

          still has this old implementation. Is that intentional?

          A jsf-ri/src/main/java/com/sun/faces/application/view/FaceletPartialStateManagementStrategy.java

          Global comments:

          • mriem: you have a zero arg ctor, but it's only called by test code
            (and cactus code no less). To guard against someone calling that ctor
            for real, perhaps invent some kind of magic argument that is checked
            by the ctor and throws an IllegalStateException if it's not right?
            Something like a string with a value known by the test and the real
            code? The test will pass the literal string?
          • Again, this looks much cleaner.
          • In method saveView():

          Ahh, here is the id uniqueness check.

          How about adding a context param that makes this off when ProjectStage
          is Development? After sitting through Leonardo Uribe's MyFaces vs
          Mojarra performance comparison, I'm ready to entertain such
          expediences.
          A jsf-ri/src/main/java/com/sun/faces/application/view/FaceletFullStateManagementStrategy.java

          • mriem: please add a comment that explains why this class looks so
            similar to JspStateManagementStrategy. And a question for you, how
            does this actually differ from JspStateManagementStrategy?

          M jsf-ri/src/main/java/com/sun/faces/context/StateContext.java

          • change name of method partialStateSaving() to be
            isPartialStateSaving(). Otherwise this file is unchanged.

          M jsf-ri/src/main/java/com/sun/faces/facelets/tag/jsf/ComponentSupport.java

          • Change callsite of partialStateSaving().

          M jsf-ri/src/main/java/com/sun/faces/application/view/FaceletViewHandlingStrategy.java

          • I note that FaceletPartialStateManagementStrategy has a
            locateComponentByClientId() method, and FaceletViewHandlingStrategy
            does too. How are these to incarnations of the same named method
            related? Can this be moved to Util?
          Show
          Ed Burns added a comment - http://java.net/jira/browse/JAVASERVERFACES-2373 These are Ed's notes when reviewing the 20120509 changebundle. Actionable comments addressed to Manfred start with mriem:. Please look through these suggestions and address my comments. I think I'd like to see another iteration. SECTION: test changes M jsf-ri/test/com/sun/faces/application/TestViewHandlerImpl.java M jsf-ri/test/com/sun/faces/context/TestStateContext.java M jsf-ri/test/com/sun/faces/component/visit/TestTreeWithUIDataVisit.java M test/agnostic/dynamic/src/test/java/com/sun/faces/test/agnostic/dynamic/Issue2373IT.java A test/agnostic/dynamic/src/main/java/com/sun/faces/test/agnostic/dynamic/RemoveComponentBean.java M test/agnostic/dynamic/src/main/webapp/index.xhtml A test/agnostic/dynamic/src/main/webapp/removeComponent.xhtml SECTION: State Management changes M jsf-ri/src/main/java/com/sun/faces/application/StateManagerImpl.java General comments on this class: Overall, this class looks much cleaner, but I'm always suspicious of code that looks dramatically simpler, because stuff that looks dirty is often just bugfixes. Nonetheless, I'll trust that your changes simultaneously add clarity, remove complexity, and preserve bug fixes. That's a tall order, though! mriem: Four or so methods and two inner classes have been removed. Where did these end up? Remove apologies from class javadoc. Remove ctor, moved to JspStateManagementStrategy. Remove all ivars, moved to JspStateManagementStrategy. In method saveView(): mriem: on line 69, you access the context local variable, but check for its nullness on line 71. That's an NPE waiting to happen. mriem: you've removed the id uniqueness check from this method. I assume it will show up somewhere else. Please verify that it does. In method restoreView(): mriem: you've removed the wordy comment about the need to clone the tree... Is that business handled somewhere else now? Yes: looks like it's in the new class JspStateManagementStrategy. A jsf-ri/src/main/java/com/sun/faces/application/view/JspStateManagementStrategy.java Assume the responsibilities from the former StateManagerImpl.java M jsf-ri/src/main/java/com/sun/faces/application/view/StateManagementStrategyImpl.java Removed. Functionality taken over by pair of classes: FaceletPartialStateManagementStrategy and FaceletFullStateManagementStrategy. mriem: But I note that ./test/unit/src/main/java/com/sun/faces/application/view/StateManagementStrategyImpl.java still has this old implementation. Is that intentional? A jsf-ri/src/main/java/com/sun/faces/application/view/FaceletPartialStateManagementStrategy.java Global comments: mriem: you have a zero arg ctor, but it's only called by test code (and cactus code no less). To guard against someone calling that ctor for real, perhaps invent some kind of magic argument that is checked by the ctor and throws an IllegalStateException if it's not right? Something like a string with a value known by the test and the real code? The test will pass the literal string? Again, this looks much cleaner. In method saveView(): Ahh, here is the id uniqueness check. How about adding a context param that makes this off when ProjectStage is Development? After sitting through Leonardo Uribe's MyFaces vs Mojarra performance comparison, I'm ready to entertain such expediences. A jsf-ri/src/main/java/com/sun/faces/application/view/FaceletFullStateManagementStrategy.java mriem: please add a comment that explains why this class looks so similar to JspStateManagementStrategy. And a question for you, how does this actually differ from JspStateManagementStrategy? M jsf-ri/src/main/java/com/sun/faces/context/StateContext.java change name of method partialStateSaving() to be isPartialStateSaving(). Otherwise this file is unchanged. M jsf-ri/src/main/java/com/sun/faces/facelets/tag/jsf/ComponentSupport.java Change callsite of partialStateSaving(). M jsf-ri/src/main/java/com/sun/faces/application/view/FaceletViewHandlingStrategy.java I note that FaceletPartialStateManagementStrategy has a locateComponentByClientId() method, and FaceletViewHandlingStrategy does too. How are these to incarnations of the same named method related? Can this be moved to Util?
          Hide
          Manfred Riem added a comment -

          M jsf-ri/src/main/java/com/sun/faces/application/StateManagerImpl.java

          1. The methods have moved to both FaceletFullStateManagementStrategy and JspStateManagementStrategy.

          2. In method saveView: will be moving that context.getViewRoot down into if block

          3. Uniqueness check shows up in each of the StateManagementStrategy impls as it is their responsibility.

          4. Correct, see 1. above.

          test/unit/src/main/java/com/sun/faces/application/view/StateManagementStrategyImpl.java

          1. This is your local workspace playing a trick on you. The test/unit project downloads the latest
          installed copy from your local Maven repo. If you did not build using mvn.deploy.snapshot.local this
          is why that is happening. It won't be part of the commit.

          Global comments:

          1. Not sure what you mean to convey here. Yes it is possible to construct an instance of a particular
          implementation of the StateManagementStrategies.

          A jsf-ri/src/main/java/com/sun/faces/application/view/FaceletFullStateManagementStrategy.java

          They look so similar because they both originated from the same source. Eventually I hope we can
          make it so the JSP part can be turned off completely. Separating it out should make that easier.

          M jsf-ri/src/main/java/com/sun/faces/application/view/FaceletViewHandlingStrategy.java

          Correct, more refactoring will need to be done but I did want to have a point where we could push it
          into a release. Eventually the dynamic parts of each StateManagementStrategy will be pulled up into FaceletViewHandlingStrategy and each StateManagementStrategy just saves state (and does not need to
          know about how to handle dynamic components).

          The id uniqueness check.

          I would like to delay that until after this issue. Lets file an issue to keep track of that.


          Can we go ahead and get this in with the fix for StateManagerImpl #2 in place?

          Show
          Manfred Riem added a comment - M jsf-ri/src/main/java/com/sun/faces/application/StateManagerImpl.java 1. The methods have moved to both FaceletFullStateManagementStrategy and JspStateManagementStrategy. 2. In method saveView: will be moving that context.getViewRoot down into if block 3. Uniqueness check shows up in each of the StateManagementStrategy impls as it is their responsibility. 4. Correct, see 1. above. test/unit/src/main/java/com/sun/faces/application/view/StateManagementStrategyImpl.java 1. This is your local workspace playing a trick on you. The test/unit project downloads the latest installed copy from your local Maven repo. If you did not build using mvn.deploy.snapshot.local this is why that is happening. It won't be part of the commit. Global comments: 1. Not sure what you mean to convey here. Yes it is possible to construct an instance of a particular implementation of the StateManagementStrategies. A jsf-ri/src/main/java/com/sun/faces/application/view/FaceletFullStateManagementStrategy.java They look so similar because they both originated from the same source. Eventually I hope we can make it so the JSP part can be turned off completely. Separating it out should make that easier. M jsf-ri/src/main/java/com/sun/faces/application/view/FaceletViewHandlingStrategy.java Correct, more refactoring will need to be done but I did want to have a point where we could push it into a release. Eventually the dynamic parts of each StateManagementStrategy will be pulled up into FaceletViewHandlingStrategy and each StateManagementStrategy just saves state (and does not need to know about how to handle dynamic components). The id uniqueness check. I would like to delay that until after this issue. Lets file an issue to keep track of that. Can we go ahead and get this in with the fix for StateManagerImpl #2 in place?
          Hide
          Ed Burns added a comment -

          MR> Can we go ahead and get this in with the fix for StateManagerImpl #2 in place?

          Yes, please go ahead. r=edburns.

          Show
          Ed Burns added a comment - MR> Can we go ahead and get this in with the fix for StateManagerImpl #2 in place? Yes, please go ahead. r=edburns.
          Hide
          Manfred Riem added a comment -

          Changes committed

          Show
          Manfred Riem added a comment - Changes committed
          Hide
          Manfred Riem added a comment -

          Verified and closing

          Show
          Manfred Riem added a comment - Verified and closing

            People

            • Assignee:
              Manfred Riem
              Reporter:
              prakashudupa
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - Not Specified
                Not Specified
                Remaining:
                Remaining Estimate - 0 minutes
                0m
                Logged:
                Time Spent - 1 day
                1d