Bug 482972

Comment 1 Alexander Nyßen 2015-11-26 04:20:47 EST

The current behavior of AdaptableSupport is as follows: an adapter can be registered under a given type key (say T_k), and it is assumed that the adapter runtime type (say T_a) is complaint to that type (we cannot check it, only for raw types). According to the current contract, clients can retrieve adapters under any type that is assignable from T_k, i.e. also via super-types of T_k, for which the adapter literally was not registered. 

What I was referring to in comment #1 is to enable that an adapter can be retrieved by any type in between T_k and T_a. This would of course require that we pass in information about the adapter runtime type upon registration, at least if the registration type key does not match the runtime type of the adapter (we could use a TypeToInstanceMap within AdaptableSupport to realize this). 

If this new scheme of registering and retrieving adapters is supported, I would tend to no longer allow the current scheme of retrieving adapters via any super-type of the registration type T_k. This would give clients the change to control the upper bound (via which an adapter can be retrieved) when registering the adapter. 

Consider an example. Assume I want to register an instance of FXViewer under the type key IViewer<Node>. If its unique, I can retrieve it via the type key Object, as well as via the type key IViewer<Node>, but not via its runtime type FXViewer. That is not intuitive and should be changed. With the proposed change, I could no longer retrieve it via the Object key, but via IViewer<Node> as well as FXViewer type keys. That seems to be appropriate.

Comment 2 Alexander Nyßen 2015-11-26 13:28:12 EST

-Changed IAdaptable interface to provide actual type information about the to be registered adapter in case the adapter type is now raw type (and it can thus not be properly determined because of type erasure).
-Changed that unregistering of adapters is performed through the adapter not through the key.
-Changed implementation of AdaptableSupport to enable that adapters can be retrieved by any type key in between the registration type key and the actual type of the adapter.
- Adjusted AdapterStore to changes in IAdaptable and AdaptableSupport. Removed setAdapters() method with Map argument and marked setAdapter(AdapterKey, TypeToken, Object) as to be injected instead.
-Adjusted adapter map injection support to support injection of setAdapter(AdapterKey, TypeToken, Object) method (and to infer the approriate actual type of the adapter from the module bindings (even if its a parameterized type).
-Adjusted AbstractDomain, AbstractViewer, and AbstractVisualPart to the changes in IAdaptable and AdaptableSupport. Removed setAdapters() method with Map argument and marked setAdapter(AdapterKey, TypeToken, Object) as to be injected instead.

What remains is that we need to investigate whether IAdaptable can be made compatible with org.eclipse.core.runtime.IAdaptable (now that it is generified as well).

Comment 3 Alexander Nyßen 2015-11-29 11:24:14 EST

Having introduced an actual type argument, it seems that the upper bound introduced by the key type (which does not have to match the actual type) does not lead to intuitive behavior either. What does not feel good is that if an adapter is queried, even if a compatible adapter is present, it is only found if it has been registered with a key that is 'general' enough.

As such, I came to the conclusion that the most intuitive behavior is that adapters are only registered via their actual type, and that they then can be retrieved via any type key to which their actual type is compatible. I am intensively working on making that happen (and changed the title to reflect this new scope).

Comment 4 Alexander Nyßen 2015-11-29 11:54:18 EST

I pushed the following changes to origin/master:

- Changed setters in IAdaptable to no longer accept an AdapterKey (which is only used for retrieving adapters), but to always register an adapter with actual type only (in case its a parameterized type, that cannot be inferred from the adapter instance).
- Changed getters in IAdaptable to comply to the platform's IAdaptable#getAdapter(), i.e. instead of (getAdapter(Class<? super T>)) we now have getAdapter(Class<T>)  as well.
- Adjusted AdaptableSupport, AdapterStore, AbstractVisualPart, AbstractDomain, and AbstractViewer to comply to the new contract. 
- Adjusted all direct clients that registered and retrieved adapters to comply to the changes in IAdaptable.
- Introduced a Types utility class (which supports to infer the actual parameter type from a context class) and changed all adapter retrievals within MVC to be type-safe, even if the VR parameter is unbound.
- Changed that adapter map bindings may now omit the type key within the used AdapterKey in case it can be inferred from the adapter instance itself, or through the binding (i.e. in case  a constructor binding is used). AdapterKey contract had to be changed to allow this (it now tolerates null keys).
- Enhanced AdapterMapInjector to handle AdapterKeys without type keys (and to infer the actual adapter type from the bindings, or the instance).
- Enhanced AdaptableTypeListener to perform some sanity checks on potential injection points. That is, in order to not collide with Guice member injection, the adapter map injection method may not also specify an inject annotation. Further, the adapter map annotation may not specify an adaptable type.

The desired behavior is now established, as adapters can now be retrieved intuitively. Further, compatibility with platform's IAdaptable (as the getAdapter(Class) now has the same signature).

Resolving as fixed in 3.11.0 M4.

Comment 5 Alexander Nyßen 2015-11-30 10:34:35 EST

I improved the mechanism a bit further (and now regard it as being fixed):

- Ensured that AdapterMap bindings are only used in adapter map bindings, not to mark injection points.
- Introduced a new @InjectAdapters annotation to mark the injection point for adapter injection.
- Refer to mechanism as 'adapter injection' instead of 'adapter map injection', as no maps are injected (only map bindings are used to specify the adapter bindings). Renamed AdapterMapInjector into AdapterInjector and AdapterMapInjectionSupport into AdapterInjectionSupport to reflect this.
- Reworked complete javadoc to properly document the mechanism.

Leaving as resolved in 3.11.0 M4.