Understanding Adapters.java

[wimjongman]

 private static <T> T doGetAdapter( Object adaptable, Class<T> adapterType ) {
T result;
if( adapterType.isInstance( adaptable ) ) {
result = adapterType.cast( adaptable );
} else if( adaptable instanceof IAdaptable ) {
result = getAdapterFromAdaptable( adaptable, adapterType );
} else {
result = getAdapterFromRegistry( adaptable, adapterType );
}
return result;
}

The second "else if": You assume that when the "if" succeeds then "result" is valid. This might not be the case. "result" might be null because the object could not adapt itself (which is common). In this case you still have to go to the registry.

There is also an outside case that you might want to examine when there is an unloaded adapter. Please see my code which I believe to be correct.

    /**
     * Replaces the adapter boiler plate code and wraps it in a generic method.
     * Note that this method activates plug-ins if they have the required
     * adapter but are not loaded.
     * 
     * @param object
     * @param clazz
     * @return the adapted object or null if it could not be adapted.
     */
    public static <T> T getAdapter(Object object, Class<T> clazz) {

        if (object == null || clazz == null) {
            return null;
        }

        if (clazz.isAssignableFrom(object.getClass())) {
            return (T) object;
        }

        if (object instanceof IAdaptable) {
            T adapter = (T) ((IAdaptable) object).getAdapter(clazz);
            if (adapter != null) {
                return adapter;
            }
        }

        T adapter = (T) Platform.getAdapterManager().getAdapter(object, clazz);
        if (adapter != null) {
            return adapter;
        }

        if (Platform.getAdapterManager().hasAdapter(object, clazz.getName())) {
            return (T) Platform.getAdapterManager().loadAdapter(object, clazz.getName());
        }

        return null;
    } 

[fappel]

Indeed the "else if" assumption was buggy. Thanks for pointing that out. I have added an appropriate test and changed the implementation accordingly.

Regarding the unloaded adapter I usually would agree with your proposal. But there is something else to respect here. The behavior of the workbench's DeferredTreeContentManager relies on org.eclipse.ui.internal.util.Util#getAdapter, which the Adapters class mimics (admittedly not very well as it turned out...).

I prefered not to introduce a dependency to workbench internals, but to implement an own utility that provides the same behavior (in a typesafe manner) instead. This was a choice between two bad options. Which is also why the Assert#isNotNull parameter check for example is done instead of ignoring null silently like your implementation does.

But the important point here is that Util#getAdapter omits loading of an adapter, which might be to avoid starting bundles unintendedly[1]. And this is why I keep the original behavior in place, which is of course arguable from a general point of view.

After completing the fix the Util's PlatformObject check in case adaptable returns null suddenly looked reasonable too. This avoids redundant registry lookups, which the current Adapters version does also now. So hopefully the current version is safe to use as a replacement for the workbench's Util behavior.

Having said this I will close this issue as resolved. But feel free to reopen if you disagree ;-)

[1]: JavaDoc excerpt of IAdaperManager#loadAdapter(Object,String):
Note that unlike the getAdapter methods, this method
will cause the plug-in that contributes the adapter factory to be loaded
if necessary. As such, this method should be used judiciously, in order
to avoid unnecessary plug-in activations. Most clients should avoid
activation by using getAdapter instead.

[wimjongman]

if( !( adaptable instanceof PlatformObject ) ) { return getAdapterFromRegistry( adaptable, adapterType ); } return null;

I see, you assume the registry lookup is already done because it is a platform object? Clever but a little tricky because implementations might not go to the registry?

But the important point here is that Util#getAdapter omits loading of an adapter, which might be to avoid starting bundles unintendedly[1]. And this is why I keep the original behavior in place, which is of course arguable from a general point of view.

Yes that is lovely discussion :D. I am favor of loading because:

• The plug-in developer did not make the adapter factory by accident.
• The user did not install the plug-in by accident.
• The developer is not in charge of activating the plug-in
• The user is not aware of this technical detail
• The user is not capable of starting the plug-in manually
• Starting a bundle is a few millisecs in 99% of the cases

This is why I do the checking and the subsequent loading but I agree, it is a matter of perspective.

[fappel]

I tend to go with your argumentation regarding the loading topic. But as I said, I wanted to keep the behavior as it is given by the workbench's utility class. This is because I thought it might be even less understandable if similar UI components sometimes trigger plug-in activation and sometimes they don't.

Regarding the PlatformObject: I do not like this check very much, but this is also as the Util implementation behaves. I guess as the only purpose of the PlatformObject is to ask the registry for the adapter, it is assumed very unlikely that someone overrides the PlatfromObject#getAdapter(Class) implementation, or at least is clever enough to add the super call if doing so. In case the super call is omitted and no adapter is returned, this implementation prevents of course the registry lookup completely.

[wimjongman]

Cool! Thanks again.