New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

If o:tree component is added dynamically to the component tree, it does not get restored correctly at the next request #228

Closed
hughnguyen82 opened this Issue Mar 31, 2016 · 8 comments

Comments

Projects
None yet
2 participants
@hughnguyen82

hughnguyen82 commented Mar 31, 2016

During RESTORE_VIEW, Tree.visitTree() is called to restore its children. However, when it eventually call Tree.getNodes(PhaseId), the processing of its children is skipped, because getNodes() only build the nodes map during RENDER_RESPONSE

@BalusC

This comment has been minimized.

Show comment
Hide comment
@BalusC

BalusC Mar 31, 2016

Member

"Added dynamically"? How exactly?

The getNodes() also builds the nodes map when it's not built yet.

Member

BalusC commented Mar 31, 2016

"Added dynamically"? How exactly?

The getNodes() also builds the nodes map when it's not built yet.

@hughnguyen82

This comment has been minimized.

Show comment
Hide comment
@hughnguyen82

hughnguyen82 Apr 1, 2016

It's added programatically at during UIViewRoot's PreRenderViewEvent handling. That approach of component tree manipulation was discussed here http://blog.kennardconsulting.com/2010/10/safely-manipulating-component-tree-with.html, and generally work fine in most cases

In current OmniFaces 2.2, getNodes() started like this:

if (phaseId == PhaseId.RENDER_RESPONSE || nodes == null) {
...
}

When visitTree is called during RESTORE_VIEW, the parameter phaseId passed in to this method is ANY_PHASE, and thus the nodes map is not build, and thus the children (i.e TreeNode) are not processed.

hughnguyen82 commented Apr 1, 2016

It's added programatically at during UIViewRoot's PreRenderViewEvent handling. That approach of component tree manipulation was discussed here http://blog.kennardconsulting.com/2010/10/safely-manipulating-component-tree-with.html, and generally work fine in most cases

In current OmniFaces 2.2, getNodes() started like this:

if (phaseId == PhaseId.RENDER_RESPONSE || nodes == null) {
...
}

When visitTree is called during RESTORE_VIEW, the parameter phaseId passed in to this method is ANY_PHASE, and thus the nodes map is not build, and thus the children (i.e TreeNode) are not processed.

@hughnguyen82

This comment has been minimized.

Show comment
Hide comment
@hughnguyen82

hughnguyen82 Apr 1, 2016

Oh, my mistake: || should cause nodes to be rebuilt. I'll investigate further and update you

hughnguyen82 commented Apr 1, 2016

Oh, my mistake: || should cause nodes to be rebuilt. I'll investigate further and update you

@hughnguyen82

This comment has been minimized.

Show comment
Hide comment
@hughnguyen82

hughnguyen82 Apr 4, 2016

Ok, I think I got it this time.
The issue seems to be at the State Saving part: the Tree's children (TreeNode) are not saved, if the TreeModel is empty.
Because the Tree component is dynamically created, it can only rely on the Restore View to restore its children at the subsequent request.

private <R> R processTreeNode(PhaseId phaseId, Callback.ReturningWithArgument<R, TreeNode> callback) {
        TreeNode treeNode = null;

        if (!currentModelNode.isLeaf()) {
            treeNode = getNodes(phaseId).get(currentModelNode.getLevel());

            if (treeNode == null) {
                treeNode = getNodes(phaseId).get(null);
            }
        }

        return callback.invoke(treeNode);
    }

If the TreeModel is empty during Render Response, then during state saving, currentModelNode.isLeaf() is true, thus the code above will not process and save the child (TreeNode).

I tried the following and it seems to fix the issue:

private <R> R processTreeNode(PhaseId phaseId, Callback.ReturningWithArgument<R, TreeNode> callback) {
        TreeNode treeNode = null;

        if (!currentModelNode.isLeaf()) {
            treeNode = getNodes(phaseId).get(currentModelNode.getLevel());          
        }

        if (treeNode == null) {
            treeNode = getNodes(phaseId).get(null);
        }

        return callback.invoke(treeNode);
    }

Thank you for your time looking into this issue

hughnguyen82 commented Apr 4, 2016

Ok, I think I got it this time.
The issue seems to be at the State Saving part: the Tree's children (TreeNode) are not saved, if the TreeModel is empty.
Because the Tree component is dynamically created, it can only rely on the Restore View to restore its children at the subsequent request.

private <R> R processTreeNode(PhaseId phaseId, Callback.ReturningWithArgument<R, TreeNode> callback) {
        TreeNode treeNode = null;

        if (!currentModelNode.isLeaf()) {
            treeNode = getNodes(phaseId).get(currentModelNode.getLevel());

            if (treeNode == null) {
                treeNode = getNodes(phaseId).get(null);
            }
        }

        return callback.invoke(treeNode);
    }

If the TreeModel is empty during Render Response, then during state saving, currentModelNode.isLeaf() is true, thus the code above will not process and save the child (TreeNode).

I tried the following and it seems to fix the issue:

