Community
Participate
Working Groups
I added propertysheet support to my application. When I had it working more or less ok, I refactored this support to a separate plugin and suddenly my property sheet stopped working. No error messages or log - my view just did not manage to get the IPropertySheetPage for my view. Closer inspection and debugging lead me to conclude that the problem lies in the overzealously lazy loading of the AdapterFactory, which avoids loading of the plugin bundle at any cost when retrieving adapters. The lazy loading in AdapterFactoryProxy should either force the plugin to be loaded, or if this behavior is intentional, there should be a (preferably declarative) way of forcing the plugin to be loaded.
By "default", the adapter manager does not eagerly load adapter factories from plugins that have not been activated. This is by design - we always want to avoid eager activation of plugins when it may not be necessary. However, IAdapterManager provides API so plugins that want to eagerly load adapters can do so. In the case of property sheet contributions, it seems reasonable that it will load the necessary plugin. I suggest that IAdapterManager#loadAdapter be used to load property sheet adapters to achieve this.
(In reply to comment #1) That's what I suspected ... Where this loadAdapter method should be called from? I would not want to call it from any of the plugins my property sheet contribution contributes it's property sheets to, since those plugins should not be concerned with propertysheet contributions. Neither can I call it from the contributing plugin itself, since it has not been loaded yet... So I assume it has to be done by the property sheet?
On a second thought a more generic solution would be adding an (optional) attribute to the adapter extension point that would allow plugin-writer to override this lazy loading rule in cases it might be preferable that loading an adapter should cause a plugin to be loaded as well. It seems preferable since it puts a responsibility of loading the plugin to the hands of the plugin writer.
You can see a lengthly debate on this subject in bug 82973. It is debatable whether the decision to load the extension should lie with the client that wants to do the adapting, or the factory that can perform the adaption. My inclination is toward having the client make this decision - as there are scenarios where a given factory may be used in multiple situations, some of which warranting plugin activation and others not. Thus I was suggesting in this particular case that the PropertySheet can call loadAdapter to ensure that the plugin providing the property sheet is loaded.
I think I side with Daniel in bug 82973: it feels like a breaking API change to require existing callers of IAdaptable.getAdapter() to change in order to properly support declarative adapters. John, I'd appreciate it if you could resolve this issue either way, and tell me how best to fix the Properties view (if at all).
Since there is no requirement for implementors of IAdaptable.getAdapter() to forward the lookup to IAdapterManager, there is no way to make declarative adapters work universally without code changes from clients. Even if IAdapterManager were to always eagerly load factory plugins, it won't help if the adapter manager is never called (which is the case currently with the property sheet). I'm not sure myself what the right answer is. One answer is that adapter factories were not available prior to 3.0 until the associated plugin was loaded, so it would be completely consistent to do nothing at all here. It wasn't previously possible to decouple views from their associated property sheet into a separate plugin, so nothing is lost if we leave things as they are.
Actually, I'll recant. I'm fine with making a code change in order to support declarative adapters, since it's a new feature in 3.0. It's just unclear to me what the code change should be, in order to remain backwards compatible with the direct adaptable.getAdapter() call.
(In reply to comment #7) The way i see, how declarative loading of adapters should work, needs only an additional attribute to the factory element in org.eclipse.runtime.adapter extensionpoint and some additional code in AdapterFactoryProxy: First we need to add an optional attribute to the factory element that should govern the lazy loading of the adapter factory - say we call it "forceLoad" and the default (implied) value of this attribute should be "false", so that there would be no change in behavior for those who don't need this. AdapterFactoryProxy we should create a new method that roughly looks like this: IAdapterFactory loadFactory() { String result = element.getAttribute("forceLoad"); //$NON-NLS-1$ boolean forceLoad = "true".equals(result) return loadFactory(forceLoad); } and replace all instances of the loadFactory(false) with the new method call. Maybe I'm missing here some of the details, but this should give a rough outline of what I had in mind. The change does not require a massive rewrite of the adapter clients and should be backwards compatible to any code written so far. Also this leavs both paths open - if the client code feels like calling IAdapterManager#loadAdapter, it still can do that, but then again - it allows extenders to force loading of their plugin if that is what they want. If You call IAdaptable#getAdapter directly, You already have the plugin loaded and if the getAdapter does not rely on adapter manager to get it's adapters, this situation is of no concern. There still remains an issue of one AdapterFactory providing adapters to more than one scenario, some of which need to force loading and some don't. As far as I can see, there should be an easy workaround to this problem by defining two factories - one for the cases that need to force the plugin load and the other for those that don't... Another option has also been suggested in bug 82973 that the "force" attribute should be used per adapter type instead of per factory, but that might be actually more work than needs to be...
John, are there any plans to enhance the adapter factory API along the lines above?
We have no such plans for the 3.1 timeframe. Note that PropertySheet would need to change regardless - since it currently doesn't call IAdapterManager at all, there is currently no opportunity to plugin in declarative adapters. Since loading the associated extension seems to always be acceptable in the case of the property sheet, I suggest having PropertySheet.doCreatePage call IAdapterManager.loadAdapter in the case where the direct getAdapter call on the part returns null. I.e., protected PageRec doCreatePage(IWorkbenchPart part) { // Try to get a custom property sheet page. IPropertySheetPage page = (IPropertySheetPage) part .getAdapter(IPropertySheetPage.class); if (page == null) page = (IPropertySheetPage) Platform.getAdapterManager().loadAdapter(part, IPropertySheetPage.class.getName()); if (page != null) { if (page instanceof IPageBookViewPage) initPage((IPageBookViewPage) page); page.createControl(getPageBook()); return new PageRec(part, page); } // Use the default page return null; }
Thanks for the recommendation John. I'll do this early in M7.
I've made the change suggested by John. Roland, would you be able to try your scenario with the next build?
I suppose it is fair workaround and for this particular case it should work just fine (alas I won't be able to test it until next milestone release though due to our internal policy not to use dayly CVS snapshots) My suggestion though was rather directed at a more general solution where any plugin extending an existing feature through adaptable framework would be able to decide if lazy loading would force loading of the plugin or not. There is definatley a case where this decision should be made by plugin rather than the client - eg a plugin that provides solely an extension based on the adaptable type versus the plugin that provides same adapter as a part of some larger feature set. Firstone probably does need to be loaded by the AdapterManager, but the second case would most likely be better off keeping the current behavior. So to conclude - I'm halfway happy. I can now have just the plugin extending property pages, but on the larger scale the issue is still not solved.
Roland, if you're allowed to use integration builds, there are several happening today. For the general discussion, please comment in bug 82973. This bug report covers the Propeties view case only.
Created attachment 21133 [details] Test plug-in used to verify This contains a zip file with a plug-in project that declaratively contributes an IPropertySheetPage showing a simple message in the Properties view when a plain text editor is opened.
Verified with the attached plug-in in I20050513-0010.
