MyFaces Core
  1. MyFaces Core
  2. MYFACES-2774

Remove MARK_DELETED attribute from the component

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.0
    • Fix Version/s: 2.0.2
    • Component/s: General, JSR-314
    • Labels:
      None

      Description

      The ComponentSupport.MARK_DELETED attribute is used only inside one request. It doesn't need to be saved in the state. It should be removed from the attributes of the component. Instead a list of components marked for deletion should be included in the FaceletContext.

      1. markDeletedFaceletContext.patch
        12 kB
        Marius Petoi
      2. markDeletedFaceletContext11.patch
        30 kB
        Leonardo Uribe
      3. markDeletedFaceletContext2.patch
        16 kB
        Marius Petoi
      4. markDeletedFaceletContext3.patch
        16 kB
        Marius Petoi
      5. markDeletedFaceletContext4.patch
        17 kB
        Marius Petoi
      6. markDeletedFaceletContext5.patch
        21 kB
        Leonardo Uribe
      7. markDeletedFaceletContext6.patch
        23 kB
        Marius Petoi
      8. markDeletedFaceletContext7.patch
        23 kB
        Leonardo Uribe
      9. markDeletedFaceletContext8.patch
        23 kB
        Leonardo Uribe
      10. markDeletedFaceletContext9.patch
        25 kB
        Marius Petoi

        Issue Links

          Activity

          Hide
          Marius Petoi added a comment -

          The ComponentSupport.MARK_DELETED attribute is removed from the attributes of the component. Instead, the DefaultFaceletContext is improved with one more attribute: _componentsMarkedForDeletion and three more methods that operate on this list: addComponentMarkedForDeletion, removeComponentMarkedForDeletion, isComponentMarkedForDeletion. The ComponentSupport.finalizeForDeletion and ComponentSupport.markForDeletion methods are changed accordingly.

          Show
          Marius Petoi added a comment - The ComponentSupport.MARK_DELETED attribute is removed from the attributes of the component. Instead, the DefaultFaceletContext is improved with one more attribute: _componentsMarkedForDeletion and three more methods that operate on this list: addComponentMarkedForDeletion, removeComponentMarkedForDeletion, isComponentMarkedForDeletion. The ComponentSupport.finalizeForDeletion and ComponentSupport.markForDeletion methods are changed accordingly.
          Hide
          Leonardo Uribe added a comment -

          I reviewed the patch and the idea is fine, just we need to adjust some details.

          The only possible enhancement I see is use a Map<UIComponent, Boolean> instead a List<UIComponent> for _componentsMarkedForDeletion. I guess for addition and removal operations in this case a HashMap is faster than an ArrayList.

          Don't change the signature of javax.faces classes (the patch contains some additional methods for FaceletContext). Instead, in myfaces we have a class called AbstractFaceletContext where we have all methods that we need on FaceletContext, but we can't add them.

          In theory we should try to use FaceletCompositionContext instead add methods on AbstractFaceletContext (see MYFACES-2629 for details).

          Show
          Leonardo Uribe added a comment - I reviewed the patch and the idea is fine, just we need to adjust some details. The only possible enhancement I see is use a Map<UIComponent, Boolean> instead a List<UIComponent> for _componentsMarkedForDeletion. I guess for addition and removal operations in this case a HashMap is faster than an ArrayList. Don't change the signature of javax.faces classes (the patch contains some additional methods for FaceletContext). Instead, in myfaces we have a class called AbstractFaceletContext where we have all methods that we need on FaceletContext, but we can't add them. In theory we should try to use FaceletCompositionContext instead add methods on AbstractFaceletContext (see MYFACES-2629 for details).
          Hide
          Leonardo Uribe added a comment -

          Other option instead use a Map<UIComponent, Boolean> per view could be use a Stack<Map<UIComponent, Boolean> > or something similar. ComponentSupport.markForDeletion is called first and then ComponentSupport.finalizeForDeletion is called last, but most times if no components where marked for deletion on the current "level" finalizeForDeletion does not do anything, but in that way or something similar we can "detect" when this is happening.

          Show
          Leonardo Uribe added a comment - Other option instead use a Map<UIComponent, Boolean> per view could be use a Stack<Map<UIComponent, Boolean> > or something similar. ComponentSupport.markForDeletion is called first and then ComponentSupport.finalizeForDeletion is called last, but most times if no components where marked for deletion on the current "level" finalizeForDeletion does not do anything, but in that way or something similar we can "detect" when this is happening.
          Hide
          Marius Petoi added a comment -

          Hi Leonardo,

          Thank you for the suggestions. I attached a new patch that implements the changes you required. Please have a look over it and tell me whether there is anything else I need to modify.

          Regards,
          Marius

          Show
          Marius Petoi added a comment - Hi Leonardo, Thank you for the suggestions. I attached a new patch that implements the changes you required. Please have a look over it and tell me whether there is anything else I need to modify. Regards, Marius
          Hide
          Leonardo Uribe added a comment -

          Just some comments about how to enhance this patch. I see this part:

          public void addComponentLevelMarkedForDeletion()
          {
          if (_componentsMarkedForDeletion == null)

          { _componentsMarkedForDeletion = new Stack<Map<UIComponent, Boolean>>(); }

          _componentsMarkedForDeletion.push(new HashMap<UIComponent, Boolean>());
          }

          By default it creates a HashMap of initial capacity of 16 and load factor of 0.75. But in this case, addComponentLevelMarkedForDeletion is called from ComponentSupport.markForDeletion and in that place we know the number of elements we need to add (current component + # children + # facets). So, we can take advantage of that information and configure the HashMap initial capacity / load factor properly.

          I don't know which one (Map<UIComponent, Boolean> vs Stack<Map<UIComponent, Boolean> >) will be a better structure for deal with this enhancement from the "memory" and "speed" perspective. Why create a HashMap each time I traverse a component ? What happen if the component has no children/facets ? Maybe I'm going too far but note this code is critical for performance, so it is worth to check this stuff carefully. I suggest a Stack<Map<UIComponent, Boolean> >, but why not an ArrayList<Map<UIComponent, Boolean> >?.

          Show
          Leonardo Uribe added a comment - Just some comments about how to enhance this patch. I see this part: public void addComponentLevelMarkedForDeletion() { if (_componentsMarkedForDeletion == null) { _componentsMarkedForDeletion = new Stack<Map<UIComponent, Boolean>>(); } _componentsMarkedForDeletion.push(new HashMap<UIComponent, Boolean>()); } By default it creates a HashMap of initial capacity of 16 and load factor of 0.75. But in this case, addComponentLevelMarkedForDeletion is called from ComponentSupport.markForDeletion and in that place we know the number of elements we need to add (current component + # children + # facets). So, we can take advantage of that information and configure the HashMap initial capacity / load factor properly. I don't know which one (Map<UIComponent, Boolean> vs Stack<Map<UIComponent, Boolean> >) will be a better structure for deal with this enhancement from the "memory" and "speed" perspective. Why create a HashMap each time I traverse a component ? What happen if the component has no children/facets ? Maybe I'm going too far but note this code is critical for performance, so it is worth to check this stuff carefully. I suggest a Stack<Map<UIComponent, Boolean> >, but why not an ArrayList<Map<UIComponent, Boolean> >?.
          Hide
          Marius Petoi added a comment -

          Hi Leonardo,

          Stack inherits Vector, so it has all its methods synchronized. This is not very performance-efficient, so it would be better to use ArrayList instead. A HashMap is created every time the markForDeletion and finalizeForDeletion methods are invoked. So, a HashMap is created for each component for which the methods are invoked. This way, the add, get and remove methods are invoked only once per component and always for the last element (in the ArrayList). This way, the advantages of the HashMap are also used. When the component has no children or facets, there is no problem. the algorithm iterates over the children and facets of the component and then checks whether they are marked as deleted. So, it is ok.

          I shall replace the Stack with an ArrayList, that is used as a stack. I shall upload the patch once I will have finished.

          Regards,
          Marius

          Show
          Marius Petoi added a comment - Hi Leonardo, Stack inherits Vector, so it has all its methods synchronized. This is not very performance-efficient, so it would be better to use ArrayList instead. A HashMap is created every time the markForDeletion and finalizeForDeletion methods are invoked. So, a HashMap is created for each component for which the methods are invoked. This way, the add, get and remove methods are invoked only once per component and always for the last element (in the ArrayList). This way, the advantages of the HashMap are also used. When the component has no children or facets, there is no problem. the algorithm iterates over the children and facets of the component and then checks whether they are marked as deleted. So, it is ok. I shall replace the Stack with an ArrayList, that is used as a stack. I shall upload the patch once I will have finished. Regards, Marius
          Hide
          Marius Petoi added a comment -

          As a matter of fact, looking over the FaceletCompositionContext, I see that all the stacks are implemented using LinkedList objects. And from the speed point of view, the getLast method of LinkedList is better than the size combined with get(index) of ArrayList. So, I shall use LinkedList, like you did with all the other stacks.

          Show
          Marius Petoi added a comment - As a matter of fact, looking over the FaceletCompositionContext, I see that all the stacks are implemented using LinkedList objects. And from the speed point of view, the getLast method of LinkedList is better than the size combined with get(index) of ArrayList. So, I shall use LinkedList, like you did with all the other stacks.
          Hide
          Marius Petoi added a comment -

          This is the patch with the last modifications (the use of LinkedList instead of Stack and the creation of the Map with the exact size).

          Show
          Marius Petoi added a comment - This is the patch with the last modifications (the use of LinkedList instead of Stack and the creation of the Map with the exact size).
          Hide
          Leonardo Uribe added a comment -

          I have an idea. Take a look at UIData.saveDescendantComponentStates or UIData.restoreDescendantComponentStates. It has an interesting algorithm that maybe we could apply in this case. Most of the times, the structure of the tree does not change, so an iterator will traverse the values in the same order. So, it is not really necessary a map. Maybe an simple array could do the job in a efficient way. Note if by some reason the component does not match we should traverse the whole array but in practice this will be done very few times.

          If the component does not have any facets or children, maybe we can prevent create a new HashMap (or ArrayList) using a dummy static constant (maybe a integer like -1).

          Show
          Leonardo Uribe added a comment - I have an idea. Take a look at UIData.saveDescendantComponentStates or UIData.restoreDescendantComponentStates. It has an interesting algorithm that maybe we could apply in this case. Most of the times, the structure of the tree does not change, so an iterator will traverse the values in the same order. So, it is not really necessary a map. Maybe an simple array could do the job in a efficient way. Note if by some reason the component does not match we should traverse the whole array but in practice this will be done very few times. If the component does not have any facets or children, maybe we can prevent create a new HashMap (or ArrayList) using a dummy static constant (maybe a integer like -1).
          Hide
          Marius Petoi added a comment -

          I replaced the map with an array. The order in which the children are added in the array and then extracted from the array is the same. So, the search is immediate.

          Show
          Marius Petoi added a comment - I replaced the map with an array. The order in which the children are added in the array and then extracted from the array is the same. So, the search is immediate.
          Hide
          Leonardo Uribe added a comment -

          I attached a patch with some corrections (LinkedList does not have push). Unfortunately the patch provided has errors. I attached a junit test case to see the problem.

          Note this code:

          for (i = index; i >= 0 && componentsMarkedForDeletion[i] != child; i--);
          if (i >= 0)
          {
          componentsMarkedForDeletion[i] = null;
          iter.remove();
          if (i == index)

          { index--; }

          }

          Does the ";" after the for intentional?

          It could be good if you can provide more tests based on the one proposed.

          Show
          Leonardo Uribe added a comment - I attached a patch with some corrections (LinkedList does not have push). Unfortunately the patch provided has errors. I attached a junit test case to see the problem. Note this code: for (i = index; i >= 0 && componentsMarkedForDeletion [i] != child; i--); if (i >= 0) { componentsMarkedForDeletion [i] = null; iter.remove(); if (i == index) { index--; } } Does the ";" after the for intentional? It could be good if you can provide more tests based on the one proposed.
          Hide
          Marius Petoi added a comment -

          Hi Leonardo,

          The problem was in the test case. In the second test, you were using if.xml, which has no start, nor end elements, so they should not be in the view.

          The algorithm adds the children at the end of the array and then goes backwards through the array, removing them one by one. If they are in the same order, the for will stop after the first element. Otherwise, it will execute until the child is found.

          Show
          Marius Petoi added a comment - Hi Leonardo, The problem was in the test case. In the second test, you were using if.xml, which has no start, nor end elements, so they should not be in the view. The algorithm adds the children at the end of the array and then goes backwards through the array, removing them one by one. If they are in the same order, the for will stop after the first element. Otherwise, it will execute until the child is found.
          Hide
          Leonardo Uribe added a comment -

          Hi Marius

          The patch still has a bug. I'll explain it in detail. The test looks like this:

          <ui:composition>
          <h:outputText id="start" value="Start"/>
          <c:if test="#

          {employee.management}">
          <h:form id="form">
          <h:commandButton id="button" action="test" rendered="#{true}"/>
          </h:form>
          </c:if>
          <h:outputText id="end" value="End"/>
          </ui:composition>

          So, "start" and "end" component should always be rendered. I created two tests, one that simulates "#{employee.management}

          " go from false to true and viceversa. Both fails, because "start" and "end" are trimmed from component tree.

          Show
          Leonardo Uribe added a comment - Hi Marius The patch still has a bug. I'll explain it in detail. The test looks like this: <ui:composition> <h:outputText id="start" value="Start"/> <c:if test="# {employee.management}"> <h:form id="form"> <h:commandButton id="button" action="test" rendered="#{true}"/> </h:form> </c:if> <h:outputText id="end" value="End"/> </ui:composition> So, "start" and "end" component should always be rendered. I created two tests, one that simulates "#{employee.management} " go from false to true and viceversa. Both fails, because "start" and "end" are trimmed from component tree.
          Hide
          Marius Petoi added a comment -

          Hi Leonardo,

          The test you are talking about is "if2.xhtml". In the test case, you have the following lines:

          e.setManagement(false);
          vdl.buildView(facesContext, root,"if2.xhtml");

          c = root.findComponent("start");
          Assert.assertNotNull("start is null", c);
          c = root.findComponent("end");
          Assert.assertNotNull("end is null", c);

          These assertions pass.

          Afterwards, the "#

          {employee.management}" is set to "true" but also the view is changed ("if.xml"):

          e.setManagement(true);
          facesContext.getAttributes().remove("if.xml");
          root = facesContext.getViewRoot();
          vdl.buildView(facesContext, root,"if.xml"); // here the view is now if.xml, not if2.xhtml

          The "if.xml" looks like this:

          <ui:composition>
          <c:if test="#{employee.management}

          ">
          <h:form id="form">
          <h:commandButton id="button" action="test" rendered="#

          {true}

          "/>
          </h:form>
          </c:if>
          </ui:composition>

          So there is no start, no end elements here. So the assertions should be:

          c = root.findComponent("start");
          Assert.assertNull("start is not null", c);
          c = root.findComponent("end");
          Assert.assertNull("end is not null", c);

          Show
          Marius Petoi added a comment - Hi Leonardo, The test you are talking about is "if2.xhtml". In the test case, you have the following lines: e.setManagement(false); vdl.buildView(facesContext, root,"if2.xhtml"); c = root.findComponent("start"); Assert.assertNotNull("start is null", c); c = root.findComponent("end"); Assert.assertNotNull("end is null", c); These assertions pass. Afterwards, the "# {employee.management}" is set to "true" but also the view is changed ("if.xml"): e.setManagement(true); facesContext.getAttributes().remove("if.xml"); root = facesContext.getViewRoot(); vdl.buildView(facesContext, root,"if.xml"); // here the view is now if.xml, not if2.xhtml The "if.xml" looks like this: <ui:composition> <c:if test="#{employee.management} "> <h:form id="form"> <h:commandButton id="button" action="test" rendered="# {true} "/> </h:form> </c:if> </ui:composition> So there is no start, no end elements here. So the assertions should be: c = root.findComponent("start"); Assert.assertNull("start is not null", c); c = root.findComponent("end"); Assert.assertNull("end is not null", c);
          Hide
          Leonardo Uribe added a comment -

          Hi Marius

          I found another error on the test, so I attached another patch. It always should reference if2.xhtml. The components that render start and end should be preserved. The assertion is fine. Think about it: why "start" and "end" dissapear after beign applied? Try the test but without apply the solution and you'll see in this way it pass. Now apply the remainig changes and the test fails.

          Show
          Leonardo Uribe added a comment - Hi Marius I found another error on the test, so I attached another patch. It always should reference if2.xhtml. The components that render start and end should be preserved. The assertion is fine. Think about it: why "start" and "end" dissapear after beign applied? Try the test but without apply the solution and you'll see in this way it pass. Now apply the remainig changes and the test fails.
          Hide
          Marius Petoi added a comment -

          Hi Leonardo,

          I finally see what you mean. The problem was that first of all the UIViewRoot component was marked for deletion. Implicitely, all its children were marked for deletion, too. Then, the apply method was invoked in which the new tree was constructed. Here, the start and end were marked for deletion and also deleted (using finalizeForDeletion). Afterwards, the new components were added to the new tree. When apply was done, the new tree was finished. But then finalizeForDeletion for the UIViewRoot was invoked and start and end were deleted again.

          I attached a new patch. When the component is marked for deletion, it is removed from the previous level of marking for deletion (if it exists there). I don't know how efficient this is, as it iterates over the previous level, but this is the only way to avoid deleting the component twice.

          Marius

          Show
          Marius Petoi added a comment - Hi Leonardo, I finally see what you mean. The problem was that first of all the UIViewRoot component was marked for deletion. Implicitely, all its children were marked for deletion, too. Then, the apply method was invoked in which the new tree was constructed. Here, the start and end were marked for deletion and also deleted (using finalizeForDeletion). Afterwards, the new components were added to the new tree. When apply was done, the new tree was finished. But then finalizeForDeletion for the UIViewRoot was invoked and start and end were deleted again. I attached a new patch. When the component is marked for deletion, it is removed from the previous level of marking for deletion (if it exists there). I don't know how efficient this is, as it iterates over the previous level, but this is the only way to avoid deleting the component twice. Marius
          Hide
          Leonardo Uribe added a comment -

          After checking in deep this patch, I found that the most expensive operation is finalizeForDeletion, because per each component it traverse all components in the previous level just to found it and remove it.

          Instead, it is better to use a List<Map<String, UIComponent>> for several reasons:

          • It is possible to reuse the map on each level (use an ArrayList is better over LinkedList).
          • In this case we need random access for finalizeForDeletion, and use a Map works best.
          • the key could be MARK_CREATED attribute, because all components with a facelet taghandler has it, and if a component does not have it there is no problem.

          Also, I think the included methods on FaceletCompositionContext are too tied to the implementation detail of the algorithm. Instead, it is better to add just two methods:

          public abstract void markForDeletion(UIComponent component)
          public abstract void finalizeForDeletion(UIComponent component)

          and the remaining methods call them in a private way.

          I also solved a bug when more that one component is added to a facet:

          <h:form id="form">
          <f:facet name="header">
          <c:if test="#

          {employee.management}

          ">
          <h:commandButton id="button1" action="test" rendered="#

          {true}"/>
          <h:commandButton id="button2" action="test" rendered="#{true}

          "/>
          </c:if>
          </f:facet>
          </h:form>

          That notation was invalid on facelets 1.1.x but on jsf 2.0, with the introduction of f:metadata, which it is a facet that could contain many components, the previous code becomes valid and should be handled properly.

          Thanks to Marius Petoi for its help with this issue. It was very hard to solve it.

          Show
          Leonardo Uribe added a comment - After checking in deep this patch, I found that the most expensive operation is finalizeForDeletion, because per each component it traverse all components in the previous level just to found it and remove it. Instead, it is better to use a List<Map<String, UIComponent>> for several reasons: It is possible to reuse the map on each level (use an ArrayList is better over LinkedList). In this case we need random access for finalizeForDeletion, and use a Map works best. the key could be MARK_CREATED attribute, because all components with a facelet taghandler has it, and if a component does not have it there is no problem. Also, I think the included methods on FaceletCompositionContext are too tied to the implementation detail of the algorithm. Instead, it is better to add just two methods: public abstract void markForDeletion(UIComponent component) public abstract void finalizeForDeletion(UIComponent component) and the remaining methods call them in a private way. I also solved a bug when more that one component is added to a facet: <h:form id="form"> <f:facet name="header"> <c:if test="# {employee.management} "> <h:commandButton id="button1" action="test" rendered="# {true}"/> <h:commandButton id="button2" action="test" rendered="#{true} "/> </c:if> </f:facet> </h:form> That notation was invalid on facelets 1.1.x but on jsf 2.0, with the introduction of f:metadata, which it is a facet that could contain many components, the previous code becomes valid and should be handled properly. Thanks to Marius Petoi for its help with this issue. It was very hard to solve it.

            People

            • Assignee:
              Leonardo Uribe
              Reporter:
              Marius Petoi
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development