Community
Participate
Working Groups
This bug is a spin-off of Bug 54559. The workaround to that bug no longer works because of the following bug. This is a major/critical issue for the debugger because it breaks our automatic view management story. It looks like the behavior of IWorkbenchPage#findView has changed such that the workaround we're using for partClosed() is no longer working. As suggested, we're currently listening to partHidden() and then calling findView() to see if the view has actually been closed. IWorkbenchPage#findView() used to return null if the view wasn't open in the current perspective. But now I get a non-null value when I call findView(...) in the viewHidden() callback when a view is "closed" (really closed, not just closed for the last time). This workaround failure was reported as Bug 58591 in the debugger. To reproduce this bug, put a breakpoint in the partHidden() method of LaunchViewContextListener, then close a view in the Debug Perspective (or any perspective where the Debug View has been initialized). findView() will return a non-null value for the view that was just closed.
*** Bug 58591 has been marked as a duplicate of this bug. ***
This was apparently introduced by some changes in the presentations code. In PartTabFolder.remove(LayoutPart), it was calling presentation.removePart before removing it from its own list of children. The presentation was calling setVisible(false) on the presentable part, which then triggered the partHidden notification. When it went to do a findView, it still found the view. I've released the fix. Stefan, can you please verify?
Added regression test for this in IPartServiceTest.
Reassigning to Stefan to verify the change. It seems suspicious that we're relying on the presentation's call to setVisible to send the property change event that triggers the partHidden notification. I think the Workbench should be fully responsible for part lifecycle and such notifications, not the presentation.
Actually, I suppose it's OK to send the visibility property change if the presentation does call part.setVisible(false), but we should not rely on it doing so. We need to ensure that the lifecycle on close is: partDeactivated, partHidden, partClosed, regardless of whether the presentation sets it as invisible. I'm thinking of bug 56147, where, if fixed, the presentation may always keep parts visible. But when it's closing, the workbench should still send the visibility property change and partHidden notifications.
This bug seems to have corrected itself. I'm getting null back from findView in your partHidden method. Can you comment on whether you believe this is now closed?
Not sure who the question is directed to. If it's to me, see Nick's comments 2-5. :)
Correct, the bug has been fixed, but I had some questions for Stefan above.
Sorry... my wires are crossed. Reassigning back to Stefan. More coffee for Kim.
This bug is back. I just wrote new code on the 200405131200 build which is seeing the again. I added the DisplayView as an IPartListener2 with the following partHidden(...) method: public void partHidden(IWorkbenchPartReference partRef) { if (partRef instanceof IViewReference) { String id = ((IViewReference) partRef).getId(); // partHidden is sent whenever the view is made not // visible. To tell that the view has been "closed", // try to find it. if (id.equals(getViewSite().getId())) { IWorkbenchWindow window = DebugUIPlugin.getActiveWorkbenchWindow(); if (window != null) { IWorkbenchPage activePage = window.getActivePage(); if (activePage != null && activePage.findView(id) == null) { // Display view closed tempMemento= XMLMemento.createWriteRoot("DisplayViewMemento"); //$NON-NLS-1$ saveState(tempMemento); } } } } } activePage.findView(id) is returning the DisplayView, even though it was just closed for the last time.
Strange, I have a regression test in our suite for exactly this case, which passed in this build. I just stepped through the test and all seems well. Are you sure the page you're getting is the same one as where the part was closed? If so, would you be able to narrow it down a bit?
If you want to see the problem for yourself, put a breakpoint in DisplayView#partHidden(...) (latest from HEAD), close the Display view, and then evaluate: getSite().getWorkbenchWindow().getActivePage().findView(id) You'll get the Display view back instead of null. This case fails every time.
In what plugin can I find DisplayView?
Oops. Nevermind.
I just tried reproducing the bug in Display view and I cannot. I'm getting null back from findView(id). Are you closing this view through the API or by clicking on the close button?
I just tried this for I200405171219 and it does indeed return null for the "Display" view after it's closed.
Sorry. You need to have the Display view still open in another perspective. So the steps are: 1. Open the Display view in the Debug perspective. 2. Go to the Java perspective and open the Display view. 3. Close the Display view in the Java persepective. 4. IWorkbenchPage#findView(id) returns non-null. As an aside, I recommend using a watch expression (Expression view->Add Watch Expression) with the string: getSite().getWorkbenchWindow().getActivePage().findView(id) so you don't have to keep manually performing evaluations.
I'm still getting null even with the more specific steps you list. Could you try this against todays warmup build?
The call to getActivePage() can be avoided using: getSite().getPage().findView(id)
I still get null as well using Build id: 200405171219 and the steps from comment #17.
I'm using 200405171219 on Linux-GTK. Setting up a Win2k machine for myself now...
I can't reproduce on Win2k. Interesting...
I am sorry. I should have specified. I used Windows XP to test. I will test on Linux-GTK as well.
Arg. There must be something more to the steps. When I posted those steps, they were working fine for me. A half hour later, having made no changes to my setup, they no longer work. I'm now seeing null on Linux too. There must be a step I'm missing (something I've apparently been doing less and less...). I'll poke around some more.
Ok... The multiple perspectives was a red herring. All you have to do is hide the view first. If you then show it and close it, you get a non-null value from findView(...). I guess the reason I was seeing it less and less was because I originally getting it during normal use (during which I'd show and hide the view) and then switched to just trying to reproduce the bug. Consistent steps: 1. Launch Eclipse. 2. Open the Display view. 3. Bring some other view on top of the Display view (I was using the Error Log view). 4. Bring the Display view to the top again. 5. Close the Display view.
I was able to reproduce this bug using your steps from comment #25. You are correct. For this particular case, it does not return null even though the view has been closed. I will investigate. I am running on Windows XP and self-hosting using "org.eclipse.ui.workbench" and "org.eclipse.debug.ui" from CVS.
FYI, if you're running an integration build from this week you shouldn't need the debug plugin from CVS. The Display view's in JDT Debug UI anyway. ;)
Wow, it only took you 5 minutes to set up a Win2K machine?!? I'm impressed!
What's happening here is that if view A is on top with B underneath, then it brings B to the top before closing A. Bringing B to the top sends the partHidden event while A is still open. We can't change the order here since it's necessary to reduce flicker. Is it possible for you to track CHANGE_VIEW_HIDE notifications from a perspective listener on the window, instead of using IPartListener2? This is always sent when a view is closed in a perspective, although it unfortunately doesn't tell you -which- view.
We had the same discussion starting in bug 54559 comment 6, but that was deferred. This just seems like a bug. Sometimes listeners are notified at one point in the cycle and sometimes at another point? That makes life really rough on listeners. If it's not possible/desireable to be consistent and close the view before sending the event, could you possibly send two events?
On second thought... I don't understand comment 29 at all. At the time I close the Display view, the Display view is on top. What's the difference between closing the Display view with view B underneath it and closing the Display view with view B underneath it after view B has been on top at some point in the distant past? This problem *only* occurs if you've brought view B to the top at some point. You can have a whole stack of views under the Display view and the problem won't occur so long as the other views haven't obscured the Display view at some point. And once it gets into this error state, it stays that way. You can open and close the Display view repeatedly and never get null.
The partHidden notification is about views being obscured as well, not just closed. So the order of events makes sense if the workbench brings the view-to-be-activated-after-close to the top before closing the view. And this order is needed to minimize flicker. The controls underneath are marked invisible so if you close the one on top before making them visible you will see grey in the interim. I can try changing the order though just to see. For the inconsistent ordering, I suspect this is due to the difference when parts are lazily materialized after being restored. If you show B, then show Display, then close Display, you get the same effect as above. I'll have a look into why the lazy case is different though. In bug 54559, we discussed trying the perspective listener, but the part listener looked more promising at the time. I think it's worth trying now that the part listener isn't panning out. Do you really need to know which part is being closed? Aren't you always interested in tracking just the Display view?
This bug isn't specific to the Display view. So it will break our view management story as well. I think we're having a miscommunication about the test case. It doesn't matter if "view B" has been activated before activating the Display view. That won't cause the problem. You have to put the view on top of the Display view to get into the bug state. That's why this was so hard to figure out the test case for and why it looks like it's just a bug instead of being some kind of optimization. Steps to *not* reproduce the bug: 1. Launch Eclipse with the Display view closed. 2. Activate view B. 3. Open the Display view. 4. Close the Display view. 5. IWorkbenchPage#findView(...) will return null for the Display view. There's no difference between this case, in which view B will be activated when the Display view is closed, and the case that causes the bug. It's just that once you put view B on top of the Display view, the workbench gets into a broken state where it will always return non-null. To see this, bring view B on top of the Display view first. Then repeat steps 2, 3, and 4 and you'll get a different result.
I've been hitting this all day. It definitely breaks our view management story. We have to be able to tell when a view is closed.
We need this bug fixed for 3.0.
*** Bug 63332 has been marked as a duplicate of this bug. ***
Nick+Stefan, Do we have any options here?
Re the stacked vs. unstacked behaviour, this is expected. If the previously active part is in the same stack as the view being closed, then the previously active part is brought to top, hiding the view to be closed before it's closed. This has been the same since 2.1, is needed to reduce flicker, and we're not going to change it now. So this means that the partHidden hack cannot work for tracking when a part is closed in a perspective. I've been playing around with an alternative approach involving a new perspective event (internal for now). Please try the patches to follow and let me know if this works for you.
Created attachment 11180 [details] Patch to the workbench adding IPerspectiveListener2
Created attachment 11181 [details] Patch to Debug UI's LaunchView and LaunchViewContextListener to use the new IPerspectiveListener2
You should try this approach for showing a view as well. As it is now, the LaunchViewContextListener's partOpened method will not get called when the view is shared (i.e. was previously open in another perspective in the same window).
Another potential issue: LaunchViewContextListener keeps its view id sets locally (separate sets for each launch view), but persists them globally (in the pref store), so it's possible for these to get out of sync, e.g. when using multiple windows. The simplest approach is probably to keep these as statics.
The patch to the Debug UI plugin was a good start. I've removed our part listener and reworked things a bit so that we only update for the new perspective notifications at the right time. The API looks like exactly what we've needed (see bug 54559). Unfortunately, the HEAD stream of the workbench is busted right now and I can't open another perspective in my target. This prevents me from testing the case where we have views open in multiple perspectives (the interesting case). I needed to check SWT out of HEAD to get the workspace to compile, so that could be the source of the problem. Will test this again when we get a new integration build and I can use SWT as binary. Propose to target RC2 for this change.
Jared, sorry for the frustration on this one. I thought the partHidden workaround was going to work, but in the end I agree that only new lifecycle events will do. I'll submit an updated patch with the proposed API and workbench changes, and ask for this API addition to be allowed for RC2.
Created attachment 11257 [details] Updated workbench patch with IPerspectiveListener2 as API This patch improves the Javadoc for IPerspectiveListener2, moves it to an API package (org.eclipse.ui) and adds support for the other part-specific notifications (editor open/close, fast view add/remove).
API change approved for inclusion in 3.0.
Workbench patch released. This should make it into RC1. Jared, are there any other issues you need to have addressed here, or can this be closed now?
Sure. If there are any problems in the new support I'll open new bugs. I'm not gonna try to slip our client code into RC1, but I'll release it immediately afterwards.
OK, closing.
