Summary: | [Viewers] Treeviewer fails to call hasChildren() before getChildren() | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Eclipse Project] Platform | Reporter: | Mário Guimarães <mljrg> | ||||||||
Component: | UI | Assignee: | Platform UI Triaged <platform-ui-triaged> | ||||||||
Status: | NEW --- | QA Contact: | |||||||||
Severity: | enhancement | ||||||||||
Priority: | P3 | CC: | eclipse.felipe | ||||||||
Version: | 4.0 | ||||||||||
Target Milestone: | --- | ||||||||||
Hardware: | PC | ||||||||||
OS: | Windows Vista | ||||||||||
Whiteboard: | |||||||||||
Attachments: |
|
Description
Mário Guimarães
2010-04-22 09:15:04 EDT
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. |