Community
Participate
Working Groups
Currently, IStructuredContentProvider does not restrict the output of getElements(Object input). However, the following code produces a bug: public class MyContentProvider implements TreeContentProvider { ... public Object[] getElements(Object input) { return new Object[] {input}; } } If MyContentProvider is supplied to a TreeViewer, the result is an infinite tree. (The node representing 'input' has one child, which is a node representing 'input', and this situation recurses forever.) It seems that a comparison is being made somewhere so that if a node is equal to the input, getElements is called, otherwise, getChildren is called. So, the array returned by getElements cannot contain the input object. Either the spec on IStructuredContentProvider should be changed to restrict the returned array, or the logic in TreeViewer (and any other viewer with this problem) that determines whether an object is the root or a child should be made more general.
*** Bug 38027 has been marked as a duplicate of this bug. ***
The problem is that AbstractTreeViewer#createChildren(Widget) confuses the input element and the root element by calling #getSortedChildren(Object) -> getFilteredChildren(Object) -> #getRawChildren(Object) on the input element as well as on the tree elements. Therefore, #getRawChildren(Object) cannot decide any more whether it was called on the input element or on a tree element. The solution would be to create another set of methods such as get[Sorted|Filtered|Raw]TreeChildren(Object), which are called on tree elements, but not on the input, and whose getRawTreeChildren(Object) would use ITreeContentProvider#getChildren(Object).
This is part of the element multi times in a tree problem
Created attachment 27565 [details] Fix (In reply to comment #3) > This is part of the element multi times in a tree problem Not really - this one is easy to solve. The input element is never shown in the tree, so there's no problem with hash maps, finding the right parent, etc. As explained in bug 38027 comment 4 and in comment 2, the problem is only that if (equals(parent, getRoot())) return super.getRawChildren(parent); in AbstractTreeViewer#getRawChildren(..) is not the best way to decide whether we are fetching the root's children or another element's children. The attached patch fixes the problem in a backwards-compatible way (in contrast to the proposed solution of comment 2, which could break clients that have overridden one of the #get[Sorted|Filtered|Raw]Children(..) methods).
Created attachment 27566 [details] tiny sample plugin that shows the problem The view should show ... root + ch1 + ch2 + ch21 + ch22 + ch23 ... but it shows ... root + root + root + root ...
This patch breaks our ImportExistingTest - it is possible to have a tree with a non visible root that appears as a list of elements (this is how the ResourceAndTreeGroup works). This is working as designed. Any change to the ability to get the top level list will break the tree.
*** Bug 78359 has been marked as a duplicate of this bug. ***
I think it's inappropriate to resolve this bug as INVALID. Essentially this bug is complaining that getElement's implementation and documentation do not match each other. At the time I filed it, it seemed to me that the implementation was wrong. It seems the existing implementation has been deemed correct--which is fine--but there is no mention in the bug comments about updating the documentation. I haven't looked at the Eclipse source code for a long time, so perhaps this comment is redundant. Sorry for the spam if that's the case. However, if neither the documentation nor the implementation for getElements has changed, then it seems to me the doc should be updated to reflect reality.
*** Bug 224955 has been marked as a duplicate of this bug. ***
Committed in >20080303. I've updated the JDoc for IStructuredContentProvider to document this restriction.
(In reply to comment #10) > Committed in >20080303. > I've updated the JDoc for IStructuredContentProvider to document this > restriction. Thank you for updating the documentation. I suspected that perhaps an invisible root was required, but nothing spelled that out. Yes, I find it awkward, but I understand why it would be difficult for you to change at this point. Just make sure to let JFace developers know about these quirks in the documentation, wherever they exist. :) Thanks for looking into it. BTW, the snippet you quoted from the article (in the duplicate bug) is wrong. It says getElements() is only called in response to setInput(), but we know that's not true--it's also called instead of getChildren() for the inputElement. You might want to update that, too.
> I've updated the JDoc for IStructuredContentProvider to document this > restriction. * <p> * <b>NOTE:</b> For instances where the viewer is displaying a tree * containing a single 'root' element it is still necessary that the * 'input' does not return <i>itself</i> from this method. This leads * to recursion issues (see bug 9262). * </p> * @param inputElement the input element * @return the array of elements to display in the viewer */ public Object[] getElements(Object inputElement); Isn't this the wrong place to document this? This problem only applies to tree viewers, and not to other viewers (like TableViewer) that also use IStructuredContentProvider. I would add the note to ITreeContentProvider#getElements(Object inputElement) and ITreePathContentProvider#getElements(Object inputElement) or to AbstractTreeViewer (whose implementation causes the problem in the first place). Furthermore, the note above is not really correct. In fact, it works if getElement(..) returns the 'inputElement' object itself (cast to Object[]). The note should say: "The returned array must not contain the given inputElement, since this leads to recursion issues (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=9262)".
This should be an extremely easy thing to fix! How come this has 7 years?!
Back to inbox for the final fix (Javadoc only).
Hitesh is now responsible for watching bugs in the [Viewers] component area.
Created attachment 166513 [details] better Javadoc fix Here's a Javadoc fix that corrects the note and moves it to the interfaces that are affected.
Need a +1 from another committer to release for RC1.
+1
Released "better Javadoc fix" to HEAD.
In IStructuredContentProvider I would add an "Otherwise" or ", since": This leads to recursion issues (see bug 9262). ==> Otherwise this...
(In reply to comment #20) > In IStructuredContentProvider I guess you got tricked by the patch preview. That note in IStructuredContentProvider has been removed completely, since it doesn't apply to IStructuredContentProviders in general (besides that it was wrong).
>I guess you got tricked by the patch preview. Indeed. All good in HEAD.
Verified in I20100516-0800.
*** Bug 126276 has been marked as a duplicate of this bug. ***
I'm unsure whether this issue is marked "FIXED" because it's really fixed or a javadoc is just updated to warn about do not returning input element from getElements().... Thanks for clarification.
(In reply to comment #25) > I'm unsure whether this issue is marked "FIXED" because it's really fixed or > a javadoc is just updated to warn about do not returning input element from > getElements().... > > Thanks for clarification. The limitation only got documented and no code fix was made.
First I want to thank that this issue is documented in the Javadoc, however, I'm not sure whether I understand the bug thread. TreeViewer is not able to show the root element of a model because instead this method requires to pass in the children, again because it cannot deal with some recursion over the first element. The functional limitation: TreeViewer cannot handle the first root element; whatever the cause is. Reading this bug it seems supposed to be "fixed". But since the limitation is still there, I understand this is more like a "won't be solved". Notice, it is making all people around the globe having to create a dummy root (depending on the model classes, can be up to extremely difficult). Wouldn't just be easier to do it once internally in the JFace library? The fastest solution would be only to check whether it is first time the recursion is called; this might not be very efficient, but we are not speaking about millions of elements because we are dealing with a GUI Tree, so I consider this a minor issue. Again, at least was great to find that this in the documentation, so that partial "fix" was a great idea. It could have driven me crazy if it wasn't there. For me that is the "work around" kind of solution, and the real issue is still to be addressed. I am completely new to eclipse but I would do it myself as soon as I get somehow familiar with the code. I hope this vision from outside, the user, helps.
I've changed the bug resolution to WONTFIX, since the problem indeed still exists. Unfortunately, we can't fix this now, because a fix could break existing clients (e.g comment 6).
Yes, I understand that current encapsulation doesn't allow to chose what method to call and therefore. In case it helps, this is what I did. Notice that I realized about the different roles of getElements, getChildren and its relation with setInput. Also that there is no problem about recursion since the input element is the model object which is supposed to be more than just the root node. In the content provider (within MyTreeViewer): /*** getElements * It basically receives a whole model object and from within takes the * element which is going to be the root, and returns an array containing * it. */ @Override public Object[] getElements(Object inputElement) { MyModel = (MyModel) inputElement; Object root = MyModel.getRootElement(); // Object[] aroot = new Object[]{ root }; return aroot; }//- getElements /*** getChildren * is a "normal" getChildren */ @Override public Object[] getChildren(Object parentElement) { ModelNode node = (ModelNode) parentElement; // depends on the //model class return node.getChildren().toArray(); // getChildren, depends on //the model class }//- getChildren Wherever I am setting the model element: MyModelElement mymodelelement = ...; // whatever initialization mytreeviewer.setInput( mymodelelement ); IDK whether is clear enough. :P In summary, only consider that getElements is not a getChildren and can be passed in the whole model object, not only the root of the tree. I think this is an acceptable assumtion: the model is something more than a tree but also some information. The model contains the root though. ------------ On the other hand maybe this could be solved internally having new methods "setInput2" calling "setElements2()" and in setElements2 the problem of the recursion/encapsulation is solved. That would keep old clients working, but new ones could use setInput2. (change the "2" for a meaningful word) :)
