Community
Participate
Working Groups
Created attachment 177849 [details] test case Follow up from bug 188704. An additional issue has been reported: "It seems there are still remaining problems on eclipse 3.6. I’ve slightly updated the testcase to show that. 1. Start debug session with the breakpoint set at line 223 2. Scroll down in variables view and expand the array9. 3. Scroll up if necessary so you have the top variable an array 4. Step into foo and step out (or just change the debug context by clicking somewhere else in debug view and then change it back) 5. Expected to have at top the same array variable as at step 3"
I have a theory on how this bug occurs: The persisted state contains both the elements that should be expanded and the element that should become the top element. When restoring state, the top index is restored as soon as possible, i.e. when top items parents have been expanded. However in this case, the parent gets expanded but its children are not materialized, so the list is scrolled as far down as it can go which is only to expose the dummy element of the last array. Another example of this bug is when arrays are expanded above the top element. In this case the top element is set correctly at first, but as the children of arrays above the top element get materialized the size of the list changes and the list scrolls the intended top element down so its no longer visible. I think we could fix this by delaying when the top element is set until all the elements above it are expanded. However, this could be tricky because we currently restore expanded state of elements lazily, and I think we would have to change that such that elements which are above the top element are expanded automatically during state restore.
Pawel, Playing a little more with the testcase seems to confirm your theory. However, the second example “when arrays are expanded above the top element”, it seems to reproduce only under additional conditions: the above expanded array should be in initial visible range. This happens probably because viewer requests the top elements that are visible as you point it out in bug 188704, comment #4, problem #2. Otherwise, the top item is restored correctly even in case where there is an above array expended but it wasn’t and it isn’t visible. The array will be expanded lazy when user manually scrolls until array becomes visible. Therefore, wouldn’t be enough that the fix would consist in just delaying when the top element is set until all _visible_ elements above it are expanded? (or, in other words, when all current expand requests are finished, having all children materialized)
Created attachment 177944 [details] testcase2 Test where restore top item works 1) expand array1 2) scroll down until you have at top, let's say array12 3) switch the debug context to trigger restore 4) the top item restored is array12 as expected even the above array1 is expanded
Created attachment 177945 [details] testcase3 Test where it fails: The same steps as above but on updated source whre the x variables are missing.
(In reply to comment #2) > Therefore, wouldn’t be enough that the fix would consist in just delaying when > the top element is set until all _visible_ elements above it are expanded? (or, > in other words, when all current expand requests are finished, having all > children materialized) Yes, I think you're right. This fix could be much simpler than what I proposed.
Created attachment 178029 [details] patch_fix I’ve created a patch with the fix. However, it was not enough to delay the REVEAL until updates are completed. Revealing some elements can trigger expanding some of elements that have just been revealed. Therefore, we have to check one more time, after the new triggered updates are completed, if we have to set again the top index. This happens on the first use case described.
Created attachment 178031 [details] patch2 to pass org.eclipse.debug.ui tests I saw some tests failing from org.eclipse.debug.ui after running with my updates. This new patch addresses those issues.
Thank you for the patch :-) It's a nice and simple solution, though there's a couple of things we should consider: 1) If the view is disposed the listener should clean itself up proactively, rather than relying on guards in the viewer methods. 2) If the user switches quickly between stack frames and the registers view doesn't have time to restore the complete state. The partial restored state needs to be saved back to the memento. The current mechanism to do that relies on the flags being accurate in the restore delta. In this solution, the REVEAL flag can be cleared from the restore delta long before the index is restored. If user switches the context in that time, the top index will be lost. Have you considered adding an additional test to the "if ((delta.getFlags() & IModelDelta.REVEAL) != 0)" statement to avoid clearing the reveal flag until there's no pending and running udpates? Also, I haven't tried writing unit tests to validate the set top index behavior. Would you consider writing such test to ensure that this feature is reliable in the future?
(In reply to comment #8) Thank you for your comments. > 1) If the view is disposed the listener should clean itself up proactively, > rather than relying on guards in the viewer methods. Agree. I’ll update the patch. > 2) If the user switches quickly between stack frames and the registers view > doesn't have time to restore the complete state. The partial restored state > needs to be saved back to the memento. The current mechanism to do that relies > on the flags being accurate in the restore delta. In this solution, the REVEAL > flag can be cleared from the restore delta long before the index is restored. > If user switches the context in that time, the top index will be lost. I think this “set top” pending task should be cancelable to support 1) and 2). I can remove the code clearing the REVEAL flag. It seems that checkIfRestoreComplete() sets to null the pending delta and reports true anyway if the REVEAL flag is the last remaining flag. That’s why the pending delta can be already null by the time setting the top index. I don’t know the reasons behind this, but it might be a problem if cancelation is requested just after the pending delta is set to null and setting the top index is about to begin. > Have you considered adding an additional test to the "if ((delta.getFlags() & > IModelDelta.REVEAL) != 0)" statement to avoid clearing the reveal flag until > there's no pending and running udpates? The flag might be never cleared in this case. As I said above, the pending delta might be already null by this time and restorePendingStateNode() would not be called. > Also, I haven't tried writing unit > tests to validate the set top index behavior. Would you consider writing such > test to ensure that this feature is reliable in the future? Sure. I can try to add a unit test, probably next week.
(In reply to comment #9) > I think this “set top” pending task should be cancelable to support 1) and 2). I agree. This solution is simple enough that its worth seeing if it can satisfy these. > I can remove the code clearing the REVEAL flag. It seems that > checkIfRestoreComplete() sets to null the pending delta and reports true > anyway if the REVEAL flag is the last remaining flag. Unfortuantely I don't precisely remember why I added this exception, but I believe it's because there's a chance that the REVEAL flag is never cleared if the element to be revealed was removed. It may be safe to remove this exception. > Sure. I can try to add a unit test, probably next week. Great, thank you.
Created attachment 179362 [details] unit_test_patch I’ve added three unit tests for setting top index behavior. First just checks if top item is restored correctly for a simple model with non expanded elements. This test should pass with the current code. The second and third tests are checking the two use cases described above in this bug. They should fail without the fix patch.
Created attachment 179364 [details] fix_patch Patch with the fix
I ran the tests with/without the patch on Windows, and they work as expected. I noticed the tests are specific to the JFace viewer (i.e. they don't get run for the virtual viewer). Pawel, do the tests/implementation need to be updated to work on the virtual viewer as well?
The virtual viewer implementation for ITreeModelContentProviderTarget.getTopElementPath() just returns null. That’s why these tests (related to top item) would always fail for virtual viewer.
(In reply to comment #13) > I ran the tests with/without the patch on Windows, and they work as expected. I > noticed the tests are specific to the JFace viewer (i.e. they don't get run for > the virtual viewer). Pawel, do the tests/implementation need to be updated to > work on the virtual viewer as well? I never implemented a "view pane" for the virtual viewer, so setting a top item doesn't really make sense. For now the jface tests should be enough. I also ran the tests, but I'm getting a failure in testRestoreTopTriggersExpand on line: Assert.assertEquals(elements[indexLastElem-1], originalTopPath.getLastSegment()); I think the problem is that the resizing of the view is not complete before the top item is set. I'm not really sure how to test for that though... maybe I'll think of something over the weekend.
Created attachment 179602 [details] unit_test_patch Maybe adding “while(fDisplay.readAndDispatch()) {}” after resizing and before setting top item will help. I’ve updated the patch accordingly. Let me know if it works. I didn’t replicate the failure on my computer even when I ran the test in a loop multiple times.
Created attachment 179603 [details] unit_test_patch Same update for the other two tests.
(In reply to comment #16) > Maybe adding “while(fDisplay.readAndDispatch()) {}” after resizing and before > setting top item will help. Unfortunately this didn't work either. I guess the re-sizing of the window is somehow different on GTK. But I took a different approach, setting the initial window size to 300x100 and this did the trick. I committed the fix and the tests. Thank you very much for both :-)
Already reviewed.
(In reply to comment #18) > > I committed the fix and the tests. Thank you very much for both :-) You're welcome.