private <R> R processTreeNode(PhaseId phaseId, Callback.ReturningWithArgument<R, TreeNode> callback) {
        TreeNode treeNode = null;

        if (!currentModelNode.isLeaf()) {
            treeNode = getNodes(phaseId).get(currentModelNode.getLevel());          
        }

        if (treeNode == null) {
            treeNode = getNodes(phaseId).get(null);
        }

        return callback.invoke(treeNode);
    }

Thank you for your time looking into this issue

@hughnguyen82

This comment has been minimized.

Show comment
Hide comment
@hughnguyen82

hughnguyen82 Apr 4, 2016

Sorry to bother you once more.
I attempted a more proper fix:

protected boolean visitTreeNode(final VisitContext context, final VisitCallback callback) {
        // to ensure that state saving / restoring process the children
        if (this.getCurrentModelNode().isRoot()) {
            for (final UIComponent child : this.getChildren()) {
                if (child.visitTree(context, callback)) {
                    return true;
                }
            }

            return false;
        }

        else {
            // your existing code
            return processTreeNode(PhaseId.ANY_PHASE, new Callback.ReturningWithArgument<Boolean, TreeNode>() {
                @Override
                public Boolean invoke(TreeNode treeNode) {
                    if (treeNode != null) {
                        return treeNode.visitTree(context, callback);
                    }

                    return false;
                }
            });
        }
    }

hughnguyen82 commented Apr 4, 2016

Sorry to bother you once more.
I attempted a more proper fix:

protected boolean visitTreeNode(final VisitContext context, final VisitCallback callback) {
        // to ensure that state saving / restoring process the children
        if (this.getCurrentModelNode().isRoot()) {
            for (final UIComponent child : this.getChildren()) {
                if (child.visitTree(context, callback)) {
                    return true;
                }
            }

            return false;
        }

        else {
            // your existing code
            return processTreeNode(PhaseId.ANY_PHASE, new Callback.ReturningWithArgument<Boolean, TreeNode>() {
                @Override
                public Boolean invoke(TreeNode treeNode) {
                    if (treeNode != null) {
                        return treeNode.visitTree(context, callback);
                    }

                    return false;
                }
            });
        }
    }
@BalusC

This comment has been minimized.

Show comment
Hide comment
@BalusC

BalusC Apr 8, 2016

Member

It would be helpful to have a reproducer of the problem you're attempting to solve.

I think after all Tree#visitTree() method has to be altered. Undo your changes so far and add this piece to the very top of the Tree#visitTree() method and let me know if that indeed solves it for you.

if (getModel(PhaseId.ANY_PHASE).isLeaf()) {
    return super.visitTree(context, callback);
}
Member

BalusC commented Apr 8, 2016

It would be helpful to have a reproducer of the problem you're attempting to solve.

I think after all Tree#visitTree() method has to be altered. Undo your changes so far and add this piece to the very top of the Tree#visitTree() method and let me know if that indeed solves it for you.

if (getModel(PhaseId.ANY_PHASE).isLeaf()) {
    return super.visitTree(context, callback);
}
@hughnguyen82

This comment has been minimized.

Show comment
Hide comment
@hughnguyen82

hughnguyen82 Apr 11, 2016

Your fix work perfectly, thank you very much.

Regarding a reproducer: it's a bit messy.

The o:tree is dynamically added to the tree by a dynamic include component that I build based on a suggestion from yourself here:
http://stackoverflow.com/questions/13680192/dynamic-ajax-navigation-with-uiinclude
plus some tuning using the ideas from here:
http://blog.kennardconsulting.com/2010/10/safely-manipulating-component-tree-with.html

Would you consider allowing me to contribute this component to omnifaces? If yes, I'll try to clean it up and send it to you on a separate case.

hughnguyen82 commented Apr 11, 2016

Your fix work perfectly, thank you very much.

Regarding a reproducer: it's a bit messy.

The o:tree is dynamically added to the tree by a dynamic include component that I build based on a suggestion from yourself here:
http://stackoverflow.com/questions/13680192/dynamic-ajax-navigation-with-uiinclude
plus some tuning using the ideas from here:
http://blog.kennardconsulting.com/2010/10/safely-manipulating-component-tree-with.html

Would you consider allowing me to contribute this component to omnifaces? If yes, I'll try to clean it up and send it to you on a separate case.

@BalusC BalusC closed this in cd97323 Apr 11, 2016

@BalusC

This comment has been minimized.

Show comment
Hide comment
@BalusC

BalusC Apr 11, 2016

Member

Fix is available in today's 2.4-SNAPSHOT.

Wrt the dynamic include component, oh the old story. Nice that you took a second look at it. There are indeed some things done wrong and JSF 2.2 offers new ways to dynamically create a component tree based on a template. Feel free to contribute via a pull request. I will verify and test it and eventually add to OmniFaces.

Member

BalusC commented Apr 11, 2016

Fix is available in today's 2.4-SNAPSHOT.

Wrt the dynamic include component, oh the old story. Nice that you took a second look at it. There are indeed some things done wrong and JSF 2.2 offers new ways to dynamically create a component tree based on a template. Feel free to contribute via a pull request. I will verify and test it and eventually add to OmniFaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment