Bugzilla – Full Text Bug Listing
|
Description
Matthew Hatem
We should look at doing the same for the view toolbar too. Created attachment 33169 [details]
DRAFT patch for jface
This is a draft patch for Jface
Created attachment 33171 [details]
DRAFT patch for ui.workbench
Created attachment 33172 [details]
patch for the presentations example plugin to demo other patches
Created attachment 33173 [details]
patch for the RCP browser example plugin to demo other patches
These patches shouldn't have any breaking API (although the "xx2" stuff is rather ugly). I haven't tackled the view toolbars yet but it should be straight forward to build on top of what I have done here. I've added a new class (ActionBarPresentation) that is responsible for building toolbars within the context of a presentation factory. The other larger pieces of the patch are related to changing workbench internals to use the coolbar and toolbar interfaces instead of the implementations. Eric, could you please review this patch? I can provide more context if needed. Paul, note that this includes changes to the presentations API as well, so you might want to have a look over it too. The patch looks good overall. I have the following comments. Would you be able to provide an updated patch incorporating these based on the latest contents from HEAD? Overall - for new types, @since tags only needed on type, not all methods - missing Javadoc on some new API methods, esp. @param and @return tags - missing @since 3.2 tags on a few API methods WorkbenchWindow - create*Manager methods that just call super can be deleted - can addCoolBar and addToolBar logic be moved up to ApplicationWindow? - change ApplicationWindow.createCoolBarManager2 to call createCoolBarManager, so that subclasses overriding createCoolBarManager will still get called if client calls createCoolBarManager2 (this also enables the addCoolBar and addToolBar change) IActionBarConfigurer - why expose getActionBarPresentation()? - browser example patch uses this to create custom toolbar manager in advisor - I guess this is needed since the custom coolbar manager may not be able to accept a regular ToolBarManager - can the presentation be hidden, e.g. by providng IActionBarConfigurer.createToolBarManager? ActionSetActionBars - Has if null check for actionBarPresentation when creating tool bar manager. Should ensure it always has an ActionBarPresentation implementation instead. - rather than passing in ActionBarPresentation, could delegate to look it up from window configurer (using internals WorkbenchWindow.getWindowConfigurer and WorkbenchWindowConfigurer.getPresentationFactory) AbstractPresentationFactory - missing @since tag on createToolBarPresentation - createToolBarPresentation should be createActionBarPresentation - instead of changing createStatusLine[Manager|Control] to return null and checking for null in WorkbenchWindow, should just have AbstractPresentationFactory default implementation delegate to old methods I think the effects on the presentation API are OK. PW I would like to veto this patch for 3.2. It directly conflicts with work I'm doing on commands. I was going to use the create*Manager methods to provide proxies to the new commands work. At a more basic level, this patch is attempting to allow RCP application developers control over view (i.e., presentation), but ends up locking us to a particular model implementation (i.e., actions). Part of the work with command contributions has been to try to separate the model from the presentation, so that the rendering of the menus/toolbars/etc. could be handled entirely by the presentation API. Unfortunately, it does not look like we have time for such an API in the 3.2 time frame (i.e., by API freeze in two weeks). If you're curious about how this would work, take a look at classes like SMenuManager and IMenuService. This provides a basic structure for modelling the user interface, which a renderer can later "parse" or "compile" into SWT widgets. I agree that having a clean separation between the commands "model" and how their presented would be a better solution. However, Matt's group needs a solution based on 3.2. I don't see how this patch makes things worse for the commands work than what we already have. For example, the ICoolBarManager for the window is already passed to the advisors via IActionBarConfigurer.getCoolBarManager(), allowing ActionBarAdvisor.fillCoolBar(ICoolBarManager) to populate it with pretty well any contribution item (usually a ToolBarContributionItem with an associated client-created ToolBarManager, but it's not restricted to that). Contribution items can create arbitrary controls. It would seem like all the IContributionManager APIs would have to be deprecated in favour of the proposed new declarative commands-based mechanisms. So while Matt's patch takes us further along a path that will eventually be deprecated, I don't see how it blocks you from making progress on the new story, or commits us any more than we already are to the old story. Let's discuss. I agree with Nick's comments. We need absolutely need something for 3.2. Douglas, if there is anything I can do to the patches to make them less conflicting with your work please let me know. I have ported the patches to HEAD and in the process of incorporating Nick's comments. I will release the revised patches later today. Created attachment 34107 [details]
Updated JFace patch with feedback based on HEAD
Created attachment 34108 [details]
Updated Workbench patch with feedback based on HEAD
Created attachment 34109 [details]
Updated presentations example patch based on HEAD
Created attachment 34110 [details]
Updated RCP browser example patch based on HEAD
Created attachment 34111 [details]
Optional patch to IDE plug-in to make it compatible with these changes
I have provided updated patches, all based off of HEAD as of 2:30pm 02/03/2006. The patches should reflect all feedback. Because of the possibility of conflicting with the commands work in the future, any new API has been marked as experimental and some classes have moved to internal packages. Matthew: Just so you know, it is possible to make a workbench patch, rather than a patch for each individual project. This is generally easier for us. I'm reviewing the patches now. The IDE patch includes an unwanted change to remove closeUnrelatedProjectsAction. Matt, here are my comments on the patches: Overall comments - All new API should have the EXPERIMENTAL disclaimer, including methods added to existing types in JFace. IActionBarConfigurer - createToolBarControl should not be exposed to clients - this is not currently used - note: for custom window layouts, clients use IWorkbenchWindowConfigurer.createCoolBarControl ActionPresentation - passes window in twice to constructor of ActionSetActionBars (once for service locator, once for new code's use of window) - instead, pass it in once and distinguish cases in constructor - also in CustomizePerspectiveDialog ActionBarPresentation - createToolBarManager should not take style bits -- the style bits to use should be a function of the presentation (passing in the coolbar's style bits was always bogus) - likewise for createViewToolBarManager EditorActionBars - still creates a ToolBarManager in case where an existing toolBarContributionItem is not found; should delegate to presentation instead - also need to properly handle the case where a PlaceholderContributionItem needs to get replaced with the real one. PlaceholderContributionItem is used to remember the placement of the editor action bars relative to the other toolbars, as restored from workbench.xml. InternalPresentationFactory - should have Javadoc explaining its intent, not just the EXPERIMENTAL disclaimer Example - could the example be enhanced to show the view toolbar case? Matt, can you take another pass on the patches? Nick, thanks for the feedback. I can take another pass at the patches. Re comment #21 "The IDE patch includes an unwanted change to remove closeUnrelatedProjectsAction." This was not intentional. Sorry. Created attachment 34239 [details]
Updated patches for JFace, Workbench, RCP Browser and Presentations Example
Created a Workspace patch this time ;-)
Created attachment 34308 [details]
Updated patch for JFace, Workbench, IDE, RCP Browser and Presentations Example
Here are my notes from reviewing the last patch. I've addressed all the issues raised in this updated patch.
ApplicationWindow
- Why promote coolBarManager and toolBarManager fields to be protected? I've reverted these to private.
IActionBarConfigurer
- createToolBarManager still takes style bits. I thought we were going to leave the style bits up to the presentation. I've removed the style bits argument.
ActionBarPresentation
- It really should hide the style bits for the coolbar too. I've removed the style bits argument from createCoolBarManager. WorkbendchWindow's override of createCoolBarManager(int) ignores the passed style bits (from its own constructor, which now passes SWT.NONE).
WorkbenchWindowConfigurer
- WindowActionBarConfigurer needs to delegate to proxy if set.
CustomizePerspectiveDialog
- can't return null for its IActionBarConfigurer's implementations of createToolBarContributionItem and createToolBarManager, since this gets passed to the app's advisor, which may use these (once WindowActionBarConfigurer is fixed)
- ActionSetDisplayItem.fillToolsFor should check for IToolBarContributionItem, not ToolBarContributionItem, otherwise customize perspective dialog shows nothing for toolbars when custom presentation is used
ActionSetActionBars
- Need to pass in IActionBarConfigurer to get proper implementation of bars for default window case vs. customize perspective dialog case. Fixed.
PluginActionSetBuilder
- ActionSetContribution.revokeActionSetFromCoolbar should check for IToolBarManager and IToolBarContributionItem instead of concrete classes.
IDE patch:
WorkbenchActionBuilder
- updateBuildActions should check for IToolBarContributionItem instead of ToolBarContributionItem, or it can't find toolbars when using custom presentation
I tried running the UI test suites with the custom presentation, and almost all the tests failed with SWTExceptions coming from the custom widgets. It appears that SToolBar/SToolItem and SCoolBar/SCoolItem do not properly handle dispose. When the control is disposed, the items are not also disposed (do not get the dispose callback, and isDisposed() returns false). I also saw cases where when an item is disposed, it's not removed from its parent's list of children. The UI tests run OK when not using the custom presentation, so the problem seems to be isolated to the custom presentation's widgets. But to get more confidence in the workbench/jface patches, I'd prefer if they also passed when using the custom presentation. To run the UI tests: - load org.eclipse.ui.tests and its prerequisites from CVS - select org.eclipse.ui.tests/Eclipse UI Tests/org.eclipse.ui.tests/UITestSuite - choose Debug as > JUnit Plugin Test To run the tests with the custom presentation, add the following in the Arguments tab of the launch configuration: -pluginCustomization d:\eclipse-new\customization.ini where customization.ini has: org.eclipse.ui/presentationFactoryId=org.eclipse.ui.examples.presentation.customtoolbar I'm holding off releasing the patch until tomorrow, since there's some other churn going on about the I-build and whether to resubmit. Nick, thanks for updating the patches. I should have mentioned that the custom toolbar and coolbar widgets are not yet complete. I'll try to update the custom widgets and managers so they may pass the UI test suites. Thanks again. Created attachment 34367 [details]
Updated patch for JFace, Workbench, IDE, RCP Browser and Presentations Example
Fixed patch.
A couple more issues were encountered (yet to be addressed):
InternalPresentationFactory
- should not extend WorkbenchPresentationFactory
- either extend AbstractPresentationFactory, or use a mix-in interface; the latter would allow existing presentations (like WorkbenchPresentationFactory) to be subclassed, otherwise they would need to be wrappered
- rather than adding a factory method to return an ActionBarPresentation, should add methods directly (like was done for status line)
- should not pass IWorkbenchWindow -- presentation objects are not given direct access to regular workbench objects
custom presentation:
- if item added or removed in view toolbar, it does not repaint (I added buttons to add/remove items in MockViewPart in the tests).
SToolBarContributionItem2
- handleContextMenu should use getControl2 instead of getControl in
if (toolBarManager instanceof SToolBarManager2)
toolBar = ((SToolBarManager2)toolBarManager).getControl();
otherwise toolBar gets null and an NPE results when right clicking over a toolbar
I've applied the patch to JFace, Workbench, IDE and RCP Browser (except the changes to the ini file for actually using the custom presentation). One more issue: IActionBarConfigurer.createToolBarContributionItem() should return an IToolBarContributionItem instead of IContributionItem (in case the caller needs to make further tweaks to it). To clarify the interface mix-in approach to replace InternalPresentationFactory, I'm thinking along these lines: - create a new interface IActionBarPresentationFactory with the same methods as are currently in ActionBarPresentation - delete InternalPresentationFactory and ActionBarPresentation - in the workbench, rather than looking for an InternalPresentationFactory and calling createActionBarPresentation, check if the presentation factory implements IActionBarPresentationFactory and if so cast to that; if not, return a default implementation - for the example CustomPresentationFactory, it should extend WorkbenchPresentation and implement IActionBarPresentationFactory Created attachment 34373 [details] updated patch for Workbench and Presentations Example Nick, thanks for the updates. I have created a new patch to reflect your suggestions in comment #31 and #32. I'm still working to get the custom toolbar/coolbar to pass the UI tests. I've reviewed and applied the last patch. I'm definitely happier with this approach. The custom presentation issues mentioned in comment 31 didn't seem to be addressed though. To clarify: I only released the Workbench portion of the last patch. I'm treating your custom presentation as a test case -- I don't think it should be applied to the presentations example plugin. Closing. If there are further issues with this patch, please open new PRs. Created attachment 34437 [details]
Patch for JFace, Workbench, IDE, RCP Browser and Presentations Example
For 3.2, we need to draw a stronger line between what is provisional API and what is supported public API. To this end, I've done some refactoring to move the provisional API introduced here into new internal.provisional packages. This required introducing ICoolBarManager2, and some new subclasses to support the new interfaces. The old classes and interfaces are left unchanged as much as possible. ApplicationWindow has new createCoolBarManager and createToolBarManager methods, but these are in terms of the old interfaces. This change requires a few more instanceof checks throughout JFace and the Workbench, but it's not too bad. Basically WorkbenchWindow knows that its coolbar manager is an ICoolBarManager2, and goes from there.
Verified in I20060213-1200, using custom presentation in browser example and IDE. The fix for this defect seems to have introduced bug 128888. I've committed (>20050327) a patch that simply adds 'SWT.WRAP' into -both- 'createToolBarManager' & 'createViewToolBarManager'. This fixes 128888 but I'll re-open this defect so that Matt/Nick can ensure that I haven't broken their 'custom' work. For createToolBarManager, we should check the old behaviour (i.e. 3.1 behaviour) in ActionSetActionBars. I don't believe it specified SWT.WRAP for the main toolbars. Otherwise, the change sounds fine. Matt, can you confirm? I don't remember changing any style bits but otherwise this change sounds fine. A quick check with 3.1.2 indicates that the main TB's didn't use WRAP before so I've removed that bit from the non-View ToolbarManager creation method 'createToolBarManager'... Committed in >20060330. Re-closing since bug 128888 is fixed. Matt, can you verify that this is working for you in RC3? Verified working in RC3. Thanks Matt. |