Bug 259048

Comment 1 Paul Webster 2008-12-17 09:05:14 EST

In what case ... bug 246999 does address this issue, and adds the proxy for a number of cases.

Please attach an example that shows the plugin being loaded even though the control is not.

PW

Comment 2 Pawel Piech 2008-12-17 10:59:51 EST

Created attachment 120711 [details]
Patch to org.eclipse.ui.examples.contributions

This patch adds a visibleWhen condition to a control.  Even when the control is not shown in the opened perspective, the plugin is started.  If the control is commented out completely (in plugin.xml), then the plugin is not started upon start up.

When looking at the implementation, in MenuAdditionCacheEntry, the new proxy contribution item (DynamicMenuContributionItem) is used for the dynamic menu contribution, but there is no equivalent proxy for a control.  This may be an oversight... or maybe I just don't know what I'm talking about :-)

Comment 3 Prakash Rangaraj 2008-12-19 03:56:05 EST

Created attachment 120917 [details]
Patch v01

Patch attached.

Comment 4 Paul Webster 2008-12-19 07:55:50 EST

(In reply to comment #2)
> contribution, but there is no equivalent proxy for a control.  This may be an
> oversight... or maybe I just don't know what I'm talking about :-)
> 

No, you're right (I misread the problem at first) ... Prakash is on the right track.

PW

Comment 5 Paul Webster 2008-12-30 15:24:46 EST

(In reply to comment #3)
> Created an attachment (id=120917) [details]
> Patch v01
> 

The problem with making this the same as org.eclipse.ui.internal.menus.DynamicMenuContributionItem (aside from not deleting that one :-) is that various internal code expects to deal with WorkbenchWindowControlContribution or InternalControlContribution to support setting the IWorkbenchWindow or to dock(*) the control to a side.  Their lifecycle needs to be taken into account, and probably kept separate from standard  DynamicMenuContributionItem.

You can try adding a control test similar to the ones supported by org.eclipse.ui.tests.menus.MenuBuilder and org.eclipse.ui.tests.api.workbenchpart.MenuContributionHarness

PW

Comment 6 Prakash Rangaraj 2009-03-12 13:21:11 EDT

Will try to push it for M7

Comment 7 Prakash Rangaraj 2009-03-23 13:01:07 EDT

Created attachment 129616 [details]
Patch v02

I've created a separate class DynamicToolBarContributionItem for proxying the contribution. Patch attached.

Comment 8 Paul Webster 2009-03-26 10:13:35 EDT

(In reply to comment #7)
> Created an attachment (id=129616) [details]
> Patch v02

OK, right direction.  However, you 1) can't add API past M6 in WorkbenchWindowControlContribution and 2) you can't make a change like that which would force all the client code to change.

Also, it still needs tests and it left a compile error in the examples (which should be in your workspace).

PW

Comment 9 Prakash Rangaraj 2009-03-26 12:53:37 EDT

OK. I didn't realize that I'm breaking the API. But without making that method as public, we can't access the method from proxy class (see Bug# 269478). How do we proceed now?

Comment 10 Paul Webster 2009-04-20 08:56:49 EDT

I think we'll have to differ this to 3.6 where we can consider the best API to solve this problem (which is our problem, not a contribution problem).

PW

Comment 11 Prakash Rangaraj 2009-10-01 02:40:07 EDT

Reflection could solve the problem:

In DynamicToolBarContributionItem:
	
public Control createControl(Composite parent) {
		
	WorkbenchWindowControlContribution contributionItem = getContributionItem();
	Object control = null;
	if (contributionItem != null) {
		try {
			Class clazz = ControlContribution.class;
			Method createControlMethod = clazz.getDeclaredMethod("createControl", new Class[] { Composite.class }); //$NON-NLS-1$
			createControlMethod.setAccessible(true);
			control = createControlMethod.invoke(contributionItem, new Object[] { parent });
		} catch (Exception e) {
			WorkbenchPlugin.log(e);
		}
	}
	return (Control) control;
}



But is Reflection an accepted solution?

Comment 12 Paul Webster 2009-10-01 09:07:03 EDT

(In reply to comment #11)
> 
> But is Reflection an accepted solution?

If we just need to call this method from our delegate class, I would add a public Control delegateCreateControl(*) method instead of using reflection.

We can also consider updating ControlContribution.  Should it have a delegate mode?  I'm not sold on this idea.  Should we change the fill(*) methods so they are not final?  We can still add a @nooverride statement in the javadoc, and that would allow us to make changes for the delegate case.

PW

Comment 13 Prakash Rangaraj 2009-10-12 03:11:02 EDT

Created attachment 149331 [details]
Patch v03

(In reply to comment #12)
> If we just need to call this method from our delegate class, I would add a
> public Control delegateCreateControl(*) method instead of using reflection.

   That works :-)

Comment 14 Prakash Rangaraj 2009-10-20 02:45:26 EDT

Patch v03 released to HEAD

Comment 15 Prakash Rangaraj 2009-10-27 13:13:15 EDT

Verified in I20091027-0100