Comment 1 Darin Wright 2010-10-04 09:25:04 EDT

Looks like the fix to bug 324100 is causing this.

Comment 2 Pawel Piech 2010-10-04 18:33:06 EDT

Created attachment 180217 [details]
Patch with test to reproduce the problem.

I had a theory for what causes the assertion.  So I wrote a test to reproduce it.  I reproduced it, then tried my fix.  But my fix didn't work :-(

Comment 3 Pawel Piech 2010-10-04 18:34:54 EDT

Dorin, do you see a reason for this assertion failure?

Comment 4 Dorin Ciuca 2010-10-05 09:16:18 EDT

Unfortunately, I can’t reproduce it.  I ran your new test and I got some NPEs in TestModelUpdatesListener  at if (event.getJob().getResult().getSeverity() == IStatus.ERROR) {
Updating the code to guard against null results, the test passes (without the fix).
e.g.
IStatus result = event.getJob().getResult(); 
if (result != null && result.getSeverity() == IStatus.ERROR) {
    fJobError = result;   
}
I think setting fPendingSetTopItem = null; (ModelContentProvider.java) just after PendingSetTopItem.dispose(); is redundant. fPendingSetTopItem is already set to null by dispose().

Comment 5 Pawel Piech 2010-10-05 11:26:57 EDT

Created attachment 180248 [details]
Failure trace.

(In reply to comment #4)
> Unfortunately, I can’t reproduce it.  I ran your new test and I got some NPEs
> ...

Yes, the NPEs should be guarded.  But either way I get the attached failure.  Maybe it's timing, platform specific.

Comment 6 Dorin Ciuca 2010-10-05 16:37:56 EDT

Created attachment 180286 [details]
quick fix

I reproduced it on another PC at home. I’ve added some more tracing and I was able to see the problem. It seems that it was there also before the fix for bug 324100, but there was no assert to show it.
The parent of the REVEAL node is also revealed if the REVEAL node has model index -1. See lines 620-633 in TreeModelContentProvider. 

A quick fix would be as attached. But I guess that condition (modelIndex >= 0) was put for a reason. Do you know what was the reason?

Comment 7 Pawel Piech 2010-10-05 18:20:47 EDT

(In reply to comment #6)
> A quick fix would be as attached. But I guess that condition (modelIndex >= 0)
> was put for a reason. Do you know what was the reason?

I came to the same conclusion recently and I was going to ask you the same question :-)  I'll have to go back and figure it out.

Comment 8 Pawel Piech 2010-10-06 17:41:51 EDT

Created attachment 180377 [details]
Fix.

Following this index problem led me to discover another major bug in the top index save/restore logic:  the TreeModelContentProvider.buildViewerState() which is called to save the current top index item into a delta is called by IContentProvider.inputChanged() implementation.  However, by the time inputChanged() is called, the StructuredViewer.setInput() has called unmapAllElements().  Because of this, the buildViewerState() cannot calculate the element indexes.  So the element with the REVEAL flag is saved without an index.  For restoring the top index in deep trees, this is a major problem.

This patch addresses this problem, but it contains an ugly workaround: StructuredViewer.setInput() is declared as final so we don't have a clean way to intercept client's call to change input.  Instead, I had to override unmapAllElements() and call super.unmapAllElements() after calling a new method ITreeModelContentProvider.aboutToChangeInput().

Comment 9 Pawel Piech 2010-10-06 17:43:18 EDT

Darin and Dorin, please review this patch.  If you can think of a cleaner way to override setInput() it would really help.

Comment 10 Dorin Ciuca 2010-10-07 07:49:15 EDT

(In reply to comment #8)
> This patch addresses this problem, but it contains an ugly workaround:
> StructuredViewer.setInput() is declared as final so we don't have a clean way
> to intercept client's call to change input.  Instead, I had to override
> unmapAllElements() and call super.unmapAllElements() after calling a new method
> ITreeModelContentProvider.aboutToChangeInput().

What do you think about calling contentProvider.inputAboutToChange()  in VariablesView.setViewerInput() just above viewer.setInput(context) line?

Comment 11 Dorin Ciuca 2010-10-07 08:56:07 EDT

(In reply to comment #9)
> Darin and Dorin, please review this patch.  If you can think of a cleaner way
> to override setInput() it would really help.

Pawel, see my comments below:
1)  ModelContentProvider.cancelRestore()
  You remove disposing the pending reveal. Wouldn’t be a problem in TreeModelContentProvider.handleReveal(IModelDelta) where you have 
reveal(delta); // reveal the requested node
cancelRestore(getViewerTreePath(delta), IModelDelta.REVEAL); // leaves pending reveal
...
pending reveal reveals another node

2) InternalVirtualTreeModelViewer.setInput(Object)
You can rewrite the two lines:
Object oldInput = getInput();
getContentProvider().inputAboutToChange(this, oldInput, input);

Comment 12 Pawel Piech 2010-10-07 16:27:05 EDT

(In reply to comment #11)
> (In reply to comment #9)
> 1)  ModelContentProvider.cancelRestore()
>   You remove disposing the pending reveal. Wouldn’t be a problem in
> TreeModelContentProvider.handleReveal(IModelDelta) where you have 
> reveal(delta); // reveal the requested node
> cancelRestore(getViewerTreePath(delta), IModelDelta.REVEAL); // leaves pending
> reveal
> ...
> pending reveal reveals another node
You're correct.  I made a lot of changes then restored things from history and I missed this one.  An interesting question is whether we should write a test to catch this.

> 
> 2) InternalVirtualTreeModelViewer.setInput(Object)
> You can rewrite the two lines:
> Object oldInput = getInput();
> getContentProvider().inputAboutToChange(this, oldInput, input);


(In reply to comment #10)
> What do you think about calling contentProvider.inputAboutToChange()  in
> VariablesView.setViewerInput() just above viewer.setInput(context) line?

It would complicate the API for using the viewer, it's also not ideal.

Comment 13 Dorin Ciuca 2010-10-08 03:34:42 EDT

(In reply to comment #5)
> (In reply to comment #4)
> > Unfortunately, I can’t reproduce it.  I ran your new test and I got some NPEs
> > ...
> 
> Yes, the NPEs should be guarded.  But either way I get the attached failure. 
> Maybe it's timing, platform specific.
I found why your test passed from the very beginning in that debug environment. It seems that I had org.eclipse.jface project checked out in workspace and the two lines from StructuredViewer.setInput(Object) where switched like  this
super.setInput(input);
unmapAllElements();

Comment 14 Dani Megert 2010-10-08 04:52:49 EDT

This is bugging me. Can we get a fix soon or revert the code so that the next I-build works?

Comment 15 Dorin Ciuca 2010-10-08 08:57:32 EDT

Created attachment 180484 [details]
Unit test cancel restore

(In reply to comment #12)
> I missed this one.  An interesting question is whether we should write a test
> to catch this.
> 
I wrote a test for this. 
I updated also the model update listener. I added a method to add state updates for a delta (the state delta saved from view).

Comment 16 Dorin Ciuca 2010-10-08 10:13:46 EDT

(In reply to comment #14)
> This is bugging me. Can we get a fix soon or revert the code so that the next
> I-build works?

I think it’s safe to just replace the assert with

if (fPendingSetTopItem != null) {
	fPendingSetTopItem.dispose();
}
for the moment, if you want to search for a cleaner solution of the root cause of this bug.

Comment 17 Pawel Piech 2010-10-08 11:49:09 EDT

(In reply to comment #14)
> This is bugging me. Can we get a fix soon or revert the code so that the next
> I-build works?

Good point.  I committed the fix.  Darin please review.

Comment 18 Dani Megert 2010-10-26 06:34:21 EDT

> Good point.  I committed the fix.  Darin please review.
Ping ;-)

Comment 19 Darin Wright 2010-10-26 15:02:59 EDT

Looks good.