Bugzilla – Full Text Bug Listing
| Summary: | [ViewMgmt] add attribute to close a sticky view in all open perspectives | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Jamie Liu <liuj1> |
| Component: | UI | Assignee: | Boris Bokowski <bokowski> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | cbeth, eclipse, emoffatt, Matthew_Hatem, n.a.edgar, pwebster |
| Version: | 3.2.1 | Keywords: | api, helpwanted |
| Target Milestone: | 3.3 M6 | ||
| Hardware: | PC | ||
| OS: | Windows XP | ||
| Whiteboard: | |||
| Attachments: | |||
|
Description
Jamie Liu
This issue was mentioned in bug 88568 (https://bugs.eclipse.org/bugs/show_bug.cgi?id=88568) with the original author's Point #6. However, based on the last comment (#14), it appears to be a feature request as opposed to a defect, therefore creating a separate bugzilla report to track only this issue. Sounds like a reasonable request. Unfortunately, we might not have the resources to look into this for 3.3. Would you be interested in contributing a patch? I think it's reasonable and expected that if a sticky view is closed, it should be closed in all perspectives in which it was opened due to being a sticky view. The reason this wasn't done originally is because the implementation was tricky - see 88568 comment 12, not because we thought it wasn't the right behaviour. If a sticky view S is open in perspective P due to having its own association with the view, then I'm not sure that closing S in perpective Q should affect P. But maybe that's too complicated a user model. Something to consider though. Created attachment 60756 [details]
Preliminary patch for those that want to help in testing.
It can't be this easy.
Ultimately we need to wrap this new behavior with a preference setting. I'll address that in another patch.
I haven't completely tested this but, aside from the preference setting, I don't see why this patch wouldn't resolve this bug. I'm probably missing something. There might be some hidden gotchas with saveable views.
Can someone review the patch and provide feedback.
Kim, prove Matt wrong or you lose two Curries! ;-) Created attachment 60944 [details]
Updated patch with preference and unit tests
This is a more complete patch with the preference to enable the new behavior. Naming this preference was tricky.
Before I added the preference one of the intro tests (testPerspectiveChange) was failing, which was expected. Once the preference was added this test no longer failed.
I've also added a couple new unit tests to test this new behavior. I put them in IntroTest.java. Not sure if this is the right place for them.
Mmmm almost. :) Open clean workspace Open sticky help view Open second perspective - note the help view appears Close help view in second perspective Switch back to first perspective - help view still visible I noticed another case I haven't been able to reproduce just yet - in it it I got into the state where I opened a sticky view, changed to a second perspective and saw that the help wasn't visible. I switched back to the first perspective only to find it wasn't visible there either. I hadn't thought of this approach Matthew - my attempts were around actually closing a view in a non visible perspective which seemed hopeless. This seems like it could work with enough poking and prodding... Kim, I've tried that case it it worked for me. Are you enabling the preference? Matt: yep. It may be more complicated than that. It seems like resetting the perspective has a different effect than using the "dirty" one. I'll poke at it a bit more. I am seeing some unexpected when running with saved state for the perspectives. Seems that the activatedSticky list isn't the same in that case. Btw, the preference should work the other way (if you can get this to work): The fixed behaviour should be the default, with a way to turn it off if people relied on the old behaviour. One problem might be that we never remove a view from the activatedStickyViews list. I think that's it. Since the new behavior is that the visibility of the sticky view is consistent across perspectives, then the active sticky view list needs to be cleaned up when a view is closed via hideView(). Attaching and updated patch based on HEAD. Created attachment 61000 [details]
Updated patch to resolve issues that Kim found.
Created attachment 61085 [details] New refactored patch, simplifies WorkbenchPage.java After looking at this some more I've realized that with the new close behavior we really don't need to keep track of activated sticky views for each perspective. Instead we simply need to ensure that the visibility of sticky views is consistent when we switch perspectives. In an effort to simplify the WorkbenchPage class I've refactored things a bit. I've moved all of the sticky view handling to a new class and created two classes one for the old 3.2 behavior that still uses a map and tracks the activated views, saves state etc. The class responsible for the new behavior is much more simple. I've also updated the preference and the preference initialization to reflect Boris' comments in comment #11. Created attachment 61089 [details]
New refactored patch, simplifies WorkbenchPage.java
Sorry, previous patch had a line of debug code in it. This one is clean.
The patch appears to work and is easy to understand but I'm getting three test failures - testPerspectiveChange, testPerspectiveChangeWithCloseAllPrefTrue, and testPerspectiveChangeWithCloseAllPrefFalse. There may be more but I haven't ran the full suite, only IntroTests testPerspectiveChange is expected to fail now that the new behavior is turned on by default. The other two tests testPerspectiveChangeWithCloseAllPrefTrue, and testPerspectiveChangeWithCloseAllPrefFalse are the ones that I added. Not sure why those are failing, probably the same reason as the first. I'll update the patch. +1 Created attachment 61168 [details]
Updated patch with new unit tests.
I've updated the unit tests to reflect the fact that the new behavior is now enabled by default. Also the previous tests assumed the preference was dynamic, which is no longer the case.
(In reply to comment #19) > +1 I just talked to McQ, he gave the +1 for adding the preference. Released with a small fix (NPE when running the tests). Thanks for your patch, Matt! Thanks Boris. Can you tell me where the NPE was? I'd like to adapt this patch to 3.2.2. The NPE happened when one of the perspectives was null - this happens e.g. when you close all perspectives and then open a new one. I added a null check at the beginning of update():
if (newPersp == null || oldPersp == null) {
return;
}
Verified in I20070322-1800. Thanks Matt! I owe you curry next time you're in town. :) |