Community
Participate
Working Groups
It would be great if clients could rely on a common protocol for obtaining adapters. The proposed protocol: 1. If the adapted object implements the requested interface directly, it is returned verbatim. 2. If the adapter object is an IAdaptable, its getAdapter method will be consulted for adapters. 3. The adapter manager is consulted for adapters. 4. Plugins are activated if necessary to instantiate the adapters. This PR requests that a pass be made through the UI code to ensure that all adapters are obtained via a consistent pattern. Why is this necessary? A consistent protocol makes it easier to extend the system, since all adapters behave in the same way. Item 1 and 2 shouldn't require much explanation (bug 145480 is an example of what happens when we fail to check instanceof). Item 3 makes the adapter manager much more useful by permitting it to extend ALL classes rather than a limited few. In general, whenever an adapter is requested via IAdaptable.getAdapter, someone will find a use-case where they'll need to provide that adapter via XML. bug 97780 and bug 153132 are some examples where the workbench fails to check for item 3. I've encountered this class of problem many times with various adapter types. Item 4 is necessary to reduce plugin activation. This may, at first, seem counterintuitive but I'll explain. Whenever a plugin provides an adapter, they do so because that adapter is necessary for something. Whenever the workbench fails to create the adapter because its plugin hasn't been activated yet, that "something" won't work. Somewhere, an action's enablement is computed wrong, a property sheet doesn't appear, a label provider or decorator is missing, etc. Usually, this isn't an option, so the client's only alternative is to force every plugin that provides an adapter on a upstream class to be part of early startup. In other words, rather than only being activated the first time the adapter is needed, the plugin will be activated every single time the application starts (which is much worse).
Created attachment 49647 [details] Fixes all adapter usage in the org.eclipse.ui.workbench plugin This patch fixes all adapter usage in the org.eclipse.ui.workbench plugin, with one exception: I didn't update LegacyResourceSupport yet because I wasn't sure I understood all of its subtlities. This can be the subject of another code change. If the UI team is generally in agreement with this patch, I can proceed with updating the remaining UI plugins. Note: if item 4 turns out to be the reason for not accepting the patch, it can be easily removed - just change the loadAdapter call in Util.getAdapter to getAdapter and no plugins will be activated to obtain adapters. A consistent protocol that addresses items 1-3 is still a big improvement.
Created attachment 49674 [details] Patch for ui.views
Created attachment 49675 [details] Fix for ui.navigator plugin (Note: the UI navigator plugin was already in pretty good shape. It uses a similar helper method to keep its adapters consistent. This patch just makes the helper method consistent with the ones in the other patches and fixes the one place where IAdaptable.getAdapter was called directly)
The last plugin to address is core.expressions. Boris has pointed out that, in the case of the expression language, the existence of an adapter could be used to filter out actions until the user explores a related portion of the UI (for example, to filter out the Java object contributions until the first time the user touches the JDT tooling). This makes the expressions stuff a little tricker, so I'll hold off on that one until tomorrow.
We should do at least 1.-3., and look at 4. more closely.
bug 82973 proposes a load-me-eagerly flag that can be attached to adapters. This would offer all the flexibility of item 4 without breaking plugins that rely on the nonexistence of adapters. (As you pointed out to me yesterday, there may be some existing code that relies on this behavior).
Created attachment 49961 [details] updated patch against o.e.ui.workbench
Here is what I did: - changed places where we got workbench services to use getService() instead of getAdapter() - fixed potential problems in maybe two corner cases - changed Util.getAdapter to call IAdapterManager.getAdapter instead of loadAdapter. Updated patch released to HEAD >20060912. Thanks for the patch, Stefan!
Created attachment 49970 [details] updated patch against o.e.ui.views For o.e.ui.view, I had to add a boolean parameter to the helper method, because there was one case where we already called loadAdapter(). Released to HEAD >20060912.
Created attachment 49972 [details] updated patch against o.e.ui.navigator Updated to call getAdapter instead of loadAdapter. Released to HEAD >20060912.
(In reply to comment #0) > Item 4 is necessary to reduce plugin activation. This may, at first, seem > counterintuitive but I'll explain. Whenever a plugin provides an adapter, they > do so because that adapter is necessary for something. I believe we need to look at each individual case to make the decision whether we can afford to activate plugins. Stefan, do you have concrete cases so that you could open separate bugs? Marking this one as fixed.
Boris, I was looking at the implementation of Utils.getAdapter. It is odd that the last check is for an instanceof PlatformObject. I understand that the thought may be that PlatformObject already checks the adapter manager whereas an object that implements IAdaptable may not so you should check. However, this leads to an inconsistency (or hidden behavior if you will). If I implement IAdaptable directly then I cannot prevent the adapter manager from being consulted. However, if I extends PlatformObject I can stop the adapter manager from being consulted by overiding the getAdapter method and not call super. Based on this, I would argue that the instanceof check should either be for IAdaptable or it should not be there at all. Thoughts?
(In reply to comment #12) > Based on this, I would argue that the instanceof check should either be for > IAdaptable or it should not be there at all. Thoughts? Since getAdapter() predates the adapter manager, you cannot expect implementers of IAdaptable to delegate to the adapter manager. Thus, if we want to enable clients to "externally adapt" as much as possible, we have to call the adapter manager. Not using the adapter manager for PlatformObject subclasses seems to be a pragmatic optimization given that the Javadoc for PlatformObject.getAdapter() says that subclasses overriding the method should invoke the super implementation. I would say that being able to stop the adapter manager from being consulted by not calling super is undocumented behaviour and not recommended. Why would you want to prevent others from adapting your object to something else?
Perhaps I got a little carried away with my example. My point was that your optimization does have side effects that most likely will not cause problems but could. In Team, we only consult the adapter manager if the object is not an instance of IAdaptable with the understanding that if the implementor choose to implement IAdaptable, it is their responsibility to do it right. Or, to put it another way, if it is worth optimizing out the second call to the adapter manager for platform object, it should be a concern that the manager is being called twice for all the cases of IAdaptable that don't subclass PlatformObject but do call the adapter manager (especially considering that this is new behavior).
comment 11: > I believe we need to look at each individual case to make the decision > whether we can afford to activate plugins. Stefan, do you have concrete > cases so that you could open separate bugs? Every single case where an adapter is obtained is a case where I may want to implement that adapter in a downstream plugin. Only the adapter author (NOT the code requesting the adapter) knows if their plugin needs to be activated when the adapter is requested. However, the flag proposed in bug 82973 would be sufficiently general to solve this problem in all cases, so always calling getAdapter would be work (provided the other bug gets fixed). > if it is worth optimizing out the second call to the adapter > manager for platform object, it should be a concern that the manager > is being called twice for all the cases of IAdaptable that don't > subclass PlatformObject but do call the adapter manager Calling the adapter manager twice in this case is a little inefficent but harmless. However, failing to call the adapter manager if the IAdaptable *didn't* delegate to the adapter manager would be a bug, so this optimization is only safe with PlatformObject. If, however, it turns out that this case is sufficiently common, we could fix the performance issues by letting AdapterManager cache the most recent call to getAdapter so that the second call runs instantly. Actually, this may make sense anyway... and it would let us remove the special case for PlatformObject.
*** Bug 128078 has been marked as a duplicate of this bug. ***
Jack, could you please verify that the code changes fixed your issue? You should use Eclipse 3.3M2 for that.
(In reply to comment #17) > Jack, could you please verify that the code changes fixed your issue? You > should use Eclipse 3.3M2 for that. Jack?
*** Bug 187909 has been marked as a duplicate of this bug. ***
