Bug 170521 - [Viewers] Refreshing ContainerCheckedTreeViewer does not restore checked/grayed states correctly.
Summary: [Viewers] Refreshing ContainerCheckedTreeViewer does not restore checked/gray...
Status: CLOSED WONTFIX
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.2.1   Edit
Hardware: PC Windows XP
: P3 major with 5 votes (vote)
Target Milestone: ---   Edit
Assignee: Platform UI Triaged CLA
QA Contact:
URL:
Whiteboard: stalebug
Keywords: helpwanted
Depends on:
Blocks:
 
Reported: 2007-01-15 14:44 EST by orders.lightfoot CLA
Modified: 2019-11-14 03:27 EST (History)
8 users (show)

See Also:


Attachments
ContainerCheckedTreeViewer Refresh Problem Demonstrated (5.80 KB, application/zip)
2007-01-15 14:46 EST, orders.lightfoot CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description orders.lightfoot CLA 2007-01-15 14:44:39 EST
I have a ContainerCheckedTreeViewer that contains root nodes which each have up to two sub nodes.  Those sub nodes can then have multiple leaf nodes.

The ContainerCheckedTreeViewer also has a filter which only displays the root nodes (and their associated children) that correspond to the checked element(s) in an additional table.

Each time an element is checked or unchecked in the table, the ContainerCheckedTreeViewer is refreshed so that its filter can be reapplied.

The problem occurs when I have a node that is in a grayed state and  the call to refresh (triggered by a check state change in the table) causes a node to be added or removed above the the grayed node in the ContainerCheckedTreeViewer.

When this refresh happens, the grayed states of the nodes are restored correctly, but the previously checked child nodes are all unchecked.  To make matters more strange, when debugging this issue, it appears that in the code, all of the children nodes are in a checked state, even though they are unchecked in the GUI.

I have attached a small standalone SWT application that demonstrates this problem.

To illustrate the problem, run CheckboxTreeRefreshProblem.java as an SWT application.  When the program opens, you will see a check box table on the left that list  3 root node names.

When you check a row in the table, the associated root node should appear in the ContainerCheckedTreeViewer on the right.  Unchecking a row in the table should cause the associated root node to be filtered out of the tree.

1. Check the row for "RootNode 2" in the table.  RootNode 2 should now be visible in the tree.

2. Expand RootNode 2 completely and check LeafNode 2 and LeafNode 3 in the tree.  The parent nodes should now be grayed and the value columns for the parent nodes should be updated to show the  sum of the values of the checked child nodes.

3. Check "RootNode 1" in the table.  This should cause RootNode 1 to be displaye in the tree above RootNode 2.  This is where the problem occurs.  RootNode 2 and its sub node are grayed just like they should be, but the child nodes that were previously checked are no longer checked.

4. To force another refresh, uncheck "RootNode 1" in the table.  This should cause RootNode 1 to be filtered out of the tree and should also refresh the lables of the remaining nodes in the tree.  The value columns for RootNode 2 and its sub node now indicate that all of their children are checked which can be verified in the code via debugging, but the GUI shows that no children are checked.

In an effort to ensure that all of the TreeItems have been physically created and to try to force restoring the checked and grayed states, I tried to override the refresh method of the  tree viewer to do the following:

-turn off redraw
-get the currently expanded elements
-get the currently checked elements
-get the currently grayed elements
-expand all elements
-call super.refresh()
-expand all elements again in case new items were added by refresh
-collapse all elements
-restore the previously expanded elements by calling setExpandedElements
-try to restore the previously checked elements by calling setCheckedElements
-try to restore the previously grayed elements by calling setGrayedElements
-turn redraw back on to force a redraw

