Community
Participate
Working Groups
Build Identifier: M20090211-1700 Imagine that item P in a TreeViewer has one child item C which has child item D. Imagine also that (REQ) C only appears in the view if and only if it has children (the only one is D in this example). The input model also has the hierarchy P->C->D . Now, the input model is changed so that object C no longer has any child object. In this case, when viewer.refresh() is called I would like to see only P in the viewer as per REQ. However, the result of the viewer differs depending if item P is expanded or not before the call. If item P is expanded before calling refresh() then child item C still appears in the viewer, which is incorrect since ITreeContentProvider.hasChildren(P) is false! But if item P is collapsed before calling refresh() then child item C does not appear, which is correct since ITreeContentProvider.hasChildren(P) is false. I've made some dig in the code and it seems that the problem is in org.eclipse.jface.viewers.AbstractTreeViewer in method private void updateChildren(Widget widget, Object parent, Object[] elementChildren, boolean updateLabels) This method is checking if P has children when "if(!getExpanded(ti)) ..." is true (checking children existence at line "boolean needDummy = isExpandable(ti, null, parent);"), but it is not checking if P has children when "if(!getExpanded(ti)) ..." is false. I think the solution is to call ITreeContentProvider.hasChildren() ALWAYS before calling ITreeContentProvider.getChildren(), thus respecting the contract of ITreeContentProvider. This must be done everywhere for treeviewers when refreshing the viewer. Reproducible: Always Steps to Reproduce: See the details for the steps
Created attachment 173778 [details] Snippet v01 I am not sure I fully understand the bug description. Can you please attach a snippet to demonstrate this. Here is one that I normally use for treeviewer bugs.
Created attachment 173971 [details] Changed Snippet v01 to help reproduce the problem
The snipped I had shows the problem. In this snippet, the 1st, 2nd, and 3rd levels in the TreeViewer are of classes MyModel1, MyModel2, and MyModel3. The requirement is that MyModel2 elements must only appear in the viewer if they have MyModel3 children. This requirement is implemented in method boolean hasChildren(Object) in MyContentProvider. The problem is that when we press the button "Remove Item 0.0.0.0", which removes the MyModel3 element and calls viewer.refresh(), "Item 0.0.0" (a MyModel2 element) still appears in the view if "Item 0.0" is expanded before pressing the button, but it does not appear if "Item 0.0" is collapsed. As I mentioned above, it seems that the problem is in org.eclipse.jface.viewers.AbstractTreeViewer in method private void updateChildren(Widget widget, Object parent, Object[] elementChildren, boolean updateLabels) You may say this is a "strange requirement", and it might be so, but indeed it reveals that the ITreeContentProvider contract was broken, that is, if ITreeContentProvider.hasChildren(X) returns false, there should not be any subsequent call to ITreeContentProvider.getChildren(X). Cheers Mário
Created attachment 174065 [details] Corrected Snippet (In reply to comment #3) > > You may say this is a "strange requirement", and it might be so, but indeed it > reveals that the ITreeContentProvider contract was broken, that is, if > ITreeContentProvider.hasChildren(X) returns false, there should not be any > subsequent call to ITreeContentProvider.getChildren(X). > JDoc for ITreeContentProvider#hasChildren(Object) : /** * Returns whether the given element has children. * <p> * Intended as an optimization for when the viewer does not * need the actual children. Clients may be able to implement * this more efficiently than <code>getChildren</code>. * </p> * * @param element the element * @return <code>true</code> if the given element has children, * and <code>false</code> if it has no children */ This method is intended for optimization *only*. It is not documented to follow a strict sequence/order of calls. On the other hand one can see that the implementation of MyContentProvider is at fault here. The hasChildren method answers conditionally whereas getChildren method blindly returns all children !! As a side note,I'd like to mention that the condition performed in the MyContentProvider#hasChildren should ideally happen at a model level and thus be reflected in the getChildren method - but you have already mentioned about your special requirement :). Nonetheless, the bug does highlight a potential performance tweak that I had been working on a few days - the updateChildren method could really use some improvement; and I do intend to incorporate your suggestion . Also attaching the correction in your snippet.
Hello, > This method is intended for optimization *only*. It is not documented to > follow a strict sequence/order of calls. I think it should strictly follow the order I suggested: call hasChildren() always before getChildren(). It will not only be more performant, because hasChildren() is generally very cheap, but it will also support "special requirements" like mine ;-) Some notes of mine on the Javadoc: > Intended as an optimization for when the viewer does not need the actual > children. I don't agree with this since it introduces doubt: in which cases is hasChildren() called then? You don't want programmers to waste their time guessing these questions. > Clients may be able to implement this more efficiently than > <code>getChildren</code>. Well, it is ALWAYS cheaper to determine if there are children than to retrieve them: to retrieve the children as an array you will have to know the number of children to allocate the array ... I also run the "corrected snippet" but it also has the same problem I reported with the snippet I wrote: the code you added to MyContentProvider.getElements(...) did not correct my "faulty snippet". > Nonetheless, the bug does highlight a potential performance tweak that I had > been working on a few days - the updateChildren method could really use some > improvement; and I do intend to incorporate your suggestion . Great! Cheers Mário
Marking this as an enhancement.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.
