Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 325392 - Render menu contributions using the ContributionManagers
Summary: Render menu contributions using the ContributionManagers
Status: RESOLVED FIXED
Alias: None
Product: e4
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 1.0   Edit
Hardware: PC Windows XP
: P3 major (vote)
Target Milestone: 4.1 M4   Edit
Assignee: Paul Webster CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-15 16:28 EDT by Remy Suen CLA
Modified: 2019-04-08 09:06 EDT (History)
6 users (show)

See Also:


Attachments
Disable 'auto-hiding' of MRenderedToolbars (1.54 KB, patch)
2010-09-17 13:55 EDT, Eric Moffatt CLA
no flags Details | Diff
patch of work so far. (149.78 KB, patch)
2010-10-05 14:06 EDT, Paul Webster CLA
no flags Details | Diff
Fixing the toolbar Renderer v02 (29.91 KB, patch)
2010-10-08 15:04 EDT, Paul Webster CLA
no flags Details | Diff
Revert renderer changes for I build (214.31 KB, patch)
2010-10-14 14:22 EDT, Paul Webster CLA
no flags Details | Diff
MenuManagerRenderer v01 (53.60 KB, patch)
2010-10-19 07:59 EDT, Paul Webster CLA
no flags Details | Diff
MenuManagerRenderer v02 (74.90 KB, patch)
2010-10-19 11:23 EDT, Paul Webster CLA
no flags Details | Diff
MenuManagerRenderer v03 (74.03 KB, patch)
2010-10-19 16:46 EDT, Paul Webster CLA
no flags Details | Diff
MenuManagerRenderer v07 (100.84 KB, patch)
2010-11-02 09:08 EDT, Paul Webster CLA
no flags Details | Diff
MenuManagerRenderer v08 (167.62 KB, patch)
2010-11-05 14:13 EDT, Paul Webster CLA
no flags Details | Diff
MenuManagerRenderer v10 (112.84 KB, patch)
2010-11-11 09:50 EST, Paul Webster CLA
no flags Details | Diff
MenuManagerRenderer v11 (115.42 KB, patch)
2010-11-12 11:42 EST, Paul Webster CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Remy Suen CLA 2010-09-15 16:28:54 EDT
The CleanupAddon is flagging the toolbar to not be rendered.

Thread [main] (Suspended (breakpoint at line 266 in UIElementImpl))	
	RenderedToolBarImpl(UIElementImpl).setToBeRendered(boolean) line: 266	
	CleanupAddon$1$1.run() line: 79	
	RunnableLock.run() line: 35	
	UISynchronizer(Synchronizer).runAsyncMessages(boolean) line: 134
Comment 1 Remy Suen CLA 2010-09-15 16:35:35 EDT
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
Comment 2 Remy Suen CLA 2010-09-15 19:18:10 EDT
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>
Comment 3 Remy Suen CLA 2010-09-15 19:24:43 EDT
(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.
Comment 4 Remy Suen CLA 2010-09-16 16:10:33 EDT
(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.
Comment 5 Eric Moffatt CLA 2010-09-17 13:55:02 EDT
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".
Comment 6 Eric Moffatt CLA 2010-09-17 14:11:10 EDT
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).
Comment 7 Paul Webster CLA 2010-09-24 10:26:37 EDT
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
Comment 8 Paul Webster CLA 2010-09-28 10:03:41 EDT
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
Comment 9 Paul Webster CLA 2010-09-28 13:25:34 EDT
ToolBarRendering can render handled items, direct items, and tool controls
PW
Comment 10 Paul Webster CLA 2010-09-30 13:53:17 EDT
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
Comment 11 Paul Webster CLA 2010-10-05 14:06:38 EDT
Created attachment 180268 [details]
patch of work so far.

This now includes visibleWhen updates during MenuManager menuAboutToShow

PW
Comment 12 Paul Webster CLA 2010-10-08 08:07:10 EDT
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
Comment 13 Paul Webster CLA 2010-10-08 15:04:21 EDT
Created attachment 180514 [details]
Fixing the toolbar Renderer v02

This is a draft patch modifying the toolbar rendering.  So far, not so good.

PW
Comment 14 Paul Webster CLA 2010-10-12 12:18:44 EDT
The toolbars are all appearing, but I've to a disappearing view menu problem to track down.

PW
Comment 15 Paul Webster CLA 2010-10-14 14:22:12 EDT
Created attachment 180901 [details]
Revert renderer changes for I build

released
PW
Comment 16 Paul Webster CLA 2010-10-19 07:59:00 EDT
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
Comment 17 Paul Webster CLA 2010-10-19 11:23:37 EDT
Created attachment 181191 [details]
MenuManagerRenderer v02

Stop generating multiple IMenuListeners, handle visibility and enablement in the SWT.Show

PW
Comment 18 Paul Webster CLA 2010-10-19 16:46:37 EDT
Created attachment 181226 [details]
MenuManagerRenderer v03

We now update visibility on SWT.Show and correctly map MMenus to MenuManagers.

PW
Comment 19 Paul Webster CLA 2010-11-02 09:08:22 EDT
Created attachment 182202 [details]
MenuManagerRenderer v07

Integrate with PopupMenuExtender.

PW
Comment 20 Paul Webster CLA 2010-11-05 14:13:00 EDT
Created attachment 182509 [details]
MenuManagerRenderer v08

Support for actionSet pulldown menu items like Run As in the main menu bar.

PW
Comment 21 Paul Webster CLA 2010-11-11 09:50:37 EST
Created attachment 182904 [details]
MenuManagerRenderer v10

Current state.  Apply this patch, then use the genmodel to generate the model and edit model.

PW
Comment 22 Paul Webster CLA 2010-11-12 11:42:15 EST
Created attachment 183010 [details]
MenuManagerRenderer v11

Covering popup, main menu, and view menus.

Released to HEAD.

PW
Comment 23 Paul Webster CLA 2010-11-12 12:00:08 EST
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
Comment 24 Thomas Schindl CLA 2010-11-13 09:00:36 EST
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.
Comment 25 Paul Webster CLA 2010-11-13 15:37:59 EST
(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
Comment 26 Thomas Schindl CLA 2010-11-14 05:52:26 EST
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?
Comment 27 Paul Webster CLA 2010-11-18 08:58:04 EST
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