Bugzilla – Full Text Bug Listing
| Summary: | [FastView] Ability to hide fast view toolbar | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Mike M <gubespam> | ||||||||||
| Component: | UI | Assignee: | Platform UI Triaged <platform-ui-triaged> | ||||||||||
| Status: | VERIFIED FIXED | QA Contact: | Eric Moffatt <emoffatt> | ||||||||||
| Severity: | enhancement | ||||||||||||
| Priority: | P3 | CC: | ob1.eclipse, semion | ||||||||||
| Version: | 3.4.2 | ||||||||||||
| Target Milestone: | 3.6 M2 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Windows XP | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Mike M
Mike, good pickup. I thought there was already a preference for this but it turns out not to be the case. Good pickup, ever since we din the minimized stack work we've been talking about getting rid of it so this is my chance, add the preference to show it and set it to false...;-). If you're doing an RCP app you can hide it using: org.eclipse.ui.internal.WorkbenchWindowConfigurer.setShowFastViewBars(boolean) Created attachment 143913 [details]
Suggested patch
Semion, thanks (a lot!) for the patch. I'll review it once I get onto 3.6 work... Thanks Semion, very nice patch! Fast views have been traditionally a complicated area to work with. The patched version works well for me. I'll let Eric comment on the fast view side, a few general comments: => when the fast view bar is turned off, if there are any fast views, would it be possible to convert those fast views into regular views? Otherwise they will remain fast views forever => Copyright headers: there are two dates in the copyright headers, something like: * Copyright (c) 2000, 2006 IBM Corporation and others. The first date is the year file was originally written; the second date is the last year the file was midified. People very often overlook the need to update the second year. In the copyright headers we need to also list all people who contributed to the file who are not comitters on the project. The line is usually <name>, <e-mail>[, <company>] [- fix for bug <number> <title>] Something like: * Contributors: * IBM Corporation - initial API and implementation * John Doe, john@doe.com, DOE Inc. - for for bug 12345 Time machine broke => Bundle versions. This is surprisingly confusing subject, see http://wiki.eclipse.org/Version_Numbering You are adding new API in org.eclipse.ui.workbench, so that one will have to be 3.6. The org.eclipse.ui.ide.application only has implementation changed so verison will be 1.0.200. The org.eclipse.ui only has implementation changed so version would be 3.5.100. => The WokbenchWindow - it has a commented out line of code - is it a lefover? - not sure: #getFastViewBarVisible() - should this one include getWindowConfigurer().getShowFastViewBars()? - not sure: in the new version we always create fast view bar. Do we really need to do it, as opposing to creating it only when it is visible? - not sure: there is a call defaultLayout.forceLayout(); added to the end of updateLayoutDataForContents(). Is it always needed in that method? Oleg, thanks for comments. Here are replies for some your questions: >> - when the fast view bar is turned off, if there are any fast views, would it >> be possible to convert those fast views into regular views? Otherwise they >> will remain fast views forever We still have two ways to convert them into regular views: 1) turn on "fast view" bar using the same preference page and convert as usually; 2) open views using "Show View" and move them to any place into workbench. However, if auto conversion is, from your point of view, better, I will try to do it. >> - not sure: #getFastViewBarVisible() - should this one include >> getWindowConfigurer().getShowFastViewBars()? Currently getFastViewBarVisible() returns WorkbenchWindow's internal variable, which is synchronized with the similar variable from WindowConfigurer. >> - not sure: in the new version we always create fast view bar. Do we really >> need to do it, as opposing to creating it only when it is visible? I understand requirement to hide "fast view" bar as possibility to switch on/off its visibility. Is adding/removing is more close to your design? If yes, I should try to find the way to restore bar on the same place, where it was before removing and, of course, provide it with possibility of fast views' "auto" conversion. >> - not sure: there is a call defaultLayout.forceLayout(); added to the end >> of updateLayoutDataForContents(). Is it always needed in >> that method? I was needed to add this call, because without it internal contributions disappeared from the bottom trim area after switching on/off visibility of "fast view" bar. Is my understanding correct? Semion, first off I agree with Oleg that this is an above quality patch, good work and we *really* appreciate the effort! While the current implementation works it could break existing clients due to an already existing preference; the OPEN_MODE. Open the preference dialog and go to General->Perspectives. Notice that there's an option to have all new views open as Fast Views. This could be in conflict with the 'Show Fast View Bar' option. After kicking this around with Oleg for a bit I think we may have a different approach to suggest. Rather than thinking of this as a 'global' preference we might be better off to consider it as preference that controls how the -perspective- manages the FVB's visibility. Note that the views that show up in the FVB are defined on a per-perspective basis (you can have different fast view 'sets' in different perspectives). This means that there's already code in 'Perspective#onActivate' to update the FVB (ok, it's really in the FastViewManager, see below...;-). If we were to change the preference to be 'Show Empty Fast View Bar' (or 'Hide Empty Fast View Bar'?) then we could simply change the existing activation code to show/hide the FVB based on whether or not it's empty and the preference is set. Hint: check out the FastViewManager#updateTrim method, it has all the code to show/hide the minimized stacks and all the 'special' code to manage the FVB. An added bonus is that we no longer have to care about what the 'setShowFastViewBars' does in the advisor, we're only interested in how to manage it -if it's there-. If the advisor had set it to false then there won't be a trim element with the id of the FVB so the code will never run. Created attachment 144617 [details]
The second version of the patch.
Eric, Oleg, I have tried to consider all your remarks.
I like the way the toolbar disappears when there are no views and appears after some view has been made a fast view. Very nice way to clean up UI. As you probably discovering, the problem with Eclipse is that many things can be done in a multiple ways. In this case I see two scenario that needs to be connected to this hide/show of the Fast view toolbar: Switching perspective: 1. While in the Java perspective, select "Hide FVB when empty". Fast view bar disappears, good; 2. Switch to the Debug perspective, make an Outaline a Fast View by R-Click on on his tab. Fast view bar appears, good. 3. Switch to Java perspective. Now we have an empty Fast View bar at the bottom while the option is set to "Hide" Restarting Eclipe: 1. While in the Java perspective, select "Hide FVB when empty". Fast view bar disappears, good; 2. Shutdown Eclipse and start it up again. The Fast View Bar is at the bootom while the option is set to "Hide". => On the UI side, the Perspective preference page looks somewhat broken up by the new checkbox. Do you think it will flow a bit better if: Instead of the group "Open a new view" create a group "Fast Views" with both "open a new view" and "Hide empty fast view bar" in it; probably changing "Open a new view" to be a label? => Technicalities :-) - The "Hide empty fast view bar" needs a keyboard shortcut Good progress, I think once the perspective switch and startup are connected, this will be a very nice enhancement. Created attachment 145353 [details]
Third version of the patch.
Here is one more version of the patch: GUI is changed, keyboard shortcut is added. Unfortunately, I can't recreate the problems with perspective switch and startup. Can you, please, check this once more with the new patch?
(In reply to comment #10) > ... Unfortunately, I can't recreate the problems with perspective switch and > startup. Can you, please, check this once more with the new patch? Sorry, it was a problem with my workspace - after playing with changes to presentations it somehow got into a confused state as to which presentation style it was supposed to use. The patch from 08/23 works well for me; the changes look good. The "org.eclipse.ui.workbench" bundle version in the manifest.mf probably should stay at 3.5.100. The bundle versions are confusing; here is the logic in this case: - The released bundle version for Eclipse 3.5 was 3.5.0 - When internal implementation is modified, we increment the third segment by 100, making it 3.5.100. - It was already incremented by Dani to 3.5.100 (file's CVS history). We *usually* increment versions only once per release cycle. We would need to increment it to 3.6.0 if the patch had a backwards compatible API changes, but in the last patch only internal parts are modified, so bundle version stays at 3.5.100. This is a rather confusing subject, especially when maintenance releases gets involved. Aside from this detail, patch looks and works very well for me, great work! Created attachment 145463 [details]
Slightly modified version of Semion's patch
Committed in >20090824. Applied this patch.
Semion, very nice!! Thanks a lot for this, it will make quite a few folks happy. I'll make sure you get full credit on the N&N page for the next milestone...this is, after all, new *and* noteworthy.
I made two mods to the code you sent, both trivial:
I removed the 'checkVisibility' method from the FVB and added the code directly to the 'okPressed' handling. This means that the FVB has no changes (this is good because we're actually only changing when it's visible so I didn't expect to see changes there). I had to make the 'updateTrim' public but that's OK because it's an internal class so it's not an API change.
I also reverted the change to the manifest since we don't change the qualifier to be '3.6' until there's an actual API change in the component (nobody gets this right, except Oleg...;-).
I'm just adding this patch to keep a clear record of what was actually committed...
Marking as FIXED. Works as expected in I20090915-0100. |