Bugzilla – Full Text Bug Listing
| Summary: | Render menu contributions using the ContributionManagers | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] e4 | Reporter: | Remy Suen <remy.suen> | ||||||||||||||||||||||||
| Component: | UI | Assignee: | Paul Webster <pwebster> | ||||||||||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||||||||||||
| Severity: | major | ||||||||||||||||||||||||||
| Priority: | P3 | CC: | bokowski, emoffatt, Olivier_Thomann, remy.suen, Szymon.Brandys, tom.schindl | ||||||||||||||||||||||||
| Version: | 1.0 | ||||||||||||||||||||||||||
| Target Milestone: | 4.1 M4 | ||||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||||
| OS: | Windows XP | ||||||||||||||||||||||||||
| See Also: | https://bugs.eclipse.org/bugs/show_bug.cgi?id=546165 | ||||||||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||
|
Description
Remy Suen
This is really weird. I feel like the toolbar should've been unrendered when the part was unrendered, not because the CTabFolder was disposed... Thread [main] (Suspended (breakpoint at line 70 in CleanupAddon$1)) CleanupAddon$1.handleEvent(Event) line: 70 UIEventHandler.handleEvent(Event) line: 41 EventHandlerWrapper.handleEvent(Event, Permission) line: 188 EventHandlerTracker.dispatchEvent(Object, Object, int, Object) line: 198 EventManager.dispatchEvent(Set, EventDispatcher, int, Object) line: 230 ListenerQueue.dispatchEventSynchronous(int, Object) line: 148 EventAdminImpl.dispatchEvent(Event, boolean) line: 139 EventAdminImpl.sendEvent(Event) line: 78 EventComponent.sendEvent(Event) line: 39 EventBroker.send(String, Object) line: 73 UIEventPublisher.notifyChanged(Notification) line: 58 RenderedToolBarImpl(BasicNotifierImpl).eNotify(Notification) line: 380 ENotificationImpl(NotificationImpl).dispatch() line: 1033 ENotificationImpl(NotificationImpl).dispatch() line: 1038 ElementContainerImpl$1(NotifyingListImpl<E>).remove(int) line: 724 ElementContainerImpl$1(AbstractEList<E>).remove(Object) line: 466 RenderedToolBarRenderer.cleanUp(MRenderedToolBar) line: 121 RenderedToolBarRenderer$1.widgetDisposed(DisposeEvent) line: 101 TypedListener.handleEvent(Event) line: 123 EventTable.sendEvent(Event) line: 84 ToolBar(Widget).sendEvent(Event) line: 1053 ToolBar(Widget).sendEvent(int, Event, boolean) line: 1077 ToolBar(Widget).sendEvent(int) line: 1058 ToolBar(Widget).release(boolean) line: 808 CTabFolder(Composite).releaseChildren(boolean) line: 872 CTabFolder(Widget).release(boolean) line: 811 CTabFolder(Widget).dispose() line: 446 StackRenderer(SWTPartRenderer).disposeWidget(MUIElement) line: 132 PartRenderingEngine.removeGui(MUIElement) line: 532 PartRenderingEngine$1.handleEvent(Event) line: 128 UIEventHandler.handleEvent(Event) line: 41 EventHandlerWrapper.handleEvent(Event, Permission) line: 188 EventHandlerTracker.dispatchEvent(Object, Object, int, Object) line: 198 EventManager.dispatchEvent(Set, EventDispatcher, int, Object) line: 230 ListenerQueue.dispatchEventSynchronous(int, Object) line: 148 EventAdminImpl.dispatchEvent(Event, boolean) line: 139 EventAdminImpl.sendEvent(Event) line: 78 EventComponent.sendEvent(Event) line: 39 EventBroker.send(String, Object) line: 73 UIEventPublisher.notifyChanged(Notification) line: 58 PartStackImpl(BasicNotifierImpl).eNotify(Notification) line: 380 PartStackImpl(UIElementImpl).setToBeRendered(boolean) line: 267 CleanupAddon$3$1.run() line: 226 RunnableLock.run() line: 35 UISynchronizer(Synchronizer).runAsyncMessages(boolean) line: 134 XML snippet to reproduce the problem.
<extension point="org.eclipse.ui.viewActions">
<viewContribution
id="f.viewContrib"
targetID="org.eclipse.jdt.ui.PackageExplorer">
<action
class="f.ViewAction"
id="org.eclipse.mylyn.java.actions.focus.packageExplorer"
label="label"
toolbarPath="mylyn">
</action>
</viewContribution>
</extension>
(In reply to comment #1) > This is really weird. I feel like the toolbar should've been unrendered when > the part was unrendered, not because the CTabFolder was disposed... There's definitely a problem here (which may or may not be related to this original bug). When I close the 'Javadoc' view (which has a toolbar), its toolbar is not getting disposed even though I only have the 'Java' perspective up which implies there should be no other valid placeholders pointing at it. (In reply to comment #3) > When I close the 'Javadoc' view (which has a toolbar), its > toolbar is not getting disposed even though I only have the 'Java' perspective > up which implies there should be no other valid placeholders pointing at it. Opened bug 325522 for this. Created attachment 179140 [details]
Disable 'auto-hiding' of MRenderedToolbars
NOTE: The TB going empty is a result of our using a mixed model to handle the contributions...
In its original state a rendered toolbar has *no* children. This is a reflection of the legacy code pattern where the TB is 'filled in' during widget creation by accessing the View's ToolbarManager and directly adding IContributions to it...
...but 'real' contributions (coming from extensions) are turned into model elements (MRenderedToolItems). These are, in turn, removed before the model is saved (this is where the CleanupAddon was kicking in) because the MRenderedToolbar went 'empty'.
We should re-examine this strategy...
One (admittedly rather radical) option is to *never* turn legacy IContributionItems into model elements. For cases were we want to 'mix in' E4 tool items we might look at a specialized renderer that does the inverse of what we normally do in the compatability layer...it should 'render' the toolitem by *creating* an IContributionItem and letting the menu manager deal with placing it in the correct ordering slot (we'd need to have a model attribute to match the IContributionItem's placement info "after=some.menuitem.id".
Committed in >20100917. Applied the patch. Not marking as FIXED...This is considered an interim fix as we certainly have more to do in order to manage this type of extension (see previous comment). With the discussion at yesterday's meeting, we should use this bug look at changes of how toolbars and menus are rendered. For consideration: Our menu and toolbar model are rendered by creating MenuManager and ToolBarManager to do the actual SWT rendering. A MHandledMenuItem can easily be added by generating a HandledContributionItem, and creating an equivalent DirectContributionItem (for MDirectMenuItem) is also trivial. The advantages I see: 1) MenuManager (and ToolBarManager) understand the SWT element lifecycle. They also have a well-known rendering lifecycle. 2) In compatibility we converted all of the action extension points to model, and most of the programmic contributions that we understood (ActionContributionItem, CommandContributionItem contributed programmaticly), but there are still some compatibility contributions that are not convertable to the model. 3) in the future it will be possible to potentially map back some MenuManager IContributionItems as opaque model elements (with IDs). This could help stabilize the menu ordering problems often associate with the compatibility layer. I'm looking at contribution based renderers ATM. PW Working in branch bug325392, so far 3 projects: #P org.eclipse.e4.ui.tests #P org.eclipse.e4.ui.workbench.renderers.swt #P org.eclipse.e4.ui.workbench.swt Menu rendering in e4: 1) Render a "top level" MMenu element. That is a menu under a window, a view menu, or a context menu. 2) don't render submenus. The MenuManager/ContributionItem protocol will take care of that. 3) rendering an MMenu will create IContributionItems for MHandledMenuItem and MDirectMenuItem PW ToolBarRendering can render handled items, direct items, and tool controls PW Branched org.eclipse.e4.ui.workbench as well Contributions are applied to the e4 main menu. MPopupMenus are getting their contribuions plus a bunch of blank items. PW Created attachment 180268 [details]
patch of work so far.
This now includes visibleWhen updates during MenuManager menuAboutToShow
PW
Released the first big changes: 1) the MMenu with a non menu parent is processed by the menu renderer (main menu bar, context menu, or view menu). 2) Any MMenuContributions are added to the MMenu and a record that manages their visibility is created. 3) the menu renderer generates "rendering elements" (a fancy word for IContributionItems) for the MMenu's children 4) the menu renderer sets up listeners to calculate child visibility only on a Menu SWT.Show event. The MToolBar is part-way through the same transformation. PW Created attachment 180514 [details]
Fixing the toolbar Renderer v02
This is a draft patch modifying the toolbar rendering. So far, not so good.
PW
The toolbars are all appearing, but I've to a disappearing view menu problem to track down. PW Created attachment 180901 [details]
Revert renderer changes for I build
released
PW
Created attachment 181172 [details]
MenuManagerRenderer v01
Working on the Menus only. Visibility of the menu bar works, and of items in a menu. Still need items in a submenu, and to deal with removeAllWhenShown.
PW
Created attachment 181191 [details]
MenuManagerRenderer v02
Stop generating multiple IMenuListeners, handle visibility and enablement in the SWT.Show
PW
Created attachment 181226 [details]
MenuManagerRenderer v03
We now update visibility on SWT.Show and correctly map MMenus to MenuManagers.
PW
Created attachment 182202 [details]
MenuManagerRenderer v07
Integrate with PopupMenuExtender.
PW
Created attachment 182509 [details]
MenuManagerRenderer v08
Support for actionSet pulldown menu items like Run As in the main menu bar.
PW
Created attachment 182904 [details]
MenuManagerRenderer v10
Current state. Apply this patch, then use the genmodel to generate the model and edit model.
PW
Created attachment 183010 [details]
MenuManagerRenderer v11
Covering popup, main menu, and view menus.
Released to HEAD.
PW
I've opened bugs for some of the next steps: Bug 330112 - [Compatibility] Outline view menu only shows up once per switch Bug 330114 - [Compatibility] Link the WW menu manager Bug 330115 - [Compatibility] support the old-style EditorMenuManager Bug 330116 - Support MMC additions Bug 330119 - render toolbar contributions using the ContributionManagers PW If I get the new Model elements right they are only there for the compat layer or do we expect native e4 applications also make use of it? If not I'd like to keep them out of our main UIElements.ecore and move it to Compat.ecore which extends UIElements.ecore. (In reply to comment #24) > If not I'd like to keep them out of our main UIElements.ecore and move it to > Compat.ecore which extends UIElements.ecore. The MOpaque* items are used in the compat layer only, but need to be used in the Renderer code. PW Ok. The ideally we'd make our WorkbenchRendererFactory extensible, the compat layer could naturally also set it's custom E4Workbench.RENDERER_FACTORY_URI in the context which knows about those new model elements but I think we could also get away completely
On the other hand would it be possible to simply flag the opaque elements simply e.g. adding a tag "opaque" beside that we could add a general in transient storage feature to our ApplicationElement similar to what Widget#setData() where you can restore opaqueItem.
So here is my proposal:
a) Add transient data: Map<String,Object> to ApplicationElement
b) Remove Rendered* and instead tag Menu/ToolBar* with "rendered"
c) Remove Opaque* and instead tag Menu* with "opaque"
d) Store informations like "opaqueItem", ... in the data-Map
This would keep our Model clean of stuff only needed by the compat layer. If this is not the way to go I'd suggest we move this stuff to itsown .ecore and ideally even move this .ecore to the compat-bundle.
So here are the choices:
1. Remove classes from .ecore and use data and tags to mark the classes and
make the renderer use those tags and data informations
2. Keep classes but move to Compat.ecore
2.1 Keep model stuff in ui.model.workbench
2.2 Move .ecore to compat bundle and make the compat layer:
2.2.1 Contribute It's custom IRendererFactory
2.2.2 Make WorkbenchRendererFactory extensible and the compat layer
contribute renderers who are aware of the Compat.ecore classes
The cleanest solution IMHO would be 2.2.2 because it would provide us a lot of freedom in the future to make special model elements for the compat layer where we can't get away as easy as we do with the opaque stuff e.g. if not mistaken MenuContribution and MenuContributions are also only in use in the compat layer?
MenuContributions become MMenuContributions, which are used in e4 RCP. In e4 the "flow" is always model -> renderer -> Managers/ICIs just do the rendering. When we introduce the compatibility layer stuff, we also need the Manager/ICI -> Renderer -> MOpaque model step. But it needs to be done in a couple of locations during the menu lifecycle. While it could be refactored and be plugged in, that would have to be taken into account in the rendering step such that placeholder opaque items aren't generated back *into* the Managers. PW |