Opened 8 years ago

Closed 6 years ago

#14443 closed defect (fixed)

Tree: must fail gracefully when last selected node no longer exists

Reported by: Jean-Rubin Leonard Owned by: bill
Priority: low Milestone: 1.9
Component: Dijit Version: 1.7.1
Keywords: dijit tree load expand path persist true Cc: Simon, Speich
Blocked By: Blocking:

Description

When persist is true for the dijit tree, it's supposed to position itself at the last node that was selected. When we select a node that has children, it becomes the last selected. If we delete it, it seems that the cookie is not updated to account for the fact that the last selected node has been deleted. Since the last selected node can be deleted in a number of fashion, including server side, where the node tree isn't made aware of it, It will impossible to maintain an accurate and always up to date list of the latest selected items. The tree should therefore be adjusted so that if the last selected node is gone missing, to select another one by default, either the root or the disappeared node's parent, or his grandparent if the parent has been nuked as well. This would allow to keep the persist behavior but make sure there's no error messages as nodes are deleted.

Currently when we load the tree in the specified situation, we get a "Could not expand path" error message. In the error message we get the path that it was trying to get to.

Attachments (1)

patch_adds_new_method_to_check_if_nodes_in_path_still_exist.patch (4.3 KB) - added by Simon Speich 8 years ago.
added patch which adds method to check if nodes in path still exist

Download all attachments as: .zip

Change History (11)

comment:1 Changed 8 years ago by bill

Summary: dijit tree must fail gracefully when last selected node no longer existsTree: must fail gracefully when last selected node no longer exists

Fallout from #14058. This bug actually applies when any of the nodes in the path have been deleted, not just the last one. I guess we should do a fetch of each node in the path first, to make sure it exists.

(Note also that bug will occur especially when a node is deleted from the server while the web page is closed, thus the server couldn't send a notification to the client.)

comment:2 Changed 8 years ago by Simon Speich

Bill, I am not sure where in the code the checking if a path exists should occur:

A: in _setPathsAttr() after the normalizing, but before calling selectPath() and then just remove the missing parts from the path with a new function checkPath() before feeding it into selectPath()

B: in _setPathsAttr() within the function setPath(), but instead of rejecting the deferred when node is not found, shorten the path and call setPath() again until path.length == 0

C: somewhere else?

What if the tree has changed so much that nothing is matched at all? Do we set it to root or to nothing?

BTW: what's the reason for the double negation if(!!nextNode) line 1010?

Last edited 8 years ago by Simon Speich (previous) (diff)

comment:3 Changed 8 years ago by bill

Bill, I am not sure where in the code the checking if a path exists should occur

Well, in general _setPathsAttr() should throw an error if the path doesn't exist. But the persistence code shouldn't throw an error, it should just recover. So I thought that the checking code should run before the call to _setPathsAttr() (in the section of code that calls _setPathsAttr()).

What if the tree has changed so much that nothing is matched at all? Do we set it to root or to nothing?

I thought to set it to root, or whatever tree does where persist == false.

We should add unit tests for both these cases. Previously we didn't have unit tests because it's difficult, having to clear and set cookies, but I guess all you really need to do is to clear the cookies originally, then run the Tree to a certain state, and the delete it and recreate it [after having modified the data store some].

BTW: what's the reason for the double negation if(!!nextNode) line 1010?

I assume that's needed in case the array return from filter is zero-length. Differentiating between [][0] and [false][0].

Last edited 8 years ago by bill (previous) (diff)

comment:4 Changed 8 years ago by bill

Cc: Simon Speich added

So Simon, can you give a patch for this?

comment:5 Changed 8 years ago by Simon Speich

Sorry, for not having done anything so far. I will see if I can come up with a patch this weekend.

Changed 8 years ago by Simon Speich

added patch which adds method to check if nodes in path still exist

comment:6 Changed 8 years ago by Simon Speich

The new method tree._updatePaths() checks if all nodes exist in a path. If necessary it modifies the paths before calling _set('paths'). At the moment it gets called also for paths specified as arguments to the tree constructor. Maybe it should only be called when tree.persist is set to true?

comment:7 Changed 8 years ago by Jean-Rubin Leonard

I do agree it should be called only if tree.persist is true. It's irrelevant otherwise.

comment:8 Changed 7 years ago by bill

Milestone: tbd1.9

comment:9 Changed 7 years ago by bill

Priority: highlow

comment:10 Changed 6 years ago by bill

Owner: set to bill
Resolution: fixed
Status: newclosed

In [30746]:

Set Tree::persist default to false, like other widgets, and also don't persist state of selected nodes. If desired, selectedPaths can be persisted by app code same as any other widget attribute. Fixes #14443, #16785, refs #14058 !strict.

Note: See TracTickets for help on using tickets.