Community
Participate
Working Groups
20070417 The current way of configuring the items shown on the toolbar is limited as items can only be enabled/disabled by action set. a.) need a way to enable/disable single toolbar items The workaround to define more action sets is not the same, as each action set creates a vertical separator on the toolbar. The list of action sets also gets very big and hard to manage. b.) want to enable/disable toolbar items independent from menus Currently disabling a action set removes toolbar buttons and menus. c.) Add a context menu to the toolbar with a 'Configure Toolbar...' action. The current way: Window > Customize Perspective > Commands.. is not intuitive (I miss the term 'Toolbar' here). This comes from discussion in bug 181040 (remove New Java project from toolbar), and would be, IMO, a requirement before we start changing tool bar items.
I think this fits in well with the menu override functionality. It unfortunately didn't make it into 3.3, but targets per-item toolbar or menu control. Then perhaps a dialog that displayed all buttons currently in the toolbar with their visibility status, and allowed the user to toggle them individually. A similar process could be targetted for the menus. PW
Note that due to the highly dynamic nature of toolbar or menu contributions, it will be hard to make toolbars and menus configurable in the full sense, like being able to reorder items, or making items appear when they are not. What should be doable is a UI with which you can hide items, and subsequently unhide items that you've hidden. Could we start with this and then take it from there?
Why is it hard to allow to reorder things? The "Customize Perspective" dialog has a list of all command groups. Reordering this list and using this order as the order in which things are displayed should be possible.... Anyway. Here are my priorities (first is most important): - make menu contributions independent of toolbar contributions - allow do override the default visibility of command groups (e.g. ClearCase contributes 10 toolbar buttons to all perspectives, and I have to remove them by hand for every perspective (some contributions think they are so important that they should appear on all perspectives)) - allow to enable/disable single items from a command group - reordering is not really important
See also bug 75934 and bug 2861 (has lots of dups).
Created attachment 113124 [details] First go at menu hiding UI This patch presents the first draft ui which would be used to hide menu items. To use: 1. Download patch 2. Check out org.eclipse.ui.workbench. 3. Apply the patch to the project (right click on the project > Team > Apply Patch...) 4. Start a new instance of Eclipse. 5. Start the "Customize Perspective..." dialog (right click on a toolbar > Customize Perspective...) 6. There is a new tab "Menu Items". Click on it. Use the "Menu Structure" tree on the left to choose which menu items to hide or show: checked means it is shown, unchecked means it is hidden. Use the "Hidden Menu Items" list on the right to view all hidden menu items in this perspective. Click anything in this list to highlight it in the "Menu Structure". Use "Suppress hidden menu items from Menu Structure tree" to remove hidden menu items from the "Menu Structure" - you can still see them in the "Hidden Menu Items" list.
Created attachment 114358 [details] Progress on improvement this is an intermediate snapshot of a solution for this bug. It's still very much in progress.
Created attachment 115732 [details] CheckProvider Further work has been done on this bug. We are starting to approach a final solution. We created a model to manage standard check state behaviour in CheckboxTreeViewers and added CheckProvider functionality - allowing on-demand check state querying on a per-element basis. Putting these together with the dialog logic, a useful dialog is almost complete. We discovered that our current behaviour is not perfect, however. We attempted to merge the functionality of the "Commands" tab (used to turn action sets on and off) with the "Menu Items" tab (used to turn individual menu items on and off) and the "Toolbar Items" tab. Unfortunately, simply turning menu and toolbar items on and off does not affect their keybindings, whereas turning action sets on and off activate or deactivates all keybindings underneath them. They are not interoperable, so our implementation is rather confusing to a user. As such we realized we need to take the GUI in a new direction. Attached is our work up until now - before taking the aforementioned "new direction" in an effort to take a snapshot of work up until now. It's still not complete - saving and restoring state is not yet fully implemented, and actually causing a change to be reflected in the menus and toolbars is not implemented. Nonetheless, a lot of work is represented here. I am submitting the work in two separate patches: the CheckProvider logic in one patch (as it can be applied independently of the menu / toolbar item hiding work) - this patch itself, and the remainder of the CustomizePerspectiveDialog logic in a second patch to follow. Patch 1 / 2 - CheckProvider
Created attachment 115733 [details] CustomizePerspectiveDialog logic - latest attempt Patch 2 / 2 - CustomizePerspectiveDialog. See previous attachment comments for details.
I should have mentioned, that in the two above patches, Patch 2 / 2 requires Patch 1 / 2 to be applied first.
Created attachment 115971 [details] Tests for TreeManager This includes a test class which verifies the standard check tree behaviour in the included model TreeModel.
I didn't look at the patch yet but have a question: will this also allow to configure the view tool bars?
Dani, short answer is no. This new functionality appears in the 'Customize Perspective' dialog so won't affect a view's Menu, Toolbar or Context menu (I don't think we want per-perspective view TB arrangements do we?). Also worth noting here is that this will -not- allow repositioning of menu/tb elements, it only affects the -visibility- of the various components. Hiding a menu or toolbar item will -not- disable its key-bindings either (but we will have a link to the key bindings preference page that will help if you want to unbind them). Disabling a 'Command Group' (aka ActionSet) -will- both hide the set's items as well as disabling any existing key bindings.
>I don't think we want per-perspective view TB arrangements do we? No, I'd expect the view toolbar and menu configuration to have the same life-cycle as its settings.
Created attachment 116424 [details] Tests for setCheckStateProvider First go at creating tests for the new setCheckStateProvider in CheckboxTreeViewer and CheckboxTableViewer
Thanks Matt, I'll go over the patch(es) tomorrow...
Matt, this looks very good...just a few nits: 1) We should use the same logical values as are currently used in SWT; The current ICheckProvider has a slightly different interpretation but consistency should win out here (should likely change 'isGray' to 'isGrayed' for the same reason). 2) There seems to be quite a lot of overlap in the testing of Tree vs Table versions. Perhaps we can create a common 'utility' class shared by both. This will at least make future folks in this area aware that there are two different controls sharing the bilk of the logic. Test Coverage: run the test coverage tool on your tests. What current coverage do we have? What's missing?
Created attachment 116615 [details] ICheckStateProvider - revised This patch reflects the changes Eric suggested. It contains both the ICheckStateProvider/viewer code and the test code. 1. The isChecked/isGrayed semantics now reflect SWT's use in the viewers. 2. I've extracted a lot of common functionality between test cases within test classes, and in turn extracted a lot of common functionality between the two test classes into a utility class. 3. I've run test coverage. All new code is covered. The two tests I modified - CheckboxTableViewerTest and CheckboxTreeViewerTest each have about 50% coverage. Untested parts of these viewers include ICheckStateListener handling, about half of the check state methods, selection handling, some constructors and some static methods.
Really, really close! So far in the implementation I've seen the following: Still missing the @since tags on the new API (come see me and we'll set you up with an API baseline). I'm a bit confused about the comment in the 'preservingSelection' method (in both Viewers): //Still run the updateCode, just don't try to preserve //the selection Doesn't the 'super' call explicitly preserve the selection? Perhaps something like: // Try to preserve the selection, let the ICheckProvider manage the check states I'm not even sure if that's enough change to re-spin the patch but it likely wouldn't hurt...;-). Test evaluation coming...BTW, thanks for the nice synopsis on the coverage (you might think about some tests to up the %'age for out post-M3 'test days'...;-).
Matt, the tests look fine. I'd add some explicit tests to cover each of the four boolean states just to ensure that all the 'boxes' are covered.
Created attachment 116657 [details] ICheckStateProvider - revised again Thanks for the comments Eric. Here's what I've done: 1. Set up the baseline in my IDE, added the @since tags. 2. Changed comments to make more sense 3. I hope to bolster the CheckboxTableViewer and CheckboxTreeViewer tests :-) 4. Modified the CheckboxTableViewer and CheckboxTreeViewer logic to consider all four cases (unchecked-ungrayed, unchecked-grayed, checked-ungrayed, checked-grayed) whereas it had previous converted the unchecked-grayed case to unchecked-ungrayed. The current SWT behaviour is described in the ICheckStateProvider interface: unchecked-ungrayed and unchecked-grayed are rendered identically. 5. Modified the tests I already had written to cover all four cases, whereas they had previously had only generated and checked the tree cases. Now they generate and check both. 6. Added explicit tests which simply setup a single element and test its state. I made one test for each of the four cases.
Created attachment 117242 [details] CustomizePerspectiveDialog UI Work This patch represents all work done to the UI portion of hiding menu items and toolbar items by modifying CustomizePerspectiveDialog. It requires a full code review, however at present all the necessary functionality (that I'm aware of) is there. In summary: Shortcuts tab -- same functionality Action Sets tab -- same functionality, renamed to "Command Group Availability" Menu Visibility tab -- allows menu item visibility to be set, menu items can be filtered by command group Toolbar Visibility tab -- allows toolbar item visibility to be set, toolbar items can be filtered by command group All these tabs remain up-to-date with each other's information. Action which put other tabs in an inconsistent state are not allowed (e.g. trying to make a menu item visible whose command group is unavailable). Also, tabs have not yet been reordered, however we plan on doing this.
Created attachment 117267 [details] CustomizePerspectiveDialog UI Work Found some errors in the typos. One less thing to cover in the code review :-)
Created attachment 117321 [details] Workbench Window Menu hook v01 Matt, here's a patch for you to explore overriding the main workbench window menu. PW
Hi Paul and Matt, I did something very similar to this bug a few months ago (bug 75934 comment #9). It only deals with toolbar visibility, but the main idea is the same. I'm really glad this issue is being addressed in 3.5 and I hope my patch may save you some time. -Pawel
Matt, Boris and I have reviewed the patch for the CheckboxViewer (and tests). It looks very good. The final tweaks we've identified are: - Test with setting a sorter and or a filter... - Test to ensure proper 'lazy' tree behavior (i.e. the check provider is -not- called for unexpanded tree children) - setCheckstateProvider should force 'refresh' to match 'setLabelProvider' (+ a test to ensure that the refresh happens) - The 'updateCheckStates' should either be private or the code moved into the doUpdate... method As a special bonus we should put a reference to SWT Snippet274 in the j'doc (for CheckboxTreeViewer) to point folks at the 'standard' boxing behavior sample. Once you're done attach the patch here. I'll copy it over to the actual enhancement request bug once I've found it...
Created attachment 117701 [details] ICheckStateProvider - final? I've incorporated all of your comments into this patch - hopefully it's satisfactory!
Created attachment 118138 [details] CustomizePerspectiveDialog UI and Logic Thanks to Paul's help, the CPD now actually hides menu and tool bar items! This patch also has: - cleaned up code - tabs reordered - tooltips have been given some more useful information - tooltips now have a minimum width
Created attachment 118176 [details] CustomizePerspectiveDialog UI and Logic found a bug in the last patch... it's corrected in here!
Created attachment 118192 [details] documentation for the revised CPD New help files, including interactive help, Tasks=>Perspectives help and Tutorials. Includes new pictures (mostly screen caps) - not sure if the patch will transmit them properly. Let me know and I'll send them to you.
CPD Review: Overall it looks fine...the class is a monster now at ~3400 lines but it does -lots-! Once you fix the nits below and I get Paul to review the Commands changes it's likely good to go...;-) ToolBarContributionItem: apparently unnecessary override of 'update' CoolBarManager2: only change is a newline Perspective: non-null checked 'get' of the CoolBarManager (@2602) + Fix 'ToDo' (IWorkbenchRegistryConstants?) ReopenEditorMenu: only change is whitespace CPD: Is there no way to get rid of all of the constants ? See WorkbenchTriggerPoints... Too many inner classes ?? Some of these (The TTip support classes) look like that could go into jface (internal?) "nonentity" ? How does this work ? Override your layout's 'computeSize' instead. 1116: typo 'oppose' -> 'opposed' can 'toDispose' ever get dups? if so, need an 'isDisposed()' check before disposing...use Set? Do we need funky accessibility column support? getIDFromIContributionItem: Not sure if throwing an exception is the right thing (destroys control flow) just return 'null' Refactor the CPD:ContributionItem to something that doesn't match an existing class that's already being used TreeManager: Just directly cast the object, that'll throw a CCE at the same place as you do but the code is cleaner
Created attachment 118518 [details] CustomizePerspectiveDialog UI and Logic - post code review Replies to comments: > ToolBarContributionItem: apparently unnecessary override of 'update' fixed > CoolBarManager2: only change is a newline fixed > Perspective: non-null checked 'get' of the CoolBarManager (@2602) fixed > + Fix 'ToDo' > (IWorkbenchRegistryConstants?) fixed > ReopenEditorMenu: only change is whitespace fixed > CPD: > > Is there no way to get rid of all of the constants ? See > WorkbenchTriggerPoints... I'm afraid it wasn't so simple... * icons - could not find the text anywhere else * "new" - replaced with org.eclipse.ui.ActionFactory.NEW.getId() * "openPerspective", "showView" - no way to access text * "org.eclipse.ui.newWizard" - not same as the constant in WorkbenchTriggerPoints * "org.eclipse.ui.perspectives.showPerspective" - not visible anywhere (it is private in some places) * "org.eclipse.ui.views.showView" - replaced with ShowViewMenu.SHOW_VIEW_ID * "newWizardId" - unavailable * "org.eclipse.ui.perspectives.showPerspective.perspectiveId" - unavailable * "org.eclipse.ui.views.showView.viewId" - replaced with ShowViewMenu.VIEW_ID_PARM * "org.eclipse.ui.preferencePages.Keys" - not accessible anywhere else * System.getProperty("line.separator") - every where else is used in the same way > Too many inner classes ?? Some of these (The TTip support classes) look like > that could go into jface (internal?) for the time being, we'll leave alone. In the future we may want to promote these to top-level classes > "nonentity" ? How does this work ? Override your layout's 'computeSize' > instead. totally rewrote this part: previously, many sub-composites were being used, now the GridLayout is being used properly to achieve both layout and minimum size > 1116: typo 'oppose' -> 'opposed' fixed > can 'toDispose' ever get dups? if so, need an 'isDisposed()' check before > disposing...use Set? fixed > Do we need funky accessibility column support? we've decided for the time being not to worry about this. It's very minor. > getIDFromIContributionItem: Not sure if throwing an exception is the right > thing (destroys control flow) just return 'null' changed > Refactor the CPD:ContributionItem to something that doesn't match an existing > class that's already being used renamed it to "DisplayItem" > TreeManager: Just directly cast the object, that'll throw a CCE at the same > place as you do but the code is cleaner done As well, I found a bug: when we turned filtering by command group on, the decision of whether to disable a menu item (i.e. make it grey) did not take into account which children of the item were visible: it simply calculated this state from all its children. The result was some parent menu items showing children which were all grey/disabled, but the parent did not appear grey/disabled. Clicking on its checkbox had no feedback (including no checkbox state change). I've fixed this as well.
Created attachment 119064 [details] Customize Perspective Dialog UI and Logic - revised Found a bug - in Command Groups tab, hovering over leaf menu or toolbar item in an unavailable command group gives wrong message.
Should we try to get this in for M4? This is a large change and needs as much testing as possible from the community. There is a risk involved, being so close to M4, but it is a change to a feature that seems (at least to me) not critical. What do you think?
I agree that we should get early and as much testing on this from the community as we can get, hence putting it in M4 is good. However, I see two conditions for putting this into M4 (which is claimed to be a STABLE build): 1) it must be in the next I-build (I20081202-0800) - putting it into an I-build during for the milestone week would be clearly too late and risky in my opinion 2) while enabled by default there should be a switch to turn off the new code in case of severe damage to M4
That was the intent. I'm running the tests now and if they don't show anything bad this will go in on Monday.
There are four tests that currently fail. They all occur when there is no open perspective, causing an NPE at line 1279 of WorkbenchWidnow. I'll add the null check and keep going...
Missed one, another check added at line 1193.
(In reply to comment #32) > Created an attachment (id=119064) [details] > Customize Perspective Dialog UI and Logic - revised > Looks good. The only 3 comments I have are: 1) In org.eclipse.ui.internal.Perspective.setHiddenToolbarItems(Collection) you extract the CoolBarManager from the WorkbenchWindow. This might be one of the places that requires one of the WorkbenchWindow update or re-layout methods. I'd talk to Eric about this. 2) I would still add an @since tag to org.eclipse.jface.internal.provisional.action.ICoolBarManager2.setOverrides(IContributionManagerOverrides) 3) org.eclipse.ui.menus.CommandContributionItem.getCommand() needs a proper javadoc since it is API, and I would add that the ParameterizedCommand returned from this method should be treated as a read-only object. We can remove the restriction in M5 if it is unnecessary. PW
(In reply to comment #36) > There are four tests that currently fail. They all occur when there is no open > perspective, causing an NPE at line 1279 of WorkbenchWidnow. > > I'll add the null check and keep going... Could you post the pieces of code that fail with a NPE here? I'd like to know what kind of NPEs these are.
Boris, they were in the WorkbenchPageTest suite, any that set the current perspective to null (testGetOpenPerspectives, testGetSortedPerspectives, testClosePerspective & testCloseAllPerspectives) would NPE...the code was: //Has the client intentionally hidden the menu item? if(perspective.getHiddenMenuItems().contains(id)) { return Boolean.FALSE; } I just added a null check...
Paul, I've just added the changes as you've suggested. We've been looking into the layout issues and I'll open a separate defect to cover it... Matt, looks really nice, good work! Paul, thanks dude (a lot) for making this all possible by updating the menu infrastructure to make this doable... Committed in >20081201. Applied the last patch -and- the TreeManagerTests patch.
Committed in >20081201. Applied the last patch -and- the TreeManagerTests patch.
(In reply to comment #40) > Boris, they were in the WorkbenchPageTest suite OK so the null checks were needed in the tests, not in WorkbenchWindow.java? Have you checked the Javadoc for the called methods to make sure that they don't return null illegally?
Boris, the changes were in WorkbenchWindow - you can see them on line 1193. The null variable was the result of invoking WorkbenchPage.getActivePerspective(), which is permitted to return null. The null check now takes care of this case.
O-o: this caused severe damage, see bug 257179.
Added bug 257185 to consider dynamic contribution edge case. PW
Committed in >20081202. Committed the documentation patch.
NOTE: I had to get a valid copy of the images directly from Matt, the ones in the patch are corrupt...
> NOTE: I had to get a valid copy of the images directly from Matt, the ones in > the patch are corrupt... Matt should have seen a warning when he created the patch. See bug 67306 and bug 257263.
Verified in I20081210-1800.
