Bug 35586 - [Viewers] JFace tree viewer collapses tree items on refresh
Summary: [Viewers] JFace tree viewer collapses tree items on refresh
Status: RESOLVED DUPLICATE of bug 113675
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 2.1   Edit
Hardware: PC Windows 2000
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Boris Bokowski CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2003-03-24 12:29 EST by Alex Nan CLA
Modified: 2007-06-19 15:21 EDT (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Nan CLA 2003-03-24 12:29:41 EST
I have a JFace tree viewer embedding a tree that has 4 levels of nodes. At 
refresh all the expanded nodes at levels higher than 2 are collapsed and any 
selection on one of these nodes is lost. The problem was found in Eclipse v 
2.1.0 build 200302211557. Note that this problem didn't occur in v2.0.2.
Thank you,
Alexandru P. Nan
Comment 1 Nick Edgar CLA 2003-03-25 22:49:41 EST
For 2.1, a bug was fixed whereby the expanded/collapsed state was being carried 
forward on an item for which the corresponding model element was changed.
For example, when deleting an expanded element, the element that took its place 
would then be expanded.

When you issue the refresh, are the elements that are getting collapsed equal 
to the old elements?  The viewer preserves the expansion state for equal 
elements, but not for unequal elements, even if they end up in the same 
position.
Comment 2 Alex Nan CLA 2003-03-26 11:05:48 EST
The scenario is the following: a filter is applied that is supposed to filter 
out first level nodes which have a property in a certain range of values, to be 
more specific lets say severity, which I denote by s, that could be in 0<=s<30 
low severity range, 30<=s<50 medium or 50<=s<70 high. The tree has the 
following structure: it has N first level nodes, which all have a severity 
property set and that could have children in a depth of 4, so the tree can 
actually have 5 levels of nodes. On refresh the getElements() is returning only 
the first level objects (and their children) which comply to the filter 
criteria (low, medium ,high, all, low or medium etc.)  but in fact the nodes at 
levels > 1 are the same, nothing has changed in their structure, i.e getElements
() on objects in levels >1 is returning the same objects. Every node at levels 
>2 will be for some reason collapsed and as a consequence any selection on 
those nodes will be lost. Hopefully my explanations will help, the same 
scenario used to work in 2.0.2.
Comment 3 Nick Edgar CLA 2003-03-26 15:04:08 EST
When getChildren/getElements returns the elements, are they equal to the 
previous ones?  That is, do your model elements implement equals(Object) and is 
the result true when comparing the element before and after the refresh?

Comment 4 Alex Nan CLA 2003-03-26 20:10:04 EST
No unfortunately, now I realize that all the elements on level 2 are 
dynamically created every time a refresh is called by introspecting the objects 
on level 1 which in fact are not changing. This explains now the strange 
behaviour. What to do? Actually the level 2 nodes contain the same information 
before and after the refresh but I create on every refresh a new wrapper class, 
which is probably not a good ideea. I wrote some code that caches a level 2 
node once it is created, I'm not sure what performance impact this approach 
will have, anyway the nodes are not collapsed anymore! 
Thanks.
Comment 5 Nick Edgar CLA 2003-03-26 23:21:52 EST
I recommend implementing equals(Object) on your model objects that compares 
based on some identifying criteria of the objects (if they're wrappers, you 
could compare the wrapped elements).  Don't compare attributes that may change 
during the lifetime of the object.  This may be cheaper and easier to manage 
than remembering the wrappers in hash tables.

I'll keep this PR open for consideration post 2.1 since I'm not totally 
satisfied with the new viewer behaviour.  Although the original problem with 
expanded state getting passed on to the wrong elements was fixed, there are 
several other cases where this wasn't such a bad thing.  For example, one bad 
side effect of the new behaviour is that if you rename an element, and the name 
is part of the equality criteria, then the viewer has no idea that the user 
still thinks of this as the same element.  So if it was expanded before, after 
a refresh it is collapsed, even though it's still in the same position. 

It might be better to process deleted elements specially (delete the 
corresponding SWT item rather than reusing it so that the expanded state is not 
passed on to the next element), and then just let the expanded state for the 
remaining items to be passed on (as in 2.0).  The problem with this approach is 
that it does not handle reordering of elements, e.g. when changing the sort 
order.
Comment 6 Alex Nan CLA 2003-03-27 15:50:38 EST
OK, I implement equals() for the wrapper but what do I return on getElements
(Object element) for a first level node, because in the current code I'm 
introspecting element and I'm returning a wrapper object for every EMF feature  
of element that satisfies a filter criteria. I'm doing something like this 
elementsList.add(new FeatureNode(Ebject element, EStructuralFeature feature)). 
If I'm caching FeatureNode then on the next refresh I don't have to return a 
new feature wrapper object and I will avoid the collapsing problem on an 
expanded FeatureNode. How is the equals(Object) helping me in my case? My 
problem is to avoid the collapsing of nodes, so I have to provide the same 
object to the viewer. I assume you are using equals to compare the old and new 
objects, or perhaps I did't understand your suggestion.
Thank you.
Comment 7 Nick Edgar CLA 2003-03-27 16:26:21 EST
If FeatureNode does not reimplement equals(Object) then it gets the default 
behaviour which compares object identity.  If you're caching the items, then 
this works fine.  But there is overhead to maintaining the cache (e.g. do you 
let the table grow indefinitely, or do you need to track when items can be 
removed).  Another way of doing it is to implement equals(Object) on 
FeatureNode so that the following returns true:

FeatureNode a = new FeatureNode(element, feature);
FeatureNode b = new FeatureNode(element, feature);
a.equals(b)

So even if you're recreating the FeatureNode wrapper each time, the elements 
are still equal as long as the underlying element and feature are equal.

Note that you'd also have to implement hashCode() accordingly.
Comment 8 Alex Nan CLA 2003-04-10 15:12:40 EDT
I followed your tip but unfortunately I still have the problem, this time it 
happens only in the following scenario:
- suppose I have node A, B and C which all have subnodes and I expand A and C 
to their second level children and filter out B, after refreshing the view the 
subnodes of A are still expanded but not those of C although the subnodes of C 
are the same and equals(Object elem)) returns true for them.
The only change is that the second time getElements() is returning only A and 
C, so C has changed his place and moved upwards.
Comment 9 Alex Nan CLA 2003-04-10 15:18:53 EDT
I've also noticed the fact that the viewer is slow on refresh operations,  I 
profiled  a refresh for a tree that had 2048 first level nodes.
It seems that AbstractTreeViewer.updateChildren() takes a lot of time compared 
to other methods and also AbstractTreeViewer.isExpandable() is called twice for 
every node, which makes the hasChildren() method in the ContentProvider to be 
called the same nr. of times. As a consequence if hasChildren() is doing a bit 
of work this could slow the viewer even more.
Comment 10 Nick Edgar CLA 2003-04-14 09:33:47 EDT
For the case in comment #8, it's important that equals returns true for C, when 
comparing the old and new object representing C.
If it already does this, then I'm not sure what's going on.  You'd have to set 
a breakpoint and trace through updateChildren, which tries to preserve the 
expansion state.

In a profile, updateChildren will take most of the time, since it does most of 
the work.  There are some inefficiencies due to the way the code is structured, 
so hasChildren will get called twice for each element.  Note that you can 
implement hasChildren in such a way that it returns true if that kind of 
element ever has children; you don't need to check whether it actually has 
children.  For example, it could always return true for an object representing 
a folder.
Comment 11 Nick Edgar CLA 2006-03-15 11:47:39 EST
Reassigning bugs in component areas that are changing ownership.
Comment 12 Boris Bokowski CLA 2007-06-19 15:21:50 EDT

*** This bug has been marked as a duplicate of bug 113675 ***