The problem with this is that grayed elements are also considered checked so when I call setCheckedElements with my array of previously checked elements, my previously grayed elements are now also marked as checked which then causes all of their children to be marked as checked.  Now, when setGrayedElements is called with the array of previously grayed elements, the grayed nodes are displayed correctly but their children are all checked because of the previous call to setCheckedElements.  A similar but related problem occurs when the order of the calls to setCheckedElements and setGrayedElements are reversed.
Comment 1 orders.lightfoot CLA 2007-01-15 14:46:16 EST
Created attachment 56924 [details]
ContainerCheckedTreeViewer Refresh Problem Demonstrated
Comment 2 Boris Bokowski CLA 2007-04-23 14:45:52 EDT
Sorry, but I won't have time for this in 3.3.
Comment 3 Moritz Kreutzer CLA 2008-04-07 07:52:22 EDT
Are there any efforts for fixing this in 3.4?
Comment 4 Boris Bokowski CLA 2008-04-07 13:05:49 EDT
Would you be able to contribute a patch?
Comment 5 Moritz Kreutzer CLA 2008-04-08 04:24:05 EDT
I don't think so.. But anyway, I could have a look.
Comment 6 Susan McCourt CLA 2008-05-07 23:56:53 EDT
for what it's worth, I can see why this happens, but I don't know the tree viewer code well enough to propose a solution.

I believe the problem to be in CheckboxTreeViewer.applyState(...)
The code that records the checked state uses the elements to remember what checkmarks should be restored, and then attempts to traverse the tree items and reset their check/gray data if their element was remembered in the saved state.  But non-expanded elements are typically dummy items that to not have their element set into their data.  So their check state is never restored.

   private void applyState(CustomHashtable checked, CustomHashtable grayed,
            Widget widget) {
        Item[] items = getChildren(widget);
        for (int i = 0; i < items.length; i++) {
            Item item = items[i];
            if (item instanceof TreeItem) {
                Object data = item.getData();
>>>               if (data != null) {
                    TreeItem ti = (TreeItem) item;
                    ti.setChecked(checked.containsKey(data));
                    ti.setGrayed(grayed.containsKey(data));
                }
            }
            applyState(checked, grayed, item);
        }
    }

I attempted to force the children to get updated using updateChildren(Widget widget, Object parent, Object[] elementChildren) but this didn't seem to help either.

I really don't know what I'm doing at this point but I believe this is part of the problem.  I will probably work around this problem by recording the check state myself and then resetting them.
Comment 7 Susan McCourt CLA 2008-05-13 12:05:34 EDT
See bug #207064.
PDE implemented FilteredCheckboxTree to solve many of these problems.  We might consider adopting this in platform UI.  It has some assumptions/limitations but even if it only works for certain kinds of models, it would be useful to have.
Comment 8 Joe Cox CLA 2008-05-20 06:11:34 EDT
In Class AbstractTreeViewer. There seems to be the method that throws away all the data Objects. This happens when you collapse the Tree for example.

