Bugzilla – Full Text Bug Listing
| Summary: | IAdapterManager and bundle activation | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Daniel Krügler <daniel.kruegler> | ||||
| Component: | Runtime | Assignee: | platform-runtime-inbox <platform-runtime-inbox> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | enhancement | ||||||
| Priority: | P3 | CC: | bokowski, carej, christian.pontesegger, costescuandrei, CraigFoote, daniel_megert, eclipse, gunnar, ivanla, jean-michel_lemieux, john.arthorne, Lars.Vogel, Michael.Valenta, Mike_Wilson, mober.at+eclipse, ob1.eclipse, phil.kursawe, prakash, pwebster, roland, sgandon, tjwatson, tonny.madsen | ||||
| Version: | 3.1 | ||||||
| Target Milestone: | --- | ||||||
| Hardware: | PC | ||||||
| OS: | Windows 2000 | ||||||
| Whiteboard: | |||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 86362, 193095, 197664, 218304 | ||||||
| Attachments: |
|
||||||
The current behaviour of this extension point is very much intentional. Prior
to Eclipse 3.0, there was no extension point and all adapter factories were
registered programmatically. Thus, it was always the case that "getAdapter()"
would only function against the factories that had already been registered
(i.e., those belonging to plugins that are already loaded). The behaviour you
observe with the adapters extension point preserves this old behaviour from
previous releases.
The platform always tries to avoid mechanisms that cause other plugins to be
loaded implicitly ("for free"). The current design forces the caller to decide
if they really want to cause plugins to be activated in order to obtain the
adapter. In many cases, callers are happy to only get access to the factories
that have already been loaded, so getAdapter() works fine for them. Use cases
where activation is desired must willingly do so by calling loadAdapters().
I should mention that the main reason the adapters extension point was
introduced was to add the hasAdapter() method, which works regardless of whether
the adapter factory has been loaded. The common pattern is that hasAdapter is
used to filter what is shown to the user, and then when the user clicks on the
result, the adapter is actually loaded. I.e., direct user action is required to
cause the factory to be loaded - no loading happens "for free".
First of all I didn't propose a solution which would either break backward
compatibility or even would cause any implicitly loaded plugins - My preferred
proposal was definitely position B!
I would like to emphasise, that my recommendation was to provide an
**additionally** boolean switch ("force") as part of the
org.eclipse.core.runtime.adapters ext. pt., which is **optional** and by default
is **not activated**, so that **nothing** changes for current ext. providers!
**But** this new proposal would allow users of that ext. pt. to have the chance
to get a currently impossible opportunity (without hacks), i.e. they could make
the adapters ext. pt. enforce to activate their **own** (and **not** others)
org.eclipse.core.runtime.adapters extensions. So I really don't see, how this
proposal would cause any harm. In contrast, I would say:
I have demonstrated in my original contribution, that the current usage of
loadAdapters should **never** be recommended (and is actually rarely found in
Eclipse code itself), because there possible implications are:
1) It would either imply brute force activation of **every** plugin, for which
an extension is asked for on my own IAdapter implementation (The mechanism,
which calls loadAdapter internally of that implementation, namely point (1))
2) Proposing to use AdapterManager.loadAdapter on a given IAdaptable does
**sidestep** (or hack, if you want) the actual getAdapter mechanism as I
demonstrated for feasible IAdapter implementations, which delegate there request
on some internally hosted IAdapter reference. These implementations would never
be activated and that **is a bug**!
I showed, that it **could** be solved by extending the IAdapter interface but I
also showed the weaknesses of that proposal and did **not** recommend it.
So my basic question is: Why (the hack) should the **proposed**
(backwardc-compatible!!) extension of the adapters ext. point. hurt anyone??
To my opinion this is the **only** possible way to reach the sometimes wanted
effect to activate my own extension. The only alternatives I have are:
I define my own helper ext. pt. which ensures activation of my registered
extensions, and which would activate those. **This** is indeed a bad idea:
First I have to **doubly** register my plugin classes via some **dummy** ext.
pt. just to activate them and secondly users are **not invited** to use the
org.eclipse.core.runtime.adapters ext. pt. - And this really violates one of the
basic principles of Eclipse!
I think the key distinction is whether the decision to load the factory is a function of the factory extension, or a function of the caller. For example, let's say you define a factory that adapts from "IJavaProject" to "IProject". There might be several different clients that want to perform that transformation. Some callers might want to force the factory to be loaded, either because they are an action explicitly invoked by the user, or some long-running operation that is willing to pay the activiation cost. Other callers might be satisfied to only use factories that are already loaded - say they are using the factory to determine action enablement, or some test that must be quick. In other words, IAdaptable objects and adapter factories know how to transform themselves into objects of a different type. They don't always know *why* the caller wants to perform that transformation. If I understand your proposal correctly, you want the factory extension to determine whether loading occurs or not. In this case the caller will have no choice about whether the plugin providing the factory is loaded. Some factories would be loaded eagerly, and other factories would not. The result is that when the client calls IAdapterManager#getAdapter, they can no longer control whether plugin activations occur. You might be thinking that plugin activation is a detail that should be encapsulated and hidden from clients of an API. In Eclipse it is often the opposite - we want higher level plugins (and ultimately the end user) to have that control. I realize this sometimes makes life more difficult for plugins using our API. We have both raised the subject of compatibility. IAdapterManager#getAdapter explicitly states in the API that it does not cause plugins to be activated. We cannot change this fact without breaking the API contract. This is ultimately a minor detail - if we agreed that the current API needed to be changed, we could define some new method and deprecate the old one. Finally, I understand your fundamental problem description that IAdaptable offers getAdapter, but not loadAdapter. Thus clients that want to load adapters have to side-step the adaptable object and go straight to the IAdapterManager API. I agree that this is not ideal - I wish I could have added the loadAdapter method to IAdaptable, but unfortunately that would also be a breaking API change. I don't think your proposal is a good solution to this problem, because it does so by concealing the distinction between "get" and "load" that we want to have available. I think there are two possible mehods to make my proposal better: 1) The ext. pt. interface extension via "force" activation flag could be made granular in the same way as the Java interface of IAdapterManager is, i.e. instead of **one** flag **per extension** one could allow **one** flag per **adapter type**, which essentially reflects the interface of the loadAdapter method. This small variation of my proposal weakens your argument concerning any unwanted plugin activations of individual adapters provided by one single org.eclipse.core.runtime.adapters extension. 2) Indeed my very first idea was to extend the IAdapter interface (of course in some backward-compatible way, e.g. by introduction the extending interface IAdapter2, which provides a loadAdapter method). I withdraw this solution in my OP, because it seemed to me, that actually the IAdapter interface does **not** depend on IAdapterManager (only vice versa), which means that IAdapter is not aware of the org.eclipse.core.runtime.adapters ext. pt. So proposing to add a loadAdapter method to the IAdapter interface seemed as forcing some implementation details of IAdapterManager into IAdapter (This problem does not occur in proposal 1, because extender of the adapter ext. pt. are necessarily aware of IAdapterManager). This is not **necessarily** the only valid point of view: We could consider that concepts of getAdapter and loadAdapter are reasonable for IAdapter **independently** whether any given implementation of this interface would delegate to the AdapterManager or not. If we generally agree in this kind of interpretation it would be of course absolutely fine and even better than proposal (1) in this comment, if IAdapter2 would be chosen to solve this basic problem. Finally I would like to repeat my question whether it would be possible to provide one further overload of IAdapterManager.loadAdapter/hasAdapter, which accept a Class reference (and not a String) for the second adapterType argument? Or should I add one further Bugzilla entry for that? *** Bug 88736 has been marked as a duplicate of this bug. *** Now that I understand the way that the API was intended to work, I'm removing my interest in this discussion. It is somewhat odd, but I think the current design is fine for what we need. Thanks. Since Bug 86362 was fixed with a workaround, I add my vote for this bug. I fully support Daniel's idea of extending adapter extension point with "force" attribute giving extenders a way to define thet their plugin should be loaded whenever their adapter is requested. This is not just the case with the property sheet (which got fixed) extensions but basically every other case of using adapters. In most cases the adapter client does not have to (should not) care weather the plugin gets loaded or not. This should be a matter of the plugin to decide since the plugin is the one providing the adapter and hence the plugin author knows probably best if loading of the plugin is desired or should be avoided. Adapter client only requests the adapter and should already be prepared to handle cases where adapter is not available. *** Bug 97238 has been marked as a duplicate of this bug. *** IMHO, every usage of adapters should activate the plugin if necessary. getAdapter should behave exactly like loadAdapter currently does. A 'force' attribute is better than the current state of affairs, but it is unnecessary since every adapter on an upstream class (every one that could cause plugin activation) would set it to true. > Thus, it was always the case that "getAdapter()" would only function against > the factories that had already been registered (i.e., those belonging to > plugins that are already loaded). The behaviour you observe with the > adapters extension point preserves this old behaviour from previous > releases. This is correct, but for most developers this was problem. In fact, fixing this problem was precisely the reason many of us asked for an adapters extension point to begin with. In Eclipse 2.1, you couldn't register an adapter without your plugin being loaded, so the workaround was to use the early startup extension point to eagerly activate every plugin that contained an adapter... or to create your app as one giant plugin to ensure that adapters show up in the same plugin as the thing they adapt. The adapters extension could have fixed this. Rather than a plugin being active all the time, it would only need to be activated the first time one of its adapters was requested. However, with an adapters extension point that doesn't activate plugins, the problem remains. It is *still* necessary to eagerly activate every plugin that contains an adapter if that adapter is to work correctly. > If I understand your proposal correctly, you want the factory extension > to determine whether loading occurs or not. In this case the caller will > have no choice about whether the plugin providing the factory is loaded. > Some factories would be loaded eagerly, and other factories would not. > The result is that when the client calls IAdapterManager#getAdapter, they > can no longer control whether plugin activations occur. This is true, but it's a good thing. The caller of getAdapter doesn't have enough information to know whether plugin activation should occur. Every time someone writes an adapter, they do so because some of their code depends on the existance of that adapter. They wouldn't have written an adapter otherwise. Regardless of who is asking for the adapter or why, if the adapter doesn't get created, something won't work right. In order for there to exist a getAdapter method that does not activate plugins, there must exist at least one use case where someone would want to implement an adapter even though their app would work just as well if that adapter were not created when requested. I believe that there exists no such use case, which means plugins should *always* be activated if they supply an adapter that has been requested. For reference, the adapter extension point was first introduced in bug 32498 Re: comment 9 I stand corrected. > there must exist at least one use case where someone would want to implement > an adapter even though their app would work just as well if that adapter > were not created when requested. Boris has found such a use-case. A plugin author may be using plugin activation and the existence of adapters as an alternative to contexts and capabilities: they supply an adapter in their plugin and register a bunch of action contributions that use that adapter in their visibility expression. Rather than activate and deactivate a context, they activate and deactivate the plugin when they want the actions to be shown or hidden. Note that it is the plugin supplying the adapter that knows they are using this funky pattern, not the plugin requesting the adapter. This means that the proposed flag is well justified. Although most plugins will never set it to false (since they could have used capabilities to do the same thing more efficiently), the existence of the flag means that old plugins won't break. cc'ing Boris to confirm that I quoted his use-case correctly. My take on this is that a "force" flag would be a good idea, even though it is kind of dangerous. Because it is so easy to force too many bundles to be activated, it would have to be documented fairly well, with a strong recommendation to use an adapter factory implementation class that does *not* cause bundle activation. (I.e. using the 'exceptions' clause in the Eclipse-LazyStart header.) (In reply to comment #12) > using the 'exceptions' clause in the Eclipse-LazyStart header. Could this be enforced? I.e., could the adapter manager load the adapter factory if it is in a package that has been listed in the exceptions clause? Doing this would raise the bar for clients who want to do these kinds of things. At least, they would have to learn about the exceptions clause. Furthermore, we would not need a flag in the extension point schema. Adding my vote for this bug. I have a situation where a view from plugin A is displaying adaptable objects (using BaseWorkbenchContentProvider & WorkbenchLabelProvider). The adapters for this objects are declared in plugin B using the org.eclipse.core.runtime.adapters extension point. Even though plugin A depends on plugin B, I have to force plugin B to activate for my view to work properly. Thankfully I found Daniel's post to the newsgroup before spending tons of time on the problem. I find this behavior very counterintuitive for declarative adapters. *** Bug 199057 has been marked as a duplicate of this bug. *** Is this a 'helpwanted' kind of bug, or do you have plans to address this for 3.4? I'm voting for option (B) - an optional "forceLoad" flag in the core.runtime.adapters extension point. The background and rationale for this is in bug 199057, with a very concrete use-case in bug 197664 comment 6: Some extensions (adapters) might intentionally be invisible while their declaring plugin is not loaded, so it is good that getAdapter() does not get them until some (user) action loaded their plugins. Others extensions (adapters) - particularly those that decorate the UI in some way - need to get activated whenever getAdapter() resolves them. The important point is that it's the EXTENSION which knows whether forced activation makes sense for it or not. That's the only possibility of solving this chicken-and-egg problem without introducing weird dependencies between the framework and the extension. Note that forced activation could still be suppressed via capabilities and pattern matching against the extension id if that is desired. To me, the real problem here is that a developer would not expect getAdapter() to depend on some other factor usually totally hidden from the developer. Once the developer is aware of that, getAdapter() / loadAdapter() should provide all the functionality needed. From this view point it boils to a choice of developer's convenience vs. performance and stability. To me, in this case performance and stability wins; we can't at this time change getAdaper() to activate bundle. On the subject of the "forceLoad" flag. As John pointed in the comment 3, it is usually a consumer that can make a decision on the bundle activation. Does the usecase in the comment 17 means that bundles in, let's say, menus will be presented differently depending on bundle activation? If so, the logical place to make that distinction would be in the menu contributions? The precise use case is this: * We have a tree viewer that can be extended to show items of yet unknown kind. * Those items are contributed via extension point by a non-UI plugin (let's call it subsystem.core) that should be able to also run in headless mode, so it must not have any UI dependencies. * The UI presentation of the items is contributed by a companion UI plugin (let's call it subsystem.ui) that provides the core.runtime.adapters which make the non-ui resources from subsystem.core understandable to the viewer. In terms of plugin loading: * subsystem.core will be activated whenever resources from that subsystem are needed. But it cannot activate subsystem.ui at that time because the UI might not be needed by the consumer. * subsystem.ui should be activated if and only if an adapter for the resources from subsystem.core is needed. * Note that we cannot use loadAdapter() because that would potentially activate too many plugins -- many kinds of "unknown resources" can be contributed by many different plugins. Not all of them should be force activated through loadAdapter(). We really want to be selective, i.e. express the "companionship" between the subsystem.core and subsystem.ui plugins. The viewer cannot do that because it does not know about its extensions. I have not yet found a proper way for implementing this with the current APIs. My thought was that if I tag the core.runtime.adapters extension as "forceLoad" in subsystem.ui it's a safe thing to do since the adaptableType is only declared in subsystem.core; so, when subsystem.core is already loaded it is a safe thing to also load subsystem.ui when getAdapter() resolves my factory. Any ideas how to solve that without a "forceLoad" tag? Or is it a valid use case for adding a "forceLoad" tag? It's true that with a "forceLoad" tag, the getAdapter() call might take longer in terms of performance than expected. But isn't it true that plugin activations might occur throughout Eclipse, from any kind of operation, such that we cannot expect predicatable performance anyways? Note that it's the extender who declares the "forceLoad" tag that's deliberately risking performance impact; and he does so, because he knows that the adaptableType is well-known so the plugin activation will happen only in well-known situations. Martin, thanks for the detailed description. Oleg, can you try to find me later this week to talk about this? Martin, do you think you could use hasAdapter() for the tree viewer in your example? Something like
if (hasAdapter()) {
if (getAdapter() != null)
// use the adapter
else
// use some generic transformation - adapter_info.toString() :-)
}
In general, having "forceActivation" flag just seem to make picture more complicated than it needs to be. With this flag neither consumer nor provider constrol bundle activation and, hence, the result of getAdapter(). Consider a case where there are 2 providers, with only one provider using "forceActivation=true". In this case consumer who uses getAdapter() still has to handle the no-forceActivation case properly.
To me, "forceActivation" flags really becomes behefitial only if all providers set it to "true". In your example, what happens to other non-active bundles - are they going to be missing from the tree view initially and appear after bundle activation?
Let's try to resolve this one way or another for M6 - this enhancement request most certainly has been open long enough :-). There seems to be two questions here: 1) getAdapter() has [unexpected] dependency on bundle activation 2) diffusing control over bundle activation between provider and consumer by having "forceActivation" flag. From my view point: For (1) I'd suggest deprecate getAdapter() and loadAdapter() and create one method instead - getAdapter(..., boolean forceActivation). This will eliminate accidental confusion that, I think, was really a cause of the problems. Additionaly, if it turns to be useful, the getAdapter(..., forceActivation = false) might return some stub that contains basic text information in case bundle is not activated but adapter factory is described in plugin.xml. For (2), to me, it is still not clear how having "forceActivation" flag helps. I agree that it might be a valid workaround if you deal with code consuming your adapters that is not aware of getAdapter()/loadAdapter()/hasAdapter() "details". (And have no control other that code.) However, then everybody else runs into the same problem, they in turn would have to specify "forceActivation" flag - eventually causing getAdapter() to behave exactly as loadAdapter(). If we were to go that way, more direct and more consistent way would be to actually make getAdapter() work just as the loadAdapter() does. Which, from my view point, would get a "+1" any day of the week except that we can't do it without breaking compatibility. Martin, Daniel, Boris - could you attach some code that illustrates a situation then "forceActivation" flag is needed to help understand this? (In reply to comment #22) I cannot see how this suggestion would help the original issue, or my issue. The original issue was that clients try to use IAdaptable#getAdapter() without knowing how it's implemented, and they don't get their adapter in case the adapter factory has not yet been instantiated. Clients need to go to great lengths to force instantiation of the adapter factory, before they can call IAdaptable#getAdapter() safely. The biggest problem here is that forcing instantiation of the factory requires 1.) Knowledge about IAdapterManager where the knowledge about IAdaptable should suffice 2.) Potentially also some knowledge about the factories such that it knows when it should loadAdapter() and when not 3.) Force-instantiation-code in potentially many different places where it should suffice in just one place. Just deprecating the old methods and changing method signatures does not help here at all, since the problem behind this issue would remain the same. A "forceActivate" flag in the Factory, however, would solve the issue. The cost is, admittedly, that those factories which do declare "forceActivate=true" can impact performance of clients which call getAdapter(). My counter argument here is that an implementer of a factory who sets the forceActivate flag should know what he's doing so the effects are not totally unexpected. Especially taking into account that the declarator of the factory does have an idea about how many plugin activations its own activation would potentially cause, implementors can be advised that they should only set "forceActivate=true" when they are reasonably sure that it won't cause a ripple effect, and it's usually better to find other (e.g. interactive, manual) means of plugin activation. Here is my take on this: I think from the point of view of an "adapter consumer", i.e. the code that either calls getAdapter() or loadAdapter(), the decision which method to call is easy: if response time is critical, for example, when pulling down a menu, we need to call getAdapter() instead of loadAdapter() because bundle activation is costly. However, this creates a problem on the "adapter provider" side when what they wanted was providing a menu item for the exact menu that just got pulled down. I don't think we can change getAdapter() to cause bundle activcation unconditionally - the result would be many more bundles that are being activated unnecessarily. Many existing bundles do non-negligible amounts of work when they are activated. What I suggest is a way for the "well behaved" bundles to declare that their adapter factory can be instantiated with little cost and thus should have their adapters being returned from getAdapter() even if the defining bundle is not yet activated. Note that because Equinox allows loading of classes from non-activated bundles (using the 'exceptions' clause in the Eclipse-LazyStart header), this is not really a property of the bundle itself, but rather of the contributed adapter factory. Oleg, does this make sense? As I see it, the use cases that require #getAdapter(..) to return quickly are way minor compared to the other use cases. Frankly, I can only think of the given menu creation example plus eventually some rare use cases in embedded real-time environments. Why don't we create a special API for them which will indicate that a call to #getAdapeter(..) would be expensive? IMHO, leaving the decision on a bundle to declare that it's "well behaving" is not good. Who decides if a bundle is "well behaving"? Most likely its the bundle author and the history clearly shows that this is not the best source for such decisions. I'd rather like to see #getAdapter(..) always activating bundles. Clients which potentially suffer from performance could use the additional API provided to them to find that out and would use a different API. For backwards compatibility reasons #getAdapter(..) can be modified to not activate pre-3.4 bundles. There are other areas where the plugin.xml header is already used to trigger backwards compatibility for existing bundles. Why not here too? > For backwards compatibility reasons #getAdapter(..) can be modified to not
> activate pre-3.4 bundles.
This would not be backwards compatible. The spec of the method says "this method will never cause plug-ins to be loaded", and breaking that contract breaks compatibility. It's also the wrong answer because it would cause activation of a potentially arbitrary chain of bundles that the client may not know about.
I like the idea of "well behaved" bundles being allowed to have a "forceLoad" flag, but I must admit that it's hard to define what's well behaved. A bundle might think by itself that it's well behaved, but then one of its dependent bundles could change with a future version and break that well behavedness... As for my concrete case, I currently have ...getAdapter() all over the place in my UI code. The current situation is that I need to find a proper place for putting the extra ...loadAdapter() call to ensure that the Factories which I need are activated at the right time, and getAdapter() finally works. Finding this proper place is tricky, and - looking at the original description of this bug - not properly possible in the situation where my code calls IAdaptable.getAdapter() on the adaptable object and I do not know what special code that adaptable object performs. Calling IAdapterManager.getAdapter() or IAdapterManager.loadAdapter() just isn't equivalent to IAdaptable.getAdapter() because the adaptable can do things that I do not know [nb: perhaps that method should be deprecated in favor of IAdapterManager.getAdapter()?]. So, what my code needs to do is basically make a guess what adapter factories need to be instantiated by IAdapterManager.loadAdapter(), before it calls IAdaptable.getAdapter(). In the worst case, behavior of the adapter manager and the adaptable are so different that I'm not even able to instantiate the proper factories. That's why I still think a "forceLoad" tag in the factory's plugin.xml, together with API Docs of potential problems and things to observe, is the better way out of this misery. I think the situation is a bit similar to the Plugin#start(BundleContext) method which is described with a whole set of preconditions in its API Docs. Putting too much code in there makes startup slow and problematic, so the code needs to be simple and robust... similar to what one would expect from an adapter factory with a "forceLoad" flag set. (In reply to comment #27) > I like the idea of "well behaved" bundles being allowed to have a "forceLoad" > flag, but I must admit that it's hard to define what's well behaved. It would be good to look at concrete examples, but one potential definition of "well behaved" could be that it does not cause bundle activation at all. (I.e. the factory would have to be in a package that is declared as not causing bundle activation, and it would have to be coded such that it does not make calls outside that package.) (In reply to comment #28) > "well behaved" could be that it does not cause bundle activation at all. (I.e. > the factory would have to be in a package that is declared as not causing > bundle activation, and it would have to be coded such that it does not make > calls outside that package.) Good idea, but is that realistic? Adapters that I've been dealing with contribute the UI Representation for some core model declared in another bundle. That UI Representation consists of images, labels and the like; adapters typically implement interfaces like IPropertySource, IActionFilter, IWorkbenchAdapter, IDeferredWorkbenchAdapter. I'm not an expert in class loading, but to me it looks like in order to return adapter instances, the factory needs to instantiate them. For instantiation, at least all the interface classes that the adapter implements need to be loaded; and these interfaces alone are declared in a lot of different bundles. So while it is very likely that an application which wants my factory to instantiate an adapter has all those UI bundles activated already, it still looks unlikely to me that an adapter factory could really guarantee not to cause any bundle activations? Created attachment 89246 [details]
Patch for new IAdapterManager.loadAdapter(Object, Class) API
Attached is a potential solution to the issue, making a backward compatible API extension for IAdapterManager.loadAdapter(Object, Class).
I came across this issue again through a traceback with a suprious NPE after IAdaptable.getAdapter() that resulted exactly from the fact that an adapter factory had not yet been loaded because of a rare timing issue in plugin activation.
When analyzing the code and searching for a potential fix, I found that I'd potentially need the same utility code over and over again:
* query adaptable.getAdapter()
* if it returns null, try IAdapterManager.loadAdapter()
* then query adaptable.getAdapter() again
This is kind of a hybrid solution to the original problem that interestingly no one has thought of so far. It is not perfect, we do not know exactly how adaptable.getAdapter() works for a concrete case; but it's a "best effort guess" for the most typical case where it just relays to the adapter manager.
Moreover, in the case that an implementer of adaptable.getAdapter() overrides the default behavior to do some custom adapter finding, he can still ensure that plugins get activated through my new API if he registers an additional rogue adapter factory in his plugin.xml, that does nothing but ensure the right plugins are loaded to support his custom adaptable mechanism.
This solution always leaves control of plugin loading where it currently is: with the code that queries the adapter. It also does not change performance behavior in any way, since only clients of the new API need to be prepared for performance impact. It provides a possible solution for the original issue that IAdaptable.getAdapter() never loads plugins, but without the extra hassle of inventing IAdaptable2 and with a simple backward compatible change because IAdapterManager is not intended to be implemented by clients.
To utilize the new API, client code needs to be changed from
adaptable.getAdapter(aClass);
into
Platform.getAdapterManager().loadAdapter(adaptable, aClass);
in those cases where plugin activation is potentially desired.
Please review and let me know what you think - if Daniel, the original submitter of the bug is still around I'd especially like to hear his opinion.
Note that in case this patch is not accepted and IAdapterManager is not changed, clients always have the chance to use exactly the same code in their own plugins. Putting it into IAdapterManager, however, could help reducing unnecessary code duplication by providing this common utility in a common place - and making everybody aware of this potential solution.
Legal Message: I, Martin Oberhuber, declare that I developed attached code from scratch, without referencing any 3rd party materials except material licensed under the EPL. I am authorized by my employer to make this contribution under the EPL.
PS note that in the special case where the adaptable object in question already is an instance of the adapter in question, my new code does NOT lead to any plugin activation or factory loading.
This is because the adaptable is asked first, and in this case it can be written to already return the adapter -- unlike the existing IAdapterManager.loadAdapter(Object, String).
class MyClass implements IAdaptable, IMyAdapterType {
public Object getAdapter(Class adapterType) {
if (adapterType.isInstance(this) {
return this;
}
return Platform.getAdapterManager().getAdapter(this, adapterType);
}
}
This bug has a target milestone of 3.4 M6 ... does this mean that my patch is going to be considered? Or is the target milestone wrong? The target milestone was set by Oleg on 2008-02-05, see comment #22. Martin, it would be the easiest solution for me to apply your patch and close this as fixed. Problem is, the patch does not solve the bundle activation issue (rather makes it more complicated and involves IAdaptable implementations?) and would introduce inconsistency in the getAdapter() / loadAdapter() methods (after the patch loadAdapter(Object, Class) would behave differently from loadAdapter(Object, String)). Despite my efforts to find a common ground to how to resolve this enhancement request, there does not appear to be a solution that would be satisfactory to everybody. I've actually made a digest for myself of opinions people expressed in this enhancement request; briefly discussed this on two Equinox meetings, and bugged several people trying to understand their view points. The summary, as I see it, is that this is the case where people would like to "have the cake and eat it". Form the adapter side, if you'd like to use the adapter functionality, you need to activate a bundle. It really boils down to the fact that if you use a Java class, you need to load it. If you need an adapter, use loadAdapter(). If you can do basic work without it, and use it if available, use getAdapter(). [Insert rumbling here on the rather unfortunate - from my view - behavior of getAdapter() vs loadAdpater() :-).] So far, looking just from the adapter side, it makes sense? The problem comes when adapters are used in time-sensentive operations, like UI context menus. In this case you really want: (1) methods to work fast; (2) methods to do some work to determine if this is a selection that can be adapted to, let's say, debug target. Just like eating the cake and having it too. And, i believe, it is because of this contradiction that we have so different opinions as to what to do on this issue. As discussed above, "forceActivation" flag can not be made "safe" in principle for real-world adapters. (Dependent bundles might need to be activated.) The suggestion to modify getAdapter() to activate bundles was rejected due to scalability concerns. As such, at present, there does not appear be any suitable way to implement this enhancement request. I am closing this enhancement request as "invalid" for a lack of a better term. Ok. So I guess the bottom line of this resolution is you're saying: "You'll need to find your own ways of activating your UI bundles at the right time. There's no way the adapter manager can help you activating your bundle. Find the right place to do the activation, and write the activation code yourself. Once your stuff is activated, use getAdapter()." Which is a valid approach, since bundle activation is a potentially time consuming and sensitive issue, which cannot be solved with one overall solution easily, trying to accomodate both speed, safety and deterministic behavior as well as lazy loading as well as having the stuff available when needed. That being said, I guess what I'm looking for is experiences, expertise or some best practices that various people have found as to when and where to best activate the bundles. Any suggestions or pointers where I could start looking for that? Whish I knew the answer to those questions… :-) Speaking in general, Eclipse tends to heavily shift towards lazy activation. The base Eclipse SDK has 100+ bundles; it starts in a few seconds. The largest Eclipse-based application I heard about ships on 11 CDs and still starts in a few seconds – thanks to the lazy activation approach. Speaking more concrete, it is possible to have two situations: you control the code that gets adapter, or you use a code that you can not (or don’t want to) modify that uses adapters. If it is your code – you can decide when to use getAdapter(), when to use loadAdapter(). For instance, in the example of the tree viewer you described above, you could modify view’s content provider to use loadAdapter() for things that are to be adapted to the classes you really care about and getAdapter() for the rest. Or just use loadAdapter() if that is what’s needed. If this is the code that you can not modify, the options are limited. In all honesty, the most practical approach at that point would be to just accept it that this is the way it is. Chances are it will not be the end of the world. If it is the end of the world, you can consider using the early activation extension point “org.eclipse.ui.startup”. This is not a solution I’d recommend until all other avenues were exhausted. (In fact, using this point for general bundles is highly discouraged.) Also, keep in mind that it can be turned off by the user (Prefrences -> General -> Startup and Shutdown) If you find that you need to programmatically activate a bundle, use Bundle.start(START_TRANSIENT). This way bundle’s start state won’t be cached in the configuration. That’s about all I know; I would be interested too to hear answers or opinions on those question. It's definitely a fundamental problem that we have wrestled with since the earliest days of the platform. There is no easy answer. Plug-in developers typically have the vantage point where they believe their plug-in is fundamentally important, and deserves to be started immediately or based on the most subtle trigger from the end user. They want the end user to have the best possible experience with their plug-in, which is completely understandable. Giving the best experience usually requires activation so that, for example, commands appear in the right places at the right time, and are correctly enabled/disabled at all times. Or, activation so that the plug-in's model of the world can be kept up to date by hooking various listeners.
However, this thinking doesn't scale when you consider a product that ships on 11 CDs, as Oleg mentioned, or even for the Europa release train. I may have all kinds of things installed in my system that I want to have available when I need them, but want to have no impact on the system when not needed. This requires plug-ins to err on the side of non-activation. When you don't know for sure that the user wants your plug-in's capabilities, you should remain inactive. Sometimes this means a slightly degraded user experience, such as commands that are enabled, but when clicked will say they are not applicable. When the trade-off is between a slightly degraded experience when a plug-in is inactive, and eagerly activating a plug-in when it is not needed, we need to err on the side of not activating. This is ultimately why this bug was marked INVALID. The plug-in providing an adapter factory doesn't know the context in which the call is being made, and thus can't decide whether activation is warranted or not. Only the caller can decide that, and they have the getAdapter and loadAdapter methods available so they can make the decision. Adding a force ("I believe I'm important") flag to the factory is likely to result in plug-in activations when not needed.
FYI, I think I found good solutions for my current issues with this. The trick is really to find the right places where
Platform.getAdapterManager().loadAdapter()
is to be called. I'm thankful that you Platform guys have been so restrictive with this, since I believe that the solution I have now (which uses my knowledge about when and where it's right to activate bundles) is better than an "over-all" kind of solution where bundles might be activated too eagerly.
FYI, there are few places in my code where I'm doing this (preferredly not in places where I'm in the UI thread or about to show a menu, but there are other places where I can nicely do this from a Job):
IMyAdapter adapter = (IMyAdapter)myObj.getAdapter(IMyAdapter.class);
if (adapter == null) {
//Give it a 2nd chance after potentially loading the bundle
Platform.getAdapterManager().loadAdapter(myObj, IMyAdapter.class.getName());
adapter = (IMyAdapter)myObj.getAdapter(IMyAdapter.class);
}
That's basically the boilerplate code that I had suggested putting into a new IAdapterManager API method, but actually I see it's small enough and works equally well as boilerplate in my own code (duplicated few times). It's really not that bad in the end and I'm happy with the solution.
> The plug-in providing an adapter factory doesn't know the context in > which the call is being made, and thus can't decide whether activation > is warranted or not. They know the reason the adapter factory was written and the consequences of having that adapter or not having that adapter. The guy writing the properties view may be think "hey, it's not the end of the world if you can't see the properties of a particular object when you select it". Assume he doesn't force plugin activation when requesting property pages. However, the guy writing the EMF model editor knows "my editor is totally unusable unless the user can edit the object properties in the properties view". Shipping an unusable editor is not an option... so the editor author either needs to force plugin activation for the plugin containing the property page adapters or needs to put the page adapters in the same plugin as the editor itself. > Only the caller can decide that, and they have the getAdapter and > loadAdapter methods available so they can make the decision. Adding a > force ("I believe I'm important") flag to the factory is likely to result > in plug-in activations when not needed. I disagree. In every Eclipse-based product I've ever worked on, we've had to work around this issue by putting all adapter code (and anything it depends on) in a plugin that would be eagerly activated on startup - either by using the early startup extension point or by putting the entire UI in one big plugin (if you put enough UI code in one plugin, it gets activated anyway due to editors and views). Any workaround to this problem results in significantly more plugin activation that we'd get by just activating it when needed. Just to be clear: not having the adapter is usually not an option, since it would introduce a bug. Without this feature, clients MUST work around it by forcing plugin activation... and it makes no sense to try to justify the current behavior by saying it would reduce plugin activation, when the consequence of that behavior is workarounds which cause additional plugin activation. (In reply to comment #38) > However, the guy writing the EMF model editor knows "my editor is totally > unusable unless the user can edit the object properties in the properties > view". I really like plug-in authors thinking that way. Their stuff is really always the most important one. They rule the workbench. Once again, if *I* install your plug-in into *my* workbench *I* don't care if all properties are shown until *I* really want to work with your stuff (either by activating your views or editors). As you said, in reality this isn't a problem at all. If I select your object, then this will happen verily likely in a view which presents those objects. At that time, the UI plug-in contributing that view already has been loaded together with the adapters, etc. Truth is, if I as a plug-in writer *really* care about such a fine granularity which ships my adapter contributions in a separate bundle, then I will also care that I'm a really good citizens in *any* workbench and that I will honor the plug-in activation rules anticipated in that particular application. However, if I ship my very own application, then I will be able to implement my very own bundle activation policy. As long as the above can't guaranteed I'm strongly against allowing any contributor to force activation of itself. We already have a big hole with the early startup extension but at least that's easy to disable in the preferences UI or via extension transformations. Re: comments 38 and 39 It seems to me that we need a concrete example so that we are discussing the same thing. Ideally, it would be a small RCP app with a minimal number of separate plugins with which the issue can be reproduced. Stefan, I can do the work but could use your help with setting it up correctly. Maybe some time today or tomorrow? > I really like plug-in authors thinking that way. Their stuff is really
> always the most important one. They rule the workbench.
I'm not suggesting anybody's code is "most important". I'm suggesting that *everyone's* code should work correctly.
My premise:
No code should throw unhandled exceptions. No code should have bits of UI that mysteriously disappear, be they actions, property pages, etc. No action should ever fail to perform its intended function because some plugin is inactive. The end user should never be aware of which plugins are active or inactive.
My argument:
If we ever fail to load an adapter when its plugin is active, there will be situations where things mysteriously disappear, actions don't work, etc.
My conclusion:
It is necessary to activate a plugin if that's what's necessary to ensure that its adapters are created when requested.
Your comments seemed sarcastic, but I'm not sure where the disagreement arose from. Are you disagreeing with my premise or my argument? Or are you suggesting that the conclusion does not follow from the argument?
Note: there is a case where my premise is untrue. In a few cases, the JDT team has been using plugin activation like a context. That is, rather than using a context to explicitly control the visibility of certain actions, they tie the visibility of those actions to the activation of a plugin by registering the action on an adapter that is declared in that plugin.
In this very specific case, the activation of that plugin is meant to be visible to the end user, and the design of the plugin would be built around it. That is, it would not contain any adapter declarations that are required by other plugins, or where the failure to obtain an adapter would result in exceptions or random weirdness. I acknowledge that in this case, the developers would not want their plugin activated in order to create an adapter. This is not the only case nor the most common case, but it does justify the suggestion here of having an opt-in flag for plugin activation rather than activating in all cases.
It looks like we're all on the same page that we want to avoid plugin activation wherever possible (if that were not the case, we could simply create a plugin that force-loads the adapters in question). So, we are really talking about stuff here that's contributed by means of XML markup (actions, commands, ... anything else)? - stuff that gets visible as a control in the UI without activating a plugin, but should be smart enough to activate the plugin when the user uses that UI metaphor. Perhaps what we really need need is more veratile command handlers / action proxies to perform adapter loading? What do you think about my comment #37? For our project, I finally moved away from this mechanism because I understand that it's risky to have bundles activated at unforseeable points in time. I finally found the proper points in my code which are meant to "initiate" previously unavailable functionality, and added loadAdapter() there. I understand that this may not be possible in cases where you don't have control over the code that has the getAdapter() -- hence the question of probably needing enhanced action / command handlers. > If we ever fail to load an adapter when its plugin is active That should read "If we ever fail to load an adapter when its plugin is INACTIVE". I don't intend this comment for anyone in particular, but I'm having trouble working out why there is disagreement here. The argument for activating for adapters seems to have gone like this: Pro: "We need the adapter for our code to work" Con: "Activating a plugin to load the adapter is slow" Pro: "Not creating the adapter results in bugs." Con: "I don't think the bugs would really be that serious. I've only ever seen minor issues, and users can live with that." Pro: "That's because everyone who encountered the really severe bugs forced plugin activation or lumped everything into one huge plugin as a workaround!" Con: "You shouldn't ever force plugin activation! That has horrible consequences for startup time!" Pro: "The only alternatives are to activate the plugin when necessary to create adapters, be badly behaved about early startup, or ship with bugs. Which are you endorsing?" Con: "I refuse to answer the question directly no matter how many times it's posed. However, I'll vaguely imply that the bugs aren't that severe and hope that you'll go away". Pro: "The bugs ARE severe. We tried that approach. Our users and managers noticed them and asked for them to be fixed. That leaves us with us with either adding this enhancement (which is a bit slower in certain specific cases) or doing nasty workarounds (which are very slow in every case)." The argument against loading adapters seems to hinge on the assumption that all the code would still work if we weren't to activate plugins to load adapters or resort to nasty early activation hacks... and this assumption is wrong. Martin: comment 37 seems like a good way to request adapters, but some additional optimizations are possible. Check out bug 118560, comment 3 for an alternative. However, this particular bug is about cases where the caller is using getAdapter and the author of the adapter itself has no way to change this code. (In reply to comment #39) > I really like plug-in authors thinking that way. Their stuff is really always > the most important one. They rule the workbench. > Yeah. Starting out with a sarcasm is really great way to convince people of your point of view ... :) > Once again, if *I* install your plug-in into *my* workbench *I* don't care if > all properties are shown until *I* really want to work with your stuff (either > by activating your views or editors). > You are assuming that the plug-in adds more than property sheet adapters. I had a use case where I needed to add property editors for an object selected in another view (e.g. structure navigator). The plug-in contained only the extension for the property sheet precisely or the reason that it would have minimal possible impact on the bundle loading, having minimal dependencies possible than bundling the property sheet with the rest of the code. You are basically suggesting playing nanny for all the developers out there for the sake of some mythical "irresponsible pogrammers", and denying all the responsible ones a chance to optimize their code load based on the usage scenarios only the application developer knows. You also assume probably that all extension developers develop their extensions for the Eclipse IDE and because in IDE you do some things in some specific ways, you assume that you know the use cases where your component is being used in. I, for one, am developing an RCP app that just happens to reuse some of the bits from IDE (Common Navigator, Properties view, etc.) and my usage scenarios do not really match Eclipse IDE scenarios. (In reply to comment #44) > I, for one, am developing an RCP app that just happens to reuse some of the > bits from IDE (Common Navigator, Properties view, etc.) and my usage scenarios > do not really match Eclipse IDE scenarios. But that's fundamentally different. You can implement your own activation story in your RCP app. However, if you plug-ins are deployed in an IDE or in an app outside of your RCP app, you should respect this app. (In reply to comment #45) > But that's fundamentally different. You can implement your own activation story > in your RCP app. However, if you plug-ins are deployed in an IDE or in an app > outside of your RCP app, you should respect this app. > So what you're suggesting is that Properties view (just an example here) is only to be used in IDE and not in RCP? And if I need my "version" of Properties view, I should just blatantly copy the implementation (and thus abandon any hope to gain from future improvements)? I've done this on several occasions, where either Eclipse framework fell short for me and ether due to some time constraints or similar resistance to change the code of core Eclipse, I could not just improve the core platform and build from that... <RANT>Now about 2 releases later I am faced with an extremely painful migration from 3.2 code base (which actually started out as 3.1 platform) to 3.4, because some of those essentially duplicated pieces of functionality have started to live their own lives and instead of building upon Eclipse platform, I've ended up building my own. I want the improvements, but I want my features as well...</RANT> (In reply to comment #46) > (In reply to comment #45) > > But that's fundamentally different. You can implement your own activation story > > in your RCP app. However, if you plug-ins are deployed in an IDE or in an app > > outside of your RCP app, you should respect this app. > > > > So what you're suggesting is that Properties view (just an example here) is > only to be used in IDE and not in RCP? > > And if I need my "version" of Properties view, I should just blatantly copy the > implementation (and thus abandon any hope to gain from future improvements)? I think Gunnar's point here is in an RCP app you control your activation story. You can activate the problemmatic plugins on startup (in osgi.bundles or though the OSGi programmatic framework). Of course you can use the Properties view ... and start the appropriate plugin if the lazy loading strategy used by the eclipse IDE is not to your liking. PW (In reply to comment #44) > You are assuming that the plug-in adds more than property sheet adapters. > I had a use case where I needed to add property editors for an object selected > in another view (e.g. structure navigator). > > The plug-in contained only the extension for the property sheet precisely or > the reason that it would have minimal possible impact on the bundle loading, > having minimal dependencies possible than bundling the property sheet with the > rest of the code. Thanks Roland, this is exactly what I was looking for - a concrete example showing that when you split your code into smaller plug-ins, the lazy loading strategy has unwanted consequences. The fact that we do not experience this as a problem in the IDE does not mean that there is no problem. (In reply to comment #41) > My premise: > > No code should throw unhandled exceptions. No code should have bits of UI that > mysteriously disappear, be they actions, property pages, etc. No action should > ever fail to perform its intended function because some plugin is inactive. The > end user should never be aware of which plugins are active or inactive. As mentioned in John's comment #36 paragraph 2, we've chosen a lazy loading policy in eclipse as an eager activation policy is not scalable. Our policy recognizes that there is a degradation of the user experience, but providing that certain rules are followed it should be predictable (we don't always follow the rules). One example is actions that appear enabled. The user clicks, we load plugins, and they get a popup that tells them that operation is unavailable (and from then it is disabled). It's not that eclipse didn't think about issues like "The end user should never be aware of which plugins are active or inactive." It's just that the pain of not having the lazy loading in place *far* outweighed it. I think it's still worth pursuing optimizations to reduce the degradation of the user experience, but it was an acceptable risk when this policy was decided. PW (In reply to comment #46) > So what you're suggesting is that Properties view (just an example here) is > only to be used in IDE and not in RCP? Roland, that is a concrete example and very helpful indeed. IMHO, in this case the properties view should be responsible for loading adapters but not the adapter. You should be able to use the properties view in RCP. The behavior could be implemented in a configurable way so that activation is triggered by default in RCP apps but the current behavior is preserver in the SDK (if it seems necessary). > One example is actions that appear enabled. The user clicks, we load > plugins, and they get a popup that tells them that operation is > unavailable (and from then it is disabled). I understand you think this is acceptable, but most Eclipse-provided actions don't exhibit this behavior. In the Eclipse UI, most actions are lumped into huge plugins (like org.eclipse.ui.views) that also contain views and editors - often the same views that contain the actions. That means that you'll rarely have cases where a action is visible without its plugin being loaded. If you try to break your plugins up into much finer grain (say one action, view, or editor per plugin), then you'll start to see a lot of customer complaints about this. I've tried it. The complaints happened. Ultimately, it is the developer who created the action that will have to deal with the customer complaints when the customer sees this dialog... and if enough paying customers ask for a bug to get fixed, that bug must be fixed - regardless of whether the Eclipse platform developers consider it to be an acceptable limitation. I have the greatest respect for the Eclipse platform delevopers. But no matter how intelligent and talented they may be, they are not the people who get to decide what is an acceptable bug in a downstream plugin. It is the users and maintainers of that plugin. A bug is acceptable if and only if no paying customer cares about it... and only the maintainer of that plugin knows what their customers are complaining about. As such a maintainer, I'm trying to pass the information along. Our users (and managers) have spoken - this sort of thing is not acceptable. It's fine for Eclipse to decide that this enablement thing is acceptable for its own actions. The structure of the Eclipse UI code makes the problem rare and it is the Eclipse UI team who receives the customer complaints when it goes badly. However, it's not fine for Eclipse to impose the bug on actions in downstream plugins, where the problem may be more common and the complaints go to someone else. In this case, we're speaking of activation for adapter creation rather than activation for enablement processing (FYI, I think we could fix the enablement thing without *any* plugin activation - but that's starting to drift too far into the analogy)... but the analogy holds. It's fine for the Eclipse core team to say "my adapters can be omitted if it would cause plugin activation"... but it's not fine for Eclipse to impose this limitation on downstream plugins, where the consequences may be more severe and the the resulting bugs go to someone else. > You can activate the problemmatic plugins on startup (in osgi.bundles > or though the OSGi programmatic framework). In this case, the problematic plugins include all plugins that extend the adapters extension point... which is a lot. If I understand this correctly, you seem to be endorsing the idea of eagerly activating a large number of plugins on startup. I hope we're all on the same page here - we're trying to minimize the number and size of activated plugins. If so, the use of early startup does not seem to be a desirable solution to this problem (even though it is currently the only one we have). re: comment 50 > IMHO, in this case the properties view should be responsible for > loading adapters This is true. You could easily fix this by making the properties view call loadAdapter instead of getAdapter(). However, the same will be true of all usages of getAdapter(). I could help by offering up some additional examples and one-by-one we could observe that "that could be fixed if the caller used loadAdapter instead of getAdapter"... but before I start this process, at what point will we decide that there is no remaining reason to call getAdapter instead of loadAdapter? > I think it's still worth pursuing optimizations to reduce the
> degradation of the user experience, but it was an acceptable risk
> when this policy was decided.
I guess my point is that the Eclipse platform doesn't have the authority to decide what is an acceptable level of degraded user experence in a downstream plugin or RCP app.
Eclipse-as-an-IDE has every right do decide how its own UI bits should look and feel.
However, Eclipse-as-a-platform is merely there to provide power and flexibility to us downstream RCP and plugin developers. It is our decision to make the tradeoffs between performance and user experience in our own UI bits, since it is us who will be answerable to our customers if we make the wrong decision.
I would like to stress again that while the intention is commendable, the reality shows that this restriction causes exactly the opposite effect. My belief is that this is an unnecessary "nanny restriction". In the sense that it is only designed to protect the platform from few misbehaving citizens while making life unnecessarily complex for every well behaving developer. Also, all of the workarounds suggested here, make things actually worse by causing either excessively eager loading (osgi start level hack); creating massive all-inclusive plug-ins (usual practice) that are much more expensive to load, or indiscriminately loading every bundle via loadAdapter(Object, Class) regardless if it has been designed to be considered essential functionality or is an optional add-on feature... The "bad guys" will misbehave no matter what kind of restrictions you set up - they can and will use the same kind of workarounds that the rest of the developers. It is the "good guys", you need to cater for. Give them (us?) the tools they can use for the good of the platform and they will make the platform shine ;) (In reply to comment #52) > However, Eclipse-as-a-platform is merely there to provide power and flexibility > to us downstream RCP and plugin developers. It is our decision to make the > tradeoffs between performance and user experience in our own UI bits, since it > is us who will be answerable to our customers if we make the wrong decision. And you are ... simply activate all of the plugins that you need to. This is do-able today, but I think your point is that this is still a work around to a situation and not a solution to that situation. I can see a failure mode in splitting the plugins in that if some of the classes that need to be adapted have UI dependencies, you would still have to activate UI plugins to get the desired behaviour in an RCP app. PW (In reply to comment #54) > I can see a failure mode in > splitting the plugins in that if some of the classes that need to be adapted > have UI dependencies, you would still have to activate UI plugins to get the > desired behaviour in an RCP app. True, but that final decision is now ours to make - if we see a way to get rid of that extra dependency, we can do it. If we can not, there are still probably ways to minimize the impact by leaving only the most essential of the dependencies. If there is nothing else we can do the effect is no worse than it is currently, where we bundle everything (UI and model classes and adapter factories and whatnot) together in a single bundle. (However, I'd argue that in this case there must be something wrong with the overall design of the architecture) (In reply to comment #55) > True, but that final decision is now ours to make - if we see a way to get rid > of that extra dependency, we can do it. If we can not, there are still probably > ways to minimize the impact by leaving only the most essential of the > dependencies. Sorry, my "failure mode" was w.r.t. the current work around forcing activation of a UI plugin to provide the adapter on the off chance it might be used. PW Paul said:
> And you are ... simply activate all of the plugins that you need to.
Yes, that's a good observation and this workaround works quite well. However, the point of this enhancement is to let us replace this workaround (which activates our plugins 100% of the time) with a smarter adapter manager.
Your point - while perfectly valid - is not an argument against this enhancement. It's just an observation of the circumstances that have made this enhancement desirable.
Some observations: 1. We are really scared of changing semantics of IAdapterManager#getAdapter() to be able and load plugins under certain circumstances. The fear comes from potentially introducing unexpected load time and (what's worse) unexpected threading issues/dependencies. I tend to agree that changing the known semantics of getAdapter() is not an option. 2. There is some discussion about who owns plugin activation. There are some very valid points that plugin writers need to own plugin activation at times. Other arguments were about creators of products owning plugin activation. 3. There were some examples identified where extenders need to get adapters loaded without (currently) being able to influence the code that currently uses getAdapter() but would need to use loadAdapter(). The Properties View was such an example. I'm wondering whether it might help to introduce something similar to the Capabilities extension point: Capabilities allow declaring patterns of extension ID's that can be used to hide the respective elements. What if a similar extension point were added, e.g. org.eclipse.ui.adapterLoaders which allows specifying patterns of extension ID's that should perform IAdapterManager#loadAdapter() instead of IAdapterManager#getAdapter() That would allow creators of RCPs or Products to influence adapter loading even for views etc that are outside their control. At the same time, it would limit the execution of the loadAdapter() path to few well-defined well-known places and thus avoid the risk mentioned in (1) above. For another use-case, see bug 32498, comment 13: > As a somewhat contrived scenario, imagine that in my TextSummary plugin, > I register an IOutlinePage adapter on regular text editors. When I open > a regular text editor, the Outline view will do > editor.getAdapter(IOutlinePage.class). It should find the one registered > by the TextSummary plugin, activating it if necessary. This was one of the comments from the bug report where the extension point was originally created. > We are really scared of changing semantics of IAdapterManager#getAdapter() > to be able and load plugins under certain circumstances. Activating plugins in getAdapter is not a change in getAdapter's contract. Since classloading can cause plugin activation, it has always been the case that getAdapter can activate plugins. Any implementation of an adapter factory can (and many do) invoke code such as loadAdapter that can cause plugin activation. > The fear comes from potentially introducing unexpected load time and > (what's worse) unexpected threading issues/dependencies. It would be helpful if you could describe specifically which threading issues you are concerned about. I'm not sure if an extra extension point is the best way to go. Most of the callers of getAdapter are not directly processing an extension point, so it would be hard to identify them. Even if they were, there would inevitably be at least one user of each extension point that needs their adapter loaded - so this would just be a complicated way of replacing all calls of getAdapter with loadAdapter. Not that I think this is a bad thing - but if that's where we're going anyway, we may as well just make getAdapter load the adapters without the extra extension point. Re-reading my comments here, I realize I've been pushing on this issue far too hard and probably offended the very people who have the ability to fix this. There's no way that this can lead to a productive outcome. I'd like to apologize to everyone here for the aggressive tone of my previous comments. In the interest of allowing others the chance to conduct a more level-headed discussion, I will excuse myself. Mike W, thank you for bringing this up with me. I am always hitting this, what I consider a serious bug, again and again in my RCP apps. Especially when combined with using expressions. If I want my command handler being active (or menu item visible) via expressions than I would use the <adapts> expression to check if the selection can adapt to a certain class. However, the adapts expression implementation uses the faulty getAdapter and not hasAdapter (to prevent activation) to check for the adaptability of the selected object. This results in handlers not being active/menu items not being visible despite the fact that there would be an adapter available. I believe the getAdapter method should do what it is supposed to do by its name. It should get me an adapter. And if that means to activate the plugin that is providing the adapter, so be it. If the caller has only interest in the existence of an adapter, it has to call hasAdapter. There is no need for a loadAdapter at all. This is especially annoying with the adapters extension point. As several described here, the most common use case is for adapters to be provided in seperate bundles, not even the same bundle that provides the views. And adapter should be also available before any view is opened. How else would you then get an IWorkbenchAdapter to display my own object in a workbench view? I have given out the policy to my colleagues not to use the adapter manager extension anymore, and have implemented an IAdapterManager extender that consumes OSGi services exporting IAdapterFactory and registers them with the IAdapterManager. The applications start in a snap. Plugin activation is not an issue. Whats left to fix is the faulty behaviour of the <adapt> expression. I think I have filed a bug for that last year. (In reply to comment #61) > Whats left to fix is the faulty behaviour of the <adapt> expression. I > think I have filed a bug for that last year. Looks like you don't have any issues with IAdapterManager but the <adapt> expression doesn't use it correclty. IAdapterManager#hasAdapter won't activate plug-ins, at least that's what the contract says. Do you have a reference to the bug? Did you also submit a patch for it? I do not have a bug reference yet, I have to search for it. Or maybe I only discussed it with Paul Webster on IRC. I just check the adapt Expression however. And it uses getAdapter (which I still think operates against it's name) but if getAdapter returns null it checks with the adapter manager again whats the state of the available adapter. If its plugin is not loaded, then the adapt expression returns "EvaluationResult.NOT_LOADED" and canceling any further expression evaluation by that. That's correct behaviour in part of the adapt expression though. Because it has to assume, that if it would evaluate to TRUE its expression without getting an adapter using getAdapter(), others that might call getAdapter() would also see null as the result. It cannot assume anyone would call loadAdapter().
So for example take a handler that gets only activated if a certain object adapts and the adapt expression would just only check using hasAdapter without trying to getAdapter(). Inside the handlers execute() method one would call getAdapter() and would get returned null. Why is the handler active then?
The problem also lays in the contract of getAdapter(). Even if you say you can provide an adapter for a certain object, you do not have to in the end. Depending on the state of the adaptableObject you can still decide not to provide an adapter. A simple example is a Deletable adapter. Lets say you have bunch of object in the system that can be deleted. A menuContribution would use the <adapt> expression to toggle its visibility based on the possibility of being able to delete one or all of the selected objects.
It would try to adapt the objects in the selection to Deletable. However, the IAdapterFactory could decide that certain objects are not deletable, because maybe, you do not have the rights to delete them, or the state of the object does not allow it to be deleted:
Object getAdapter(Object adaptableObject, Class adapterType) {
if (adapterType == Deletable.class && adaptableObject instance of IFile) {
if ((IFile)adaptableObject).isDirectory()) {
return null;
} else {
return new Deletable() {
void delete() {
((IFile)adaptableObject).delete();
}
}
}
}
}
So in reality the adapt expression would have indeed to allow the actual factory to perform the adaptation completely. I see the problem in that. It can be an expensive process. But let's assume that in the adapt expression it would just check for the possibility of the Deletable adapter to be used (hasAdapter()) then I would expect the handler to just call getAdapter with all objects in the selection. And not loadAdapter. Because if I want the adapter I have to live with possible plugin instantiation.
Using my OSGi AdapterManager extender solves this problem. It activates the bundle when the adapter factory is requested. It did not degrade our apps performances. What it did was to let our apps just perform in a way expected without tweaking any start levels or start states of individual bundles.
Don't get me wrong. The adapter pattern is a great piece of technology just the implementation has this one flaw that makes it very hard to use and predict.
I would vote to deprecate the getAdapter() method. The Platform cannot play Nanny for irresponsible plugin programmers.
(In reply to comment #63) > So for example take a handler that gets only activated if a certain object > adapts and the adapt expression would just only check using hasAdapter without > trying to getAdapter(). Inside the handlers execute() method one would call > getAdapter() and would get returned null. Why is the handler active then? In your example you outlined a very specific use-case which is simply not supported by the current <adapt> expression implementation. There are probably some very good reasons why the current implementation doesn't support this use case. Maybe the <adapt> expression needs some better documentation around this? BTW, you could write your own expression evaluator which simply activates the plug-in during it's evaluation expression. But even that doesn't solve the problem because you run in a dynamic environment. This environment actually makes it possible that the plug-in contributing the adapter factory _may_ be deactivated between the expression evaluation and the handler invocation. You have no control over this. That's OSGi. > I would vote to deprecate the getAdapter() method. The Platform cannot play > Nanny for irresponsible plugin programmers. So, other programmers don't close JDBC connections properly or don't release SWT resources properly. Should we deprecate #dispose() or #close() methods too? Simply put, #getAdapter may return null. That's the contract. If you don't deal with it, you have to live with the consequences. One a side note, I already proposed a while back that one solution should be to make it configurable. This could be a setting (preference, system property, etc.) which controls the behavior. RCP apps which run independent may prefer an implementation which transparently activates plug-in in #getAdapter calls. It may solve their issues. However, IMHO this does not simplify the life of plug-in developers. They must stick to the current contract if they want to be a good citizen in the IDE. Another interesting path is the one Boris proposed in comment 13. Also, the summary of this bug is somewhat misleading. It's not the AdapterManager that is behaving wrong. It's actually the combination of the AdapterManager with the extension registry. This piece is decoupled from the adapter manager implementation and only works when the extension registry is in place. Anybody interested in looking into AdapterFactoryProxy#loadFactory to try implementing Boris's proposal? (In reply to comment #64) > In your example you outlined a very specific use-case which is simply not > supported by the current <adapt> expression implementation. There are probably > some very good reasons why the current implementation doesn't support this use > case. Maybe the <adapt> expression needs some better documentation around this? I somehow fail to see the reasons why it would not evaluate to TRUE if the bundle providing the adapter is not loaded. Maybe Paul can shed some light on this? > BTW, you could write your own expression evaluator which simply activates the > plug-in during it's evaluation expression. I would like to. How do I extend the expression extension? I think that's not possible, is it? > But even that doesn't solve the > problem because you run in a dynamic environment. This environment actually > makes it possible that the plug-in contributing the adapter factory _may_ be > deactivated between the expression evaluation and the handler invocation. You > have no control over this. That's OSGi. That's true indeed. And I already handle that situation in all my handlers. I check the return result from getAdapter(). Well, right now I have converted all getAdapter() to loadAdapter() since the getAdapter() method does not what its name suggest. Call it bad API design. Its not enough to mention it on the API doc, that the method will not return an adapter just because the bundle providing the adapter is not loaded. If you just want to know if a adapter would be available you would call hasAdapter(). There is no need for all 3 methods. I think thats also the main complain here (and in other bugs). > > I would vote to deprecate the getAdapter() method. The Platform cannot play > > Nanny for irresponsible plugin programmers. > > So, other programmers don't close JDBC connections properly or don't release > SWT resources properly. Should we deprecate #dispose() or #close() methods too? No, I would expect the method to do what its name say. Get me and adapter, I honestly do not care if you would have to start the bundle for that. If I would care, I would first check using queryAdapter() and then maybe decide what to do. IAdapterManager has so many methods, getAdapter() should behave differently. I know its too late to change that. I will no longer use getAdapter() and even would like to add some kind of API warning if someone in my teams use it in the projects. > One a side note, I already proposed a while back that one solution should be to > make it configurable. This could be a setting (preference, system property, > etc.). A "forceLoad" property on the adapt expression would also help us a lot. > Anybody > interested in looking into AdapterFactoryProxy#loadFactory to try implementing > Boris's proposal? I have a bad feeling about Proxy classes in late Eclipse releases. Take the CommandStateProxy that gets instantiated instead of the one you specify in the extension point. Then try to cast the state to your expected class. A proxy should be transparent to the user. I believe Java even provides support for that. See #302032 (In reply to comment #65) > I somehow fail to see the reasons why it would not evaluate to TRUE if the > bundle providing the adapter is not loaded. Maybe Paul can shed some light on > this? It actually evaluates to NOT_LOADED, which is a third state. I wonder if the UI interprets this as FALSE. I found some places in the workbench which read NOT_LOADED as TRUE. So it may not be the <adapt> expression but the workbench which prevents your handler from being called? > I would like to. How do I extend the expression extension? I think that's not > possible, is it? Hmm, my fault. I thought it were. At least, the code is written in an extensible way but extension is prevented due to singletons resulting in "standard" implementations. Looks like a chance for another patch. ;) As a quick workaround a custom property tester should work which triggers activation. But I agree ... that approach is not the best. (In reply to comment #66) > (In reply to comment #65) > > I somehow fail to see the reasons why it would not evaluate to TRUE if the > > bundle providing the adapter is not loaded. Maybe Paul can shed some light on > > this? > > It actually evaluates to NOT_LOADED, which is a third state. I wonder if the UI > interprets this as FALSE. I found some places in the workbench which read > NOT_LOADED as TRUE. So it may not be the <adapt> expression but the workbench > which prevents your handler from being called? But the adapt expression also skips following expression evaluation if it find the bundle NOT_LOADED. It should evaluate the child-expressions if the adapter bundle is not loaded. It's a tricky situation. > > > I would like to. How do I extend the expression extension? I think that's not > > possible, is it? > > Hmm, my fault. I thought it were. At least, the code is written in an > extensible way but extension is prevented due to singletons resulting in > "standard" implementations. Looks like a chance for another patch. ;) As a > quick workaround a custom property tester should work which triggers > activation. But I agree ... that approach is not the best. Yes I know. I filed a bug years ago to make the expression language extensible and turn all standard implementations into extensions. However for that to work, and that PDE can do its content assist thingy with expressions the "custom attribute" extension point bug would have to be released first, so one could provide the content assists for each expression to the PDE IDE. Right now the PDE expression evaluation is bound to the core expressions schema I too, see this all the time. I would happily settle for an extension of the <adapt> expression element with a forceActivation attribute that defaults to false. I have always wondered why this attribute is present on the <test> expression element, but not on the <adapt> element :-) See http://blog.rcp-company.com/2010/02/using-adapters-in-handlers-to-control.html for an example for a valid use-case in RCP applications - also outlined above by Philipp and others. See also bug 32498 comment 12 as well as bug 32498 comment 14 for references on the original design of adapter factories contributed by extension, and why adapter loading doesn't trigger plugin activation. This a real problem. I am creating a Common Navigator View and some of my content provider's trigger points are based on adapters. But my adapters are not loaded when the Common Navigator is checking the trigger point. the <test> expression allow the loading of the plugin but not the <adapt> so I have to use a fake <test> to force the starting of my plugin and programmatically load the adapters. So much for declarative setup :( (In reply to comment #70) > This a real problem. > I am creating a Common Navigator View and some of my content provider's trigger > points are based on adapters. > But my adapters are not loaded when the Common Navigator is checking the > trigger point. > the <test> expression allow the loading of the plugin but not the <adapt> so I > have to use a fake <test> to force the starting of my plugin and > programmatically load the adapters. > So much for declarative setup :( The request for adapt to have a forcePluginActivation is bug 119948 PW I agree with all those who are for an "optional forceLoad flag in the core.runtime.adapters extension point - defaulting to false". We've bumped into both the situation that actions appeared enabled because a smaller plugin was not loaded to check for it's enablement (+ error popup when user clicked on it) and with contibutions that appeared in the UI because the adapter factory was not loaded when using <adapt> inside an <iterate> over <selection>. The plugin.xml clearly states that some stuff should happen ONLY if that condition is true (some things adapt to something defined in the plugin) - but still those happen if the condition is FALSE because some plugin was not loaded... It took me a while to try different things and then debug eclipse code to see why this happens sometimes. In this case for example: plugin B depends on plugin A, I don't want to merge them but plugin B constributes some things based on an adapter factory defined in B for a type defined in A. I also don't want to make plugin A aware of plugin B to make it call .loadAdapter(...). So I can only use eager loading or startup extension point (which might execute stuff too late I think in this scenario - depending on what's visible at startup). Ah ok, I voted for bug 119948 as well - as that is exactly what I want. I think this can be closed. We have Adapters.adapt. Please reopen if I misunderstood (In reply to Lars Vogel from comment #74) > I think this can be closed. We have Adapters.adapt. Please reopen if I > misunderstood Please elaborate why that should solve the problem. There is no requirement, that everyone uses Adapters.adapt, so it doesn't solve the problem. The suggested solution is a fix on the registration side, not on the usage side. (In reply to Daniel Kruegler from comment #75) > The suggested solution is a fix on the registration side, not on the usage side. We use the eager activation variant of Adapters.adapt heavily in platform code, so call callees should get all adapters. (In reply to Lars Vogel from comment #76) > We use the eager activation variant of Adapters.adapt heavily in platform > code, so call callees should get all adapters. But Adapters.adapt is just one possible way of referring to adapters. IAdaptable and IAdapterManager are official parts of org.eclipse.equinox.common. I haven't heart that anything of this API is considered to be deprecated. I'm sorry, but I can't buy that argument. The reasoning that the new Adapters API solves the problem, is incorrect. First, the internals of Eclipse have not (completely) migrated to use Adapters instead. For a simple example, see how org.eclipse.ui.internal.menus.MenuHelper#getIconURI(ImageDescriptor, IEclipseContext) still uses IAdapterManager. Second, the previously valid API depending on IAdapterManager and friends is still an official part of org.eclipse.equinox.common and has not been deprecated, so the described problem still holds. (In reply to Daniel Kruegler from comment #78) > The reasoning that the new Adapters API solves the problem, is incorrect. > > First, the internals of Eclipse have not (completely) migrated to use > Adapters instead. For a simple example, see how > org.eclipse.ui.internal.menus.MenuHelper#getIconURI(ImageDescriptor, > IEclipseContext) still uses IAdapterManager. Can you open a bug report for this. > Second, the previously valid API depending on IAdapterManager and friends is > still an official part of org.eclipse.equinox.common and has not been > deprecated, so the described problem still holds. I suggest to deprecate this old API, can you open a new bug report for this? |
Although the IAdapter interface does not mention any dependency on AdapterManager, the current org.eclipse.core.runtime.adapters ext. pt. does lead to such a coupling. Consider the following example, hosted in some plugin "CallerPlugin" asking for some arbitrary interface "II" extension: void foo(IAdaptable item) { Object result = item.getAdapter(II.class); ... } Assume now that some extension plugin "ExtPlugin" was defined, which uses the org.eclipse.core.runtime.adapters ext. pt. to provide an extension for some given IAdaptable implementation "AdapterImpl". To make things easy, lets assume, AdapterImpl uses the getAdapter implementation of PlatformObject which simply delegates the request to the AdapterManager. This situation can lead to a chicken-and-egg problem through the lazy-activation policy of the IAdapterManager#getAdapter method, which will not activate plugins defining any extension to the org.eclipse.core.runtime.adapters ext. pt. This behaviour (of IAdapterManager#getAdapter) is documented, which is fine, but that does not solve the described problem. The usual advice is, to call IAdapterManager#loadAdapter instead, but this advice actually causes more problems than it solves: 1) One option is to call AdapterManager.loadAdapter in my AdapterImpl#getAdapter impl. This workaround would solve the problem, but is **indeed** costly, because it forces the activation of more plugins than necessary. It might be noteworthy that I haven't seen any real-life implementation which uses this route. As a side note I would like to ask for one further loadAdapter overload, which expects a Class type argument instead of a raw String to ensure better type security. But this question actually belongs to another request... 2) Another option is in line with one usually observed workaround, namely the direct invocation of AdapterManager.loadAdapter on the interested object, i.e. we have to change the implementation of foo: void foo(IAdaptable item) { Object result = Platform.getAdapterManager().loadAdapter(item, II.class.getName()); ... } While appealing on the first view ("Activate only for items we need to activate"), this solution violates basic software principles, because the request in foo needs to know the way, item's IAdaptable implementation works, which is a Bad Thing! Actually it is even worth than ansatz (1), because it sidesteps the actual getAdapter mechanism. To understand that, consider the not-so-rare case that a given IAdapter implementation does **not** directly delegate to Platform.getAdapter, but instead delegates to the getAdapter call of some member, which itself is known to be implement the IAdapter interface (one example is the FileEditorInput implementation of the IAdaptable interface). 3) Use some arbitrary mechanism to ensure that the interested extensions are activated before the CallerPlugin asks for it. That is easier said than done and normally needs at least the implemenation of one **further** ext. pt. (Usually this will be a user-defined one which e.g. calls createExecutableExtension on any class insdie ExtPlugin). But currently this kind of mechanism is actually the best we can do to combine the optimum between necessary and unnecessary plugin activations. To me it seems that the interaction of org.eclipse.core.runtime.adapters ext. pt. with IAdapter.getAdapter is quite bad as described above. I see several solutions for this situation: A) One extends the IAdapater interface with one further loadAdapter method. This is bad from the point of backward compatibility (or invent IAdaptable2 instead, which is not so much better...), but it would at least allow a consistent way to handle the situation without enforcing the extension requester to know about the impl. details of the underlying IAdapter(2) implementation. B) Extend the org.eclipse.core.runtime.adapters ext. pt. in the following backward compatible way: Add some further **optional** boolean attribute "force" with default value "false" with the effect, that any "true" value enforces plugin activation similar to the way loadAdapter does! This solution would provide what actually is needed: getAdapter doesn't need to provide some arbitrary loadAdapter method (which we invented in solution A), and it moves the responsibility of knowing about the internal mechanism where it belongs to - In the implementation of the adapters ext. pt! I hope I could explain the problem situation in an understandable way. Greetings from Bremen, Daniel Krügler