[jira] Commented: (MYFACES-2774) Remove MARK_DELETED attribute from the component
[ https://issues.apache.org/jira/browse/MYFACES-2774?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12899010#action_12899010 ] Leonardo Uribe commented on MYFACES-2774: - 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. > Remove MARK_DELETED attribute from the component > > > Key: MYFACES-2774 > URL: https://issues.apache.org/jira/browse/MYFACES-2774 > Project: MyFaces Core > Issue Type: Improvement > Components: General, JSR-314 >Affects Versions: 2.0.0 >Reporter: Marius Petoi >Priority: Minor > Attachments: markDeletedFaceletContext.patch, > markDeletedFaceletContext2.patch, markDeletedFaceletContext3.patch, > markDeletedFaceletContext4.patch, markDeletedFaceletContext5.patch, > markDeletedFaceletContext6.patch, markDeletedFaceletContext7.patch, > markDeletedFaceletContext8.patch > > > 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. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (MYFACES-2774) Remove MARK_DELETED attribute from the component
[
https://issues.apache.org/jira/browse/MYFACES-2774?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12898860#action_12898860
]
Marius Petoi commented on MYFACES-2774:
---
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:
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);
> Remove MARK_DELETED attribute from the component
>
>
> Key: MYFACES-2774
> URL: https://issues.apache.org/jira/browse/MYFACES-2774
> Project: MyFaces Core
> Issue Type: Improvement
> Components: General, JSR-314
>Affects Versions: 2.0.0
>Reporter: Marius Petoi
>Priority: Minor
> Attachments: markDeletedFaceletContext.patch,
> markDeletedFaceletContext2.patch, markDeletedFaceletContext3.patch,
> markDeletedFaceletContext4.patch, markDeletedFaceletContext5.patch,
> markDeletedFaceletContext6.patch, markDeletedFaceletContext7.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (MYFACES-2774) Remove MARK_DELETED attribute from the component
[
https://issues.apache.org/jira/browse/MYFACES-2774?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12896690#action_12896690
]
Leonardo Uribe commented on MYFACES-2774:
-
Hi Marius
The patch still has a bug. I'll explain it in detail. The test looks like this:
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.
> Remove MARK_DELETED attribute from the component
>
>
> Key: MYFACES-2774
> URL: https://issues.apache.org/jira/browse/MYFACES-2774
> Project: MyFaces Core
> Issue Type: Improvement
> Components: General, JSR-314
>Affects Versions: 2.0.0
>Reporter: Marius Petoi
>Priority: Minor
> Attachments: markDeletedFaceletContext.patch,
> markDeletedFaceletContext2.patch, markDeletedFaceletContext3.patch,
> markDeletedFaceletContext4.patch, markDeletedFaceletContext5.patch,
> markDeletedFaceletContext6.patch, markDeletedFaceletContext7.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (MYFACES-2774) Remove MARK_DELETED attribute from the component
[ https://issues.apache.org/jira/browse/MYFACES-2774?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12891454#action_12891454 ] Leonardo Uribe commented on MYFACES-2774: - 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). > Remove MARK_DELETED attribute from the component > > > Key: MYFACES-2774 > URL: https://issues.apache.org/jira/browse/MYFACES-2774 > Project: MyFaces Core > Issue Type: Improvement > Components: General, JSR-314 >Affects Versions: 2.0.0 >Reporter: Marius Petoi >Priority: Minor > Attachments: markDeletedFaceletContext.patch, > markDeletedFaceletContext2.patch, markDeletedFaceletContext3.patch > > > 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. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (MYFACES-2774) Remove MARK_DELETED attribute from the component
[ https://issues.apache.org/jira/browse/MYFACES-2774?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12888743#action_12888743 ] Marius Petoi commented on MYFACES-2774: --- 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. > Remove MARK_DELETED attribute from the component > > > Key: MYFACES-2774 > URL: https://issues.apache.org/jira/browse/MYFACES-2774 > Project: MyFaces Core > Issue Type: Improvement > Components: General, JSR-314 >Affects Versions: 2.0.0 >Reporter: Marius Petoi >Priority: Minor > Attachments: markDeletedFaceletContext.patch, > markDeletedFaceletContext2.patch > > > 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. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (MYFACES-2774) Remove MARK_DELETED attribute from the component
[ https://issues.apache.org/jira/browse/MYFACES-2774?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12888742#action_12888742 ] Marius Petoi commented on MYFACES-2774: --- 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 > Remove MARK_DELETED attribute from the component > > > Key: MYFACES-2774 > URL: https://issues.apache.org/jira/browse/MYFACES-2774 > Project: MyFaces Core > Issue Type: Improvement > Components: General, JSR-314 >Affects Versions: 2.0.0 >Reporter: Marius Petoi >Priority: Minor > Attachments: markDeletedFaceletContext.patch, > markDeletedFaceletContext2.patch > > > 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. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (MYFACES-2774) Remove MARK_DELETED attribute from the component
[
https://issues.apache.org/jira/browse/MYFACES-2774?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12888724#action_12888724
]
Leonardo Uribe commented on MYFACES-2774:
-
Just some comments about how to enhance this patch. I see this part:
public void addComponentLevelMarkedForDeletion()
{
if (_componentsMarkedForDeletion == null)
{
_componentsMarkedForDeletion = new Stack>();
}
_componentsMarkedForDeletion.push(new HashMap());
}
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 vs Stack >) 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 >, but
why not an ArrayList >?.
> Remove MARK_DELETED attribute from the component
>
>
> Key: MYFACES-2774
> URL: https://issues.apache.org/jira/browse/MYFACES-2774
> Project: MyFaces Core
> Issue Type: Improvement
> Components: General, JSR-314
>Affects Versions: 2.0.0
>Reporter: Marius Petoi
>Priority: Minor
> Attachments: markDeletedFaceletContext.patch,
> markDeletedFaceletContext2.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (MYFACES-2774) Remove MARK_DELETED attribute from the component
[ https://issues.apache.org/jira/browse/MYFACES-2774?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12887140#action_12887140 ] Leonardo Uribe commented on MYFACES-2774: - Other option instead use a Map per view could be use a Stack > 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. > Remove MARK_DELETED attribute from the component > > > Key: MYFACES-2774 > URL: https://issues.apache.org/jira/browse/MYFACES-2774 > Project: MyFaces Core > Issue Type: Improvement > Components: General, JSR-314 >Affects Versions: 2.0.0 >Reporter: Marius Petoi >Priority: Minor > Attachments: markDeletedFaceletContext.patch > > > 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. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (MYFACES-2774) Remove MARK_DELETED attribute from the component
[ https://issues.apache.org/jira/browse/MYFACES-2774?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12887118#action_12887118 ] Leonardo Uribe commented on MYFACES-2774: - 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 instead a List 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). > Remove MARK_DELETED attribute from the component > > > Key: MYFACES-2774 > URL: https://issues.apache.org/jira/browse/MYFACES-2774 > Project: MyFaces Core > Issue Type: Improvement > Components: General >Affects Versions: 2.0.0 >Reporter: Marius Petoi >Priority: Minor > Attachments: markDeletedFaceletContext.patch > > > 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. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