I am not familiar with the TreeView code but this make no sense to me.
Is there a way to turn this off by flag?

	private void updateChildren(Widget widget, Object parent,
			Object[] elementChildren, boolean updateLabels) {
		// optimization! prune collapsed subtrees
		if (widget instanceof Item) {
			Item ti = (Item) widget;
			if (!getExpanded(ti)) {
				// need a dummy node if element is expandable;
				// but try to avoid recreating the dummy node
				boolean needDummy = isExpandable(ti, null, parent);
				boolean haveDummy = false;
				// remove all children
				Item[] items = getItems(ti);
				for (int i = 0; i < items.length; i++) {
					if (items[i].getData() != null) {
						disassociate(items[i]);
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>		[[[items]]][i].dispose();
					} else {
						if (needDummy && !haveDummy) {
							haveDummy = true;
						} else {
							items[i].dispose();
						}
					}
				}
				if (needDummy && !haveDummy) {
					newItem(ti, SWT.NULL, -1);
				}

				return;
			}
		}
Comment 9 Boris Bokowski CLA 2008-05-20 08:55:01 EDT
(In reply to comment #8)
> I am not familiar with the TreeView code but this make no sense to me.
> Is there a way to turn this off by flag?

No, I don't think you can turn this off. When the tree is refreshed, we prune collapsed subtrees. How does this make no sense?
Comment 10 Joe Cox CLA 2008-05-20 09:48:19 EDT
(In reply to comment #9)
> (In reply to comment #8)
> > I am not familiar with the TreeView code but this make no sense to me.
> > Is there a way to turn this off by flag?
> 
> No, I don't think you can turn this off. When the tree is refreshed, we prune
> collapsed subtrees. How does this make no sense?
> 

When i add an item to a collapsed tree (not to the root), i have to do a refresh, so the added item show up. After this refresh all collapsed items loose their check - flags. I thought that the data Object become removed after the collapse action. And because this, the check - flags does not become restored correctly.

Did i miss something?
Comment 11 Boris Bokowski CLA 2008-05-20 09:57:27 EDT
(In reply to comment #10)
> When i add an item to a collapsed tree (not to the root), i have to do a
> refresh, so the added item show up.

Can you not call AbstractTreeViewer.add(Object parent, Object element) instead of refresh?
Comment 12 Susan McCourt CLA 2008-05-20 12:11:28 EDT
The problem for me was that filtered tree calls refresh.  
Comment 13 Joe Cox CLA 2008-05-21 11:14:34 EDT
(In reply to comment #11)
> (In reply to comment #10)
> > When i add an item to a collapsed tree (not to the root), i have to do a
> > refresh, so the added item show up.
> 
> Can you not call AbstractTreeViewer.add(Object parent, Object element) instead
> of refresh?
> 

I've tied this also. The new item do not show up with the AbstractTreeViewer.add( method. Whatever i do, seems i always need to call refresh in order to see the new items.
Comment 14 Boris Bokowski CLA 2009-03-20 22:32:49 EDT
Would using an ICheckStateProvider (new API in 3.5) help with this?
Comment 15 Boris Bokowski CLA 2009-11-26 09:53:31 EST
Hitesh is now responsible for watching bugs in the [Viewers] component area.
Comment 16 Xavier Raynaud CLA 2011-01-04 09:57:02 EST
I had the same issue in gprof viewer in linux tools project.
For info, you can find here how I workaround this issue:

https://dev.eclipse.org/svnroot/technology/org.eclipse.linuxtools/gprof/trunk/org.eclipse.linuxtools.dataviewers/src/org/eclipse/linuxtools/dataviewers/abstractviewers/AbstractSTTreeViewer.java

Perhaps it can help ?
Comment 17 Jay Norwood CLA 2011-09-15 15:16:08 EDT
(In reply to comment #14)
> Would using an ICheckStateProvider (new API in 3.5) help with this?

I did just that, but ended up re-writing the treeViewer anyway.   My leafnode data was from an emf model, and I found it much faster to update all the tree nodes based on the subtree of leaf node data provided by the emf model.

The problem I see with CheckboxTreeViewer is the code below from doUpdateItem 

	setChecked(element, checkStateProvider.isChecked(element));
	setGrayed(element, checkStateProvider.isGrayed(element));

After experimenting in a new viewer I found that using the following code consistently will update the tree checked and grey state correctly in the gui on Windows.  To explain, I found that if you set the gray state true, you also have to set checked to true.  Otherwise you have to set gray state to false and the checked value to the chkstate.  The above code from doUpdateItem sets checked and grayed separately,  and I believe the ContainerCheckedTreeViewer even sets grayed to false within its setChecked.  All that was unreliable, at least on Windows.  

    public boolean setGrayChecked(Object element, boolean chkState, boolean grState) {
        Assert.isNotNull(element);
        Widget widget = internalExpand(element, false);
        if (widget instanceof TreeItem) {
            TreeItem item = (TreeItem) widget;
            if (grState){
             	item.setChecked(true);
            	item.setGrayed(true);
            }
            else{
            	item.setGrayed(false);
             	item.setChecked(chkState);
            }
            return true;
        }
        return false;
    }
Comment 18 Jay Norwood CLA 2011-09-17 11:38:44 EDT
(In reply to comment #17)
 
So, looking at the ContainerCheckedTreeViewer code again, I decided to try to use it and change my checkStateProvider code to always return isChecked true if isGrayed is true, i.e. when some of the children are checked and some not. That seems to fix the update of the grayed state.    

	@Override
	public boolean isChecked(Object element) {
		if (isGrayed(element)){
			return true;
		}
                else ...
Comment 19 Lars Vogel CLA 2019-11-14 03:27:45 EST
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.

If the bug is still relevant, please remove the "stalebug" whiteboard tag.