Community
Participate
Working Groups
RepositoryClientManager currenlty has problem accessing the configuration classes during deserialization. We should make it aware of the right classloader to make this work.
Pushed a fix to http://review.mylyn.org/#change,511 Steffen, I don't see a chance to get hold of the classloader for the configuration based on the type parameter. Under regular circumstances, I don't see a reason why the config ctor should have any side effects (see TODO). Any thoughts?
Thanks for the patch. I don't think we should create objects to obtain a class loader reference. In the worst case scenario it might not be the right class loader if the class lives in a different bundle. I would prefer if we injected the class loader in the constructor of RepositoryClientManager to make it obvious how the class works.
We could also consider passing the configuration class object. That way we could provide a default implementation for createRepositoryConfiguration and access the classloader.
I'd try to avoid leaving classloading up to consumers. I think we should not expose the classloader in the API. I uploaded a new patch set which actually gets the right classloader from the configuration type param to load it.
While I agree that API should be kept as simple as possible the class loading complications are an inherent property of the implementation and consumers should be aware of that. Since the class is SPI, consumers are implementors of OSGi bundles and it seems reasonable to me to require a Class instance to obtain the corresponding class loader. I have pushed a change to Gerrit that modifies your implementation slightly: https://git.eclipse.org/r/5970 https://git.eclipse.org/r/5971 The other consideration is maintainability of the code. I appreciate the smart use of reflective APIs but I'm worried that understanding and changing this type code is significantly more challenging and error prone than a more common solution that obtains a class loader from a class.
Steffen, I think we can close this as being fixed. The only item left for our consumers would be to push this into an API package. Or should we wait another release to see if there are any API changes left.
I moved the class into API: https://git.eclipse.org/r/#change,6094. Thanks for your efforts, Benjamin.
Hm, @org.eclipse.mylyn.internal.tasks.core@ doesn't sound like API to me ;)
I would have to agree with that :).
Moved to API package in ba998485fdb6e76b3983890e0fa17538aa36728d.
