Community
Participate
Working Groups
In the course of bugs 516114 and 516738 I got aware of http://blog.vogella.com/2015/09/16/eclipse-activator-startup-sins-tracing-the-startup-time/ When you try to get rid of existing activators you often find activators that use AbstractUIPlugin's image managing facilities (the initializeImageRegistry, getImageRegistry, .. methods). Populating an image registry can also be done in other classes it does not necessarily happen in bundle activators. The fact that the AbstractUIPlugin does provide such convenient methods to easily manager your images and that there is no other class in eclipse platform that provides comparable convenience is the cause that a lot of bundles register their images in their activators. So I propose to extract that image handling stuff out of AbstractUIPlugin. With that we have a clearer separation of concerns and activator-less bundles also can use this.
New Gerrit change created: https://git.eclipse.org/r/101785
May change does multiple things: 1) Create new class AbstractImageMager Most of the code is simply copied over from AbstractUIPlugin. I also adds the methods registerImage that simply registers an image descriptor from a given file name. Name of the class and especially it's location are only a first prototype. I am open to better ideas. 2) Use org.eclipse.ui.forms.examples to show how to use the new class. 3) Removes the code in AbstractUIPlugin's image handling methods and replaces them by delegation to the new class. This is done to avoid duplicated code. What's your opinion with regards to that change?
Is JFaceResources#getImageRegistry() not sufficient as a replacement?
(In reply to Dirk Fauth from comment #3) > Is JFaceResources#getImageRegistry() not sufficient as a replacement? As this is static all plugins would put their images into the one and only registry. I think it's a good idea to keep images from different bundles separated from each other.
(In reply to Matthias Becker from comment #2) > What's your opinion with regards to that change? I like the idea but "org.eclipse.ui.workbench" is too high in the stack. I suggest extending JFace with the option to create an image registry based on key. Bundles could use that to provide the bundle-symbolic-name as key.
(In reply to Lars Vogel from comment #5) > (In reply to Matthias Becker from comment #2) > > What's your opinion with regards to that change? > > I like the idea but "org.eclipse.ui.workbench" is too high in the stack. Sure. I just needed to put it somewhere.
(In reply to Lars Vogel from comment #5) > I suggest extending JFace with the option to create an image registry based on > key. Bundles could use that to provide the bundle-symbolic-name as key. Can you explain in more detail what exactly you have in mind?
(In reply to Matthias Becker from comment #7) > (In reply to Lars Vogel from comment #5) > > I suggest extending JFace with the option to create an image registry based on > > key. Bundles could use that to provide the bundle-symbolic-name as key. > > Can you explain in more detail what exactly you have in mind? Something like JFaceResources#getImageRegistry(String keyForRegistry). Consumers could use it, for example with String bundleSymbolicName = FrameworkUtil.getBundle(this.getClass()).getSymbolicName(); JFaceResources#getImageRegistry(bundleSymbolicName)
(In reply to Lars Vogel from comment #8) > (In reply to Matthias Becker from comment #7) > > (In reply to Lars Vogel from comment #5) > > > I suggest extending JFace with the option to create an image registry based on > > > key. Bundles could use that to provide the bundle-symbolic-name as key. > > > > Can you explain in more detail what exactly you have in mind? > > Something like JFaceResources#getImageRegistry(String keyForRegistry). > Consumers could use it, for example with > > String bundleSymbolicName = > FrameworkUtil.getBundle(this.getClass()).getSymbolicName(); > JFaceResources#getImageRegistry(bundleSymbolicName) OK. I thought about this. Your proposal does only address the createImageRegistry method of AbstractImageManager. But my solution provides much more then the creation of an image registry. My proposal: - ensures that one consumer only registers their images - provide easy to use lazy-registration (via the initializeImageRegistry method). - is may more explicit. So it's easier for consumers to understand how to use it In my option the task of JFaceResources is to provide access to shared stuff (see especially the initializeDefaultImages method). My solution is focused on plugin specific / consumer specific images. So I don't see the advantage of JFaceResources#getImageRegistry(bundleSymbolicName).
I'm unsure about your solution. For example, why is it in org.eclipse.ui which is Eclipse 3.x? In the end you provide a wrapper for registering images to a plugin local registry instance. And where is that instance kept? The previous solution was to hold the reference in the Activator.
(In reply to Dirk Fauth from comment #10) > I'm unsure about your solution. For example, why is it in org.eclipse.ui > which is Eclipse 3.x? In the end you provide a wrapper for registering > images to a plugin local registry instance. Don't get confused by the location. It should go into JFace. I first wanted to discuss this topic before I start to add something to JFace. > And where is that instance kept? It's kept in a static member. An example can be found in ExamplesImages#INSTANCE. > The previous solution was to hold the reference in the Activator. The point of my solution is to have a solution when you don't have an Activator / if you want to get rid of your activator.
I agree with Dirk that the proposed solution is not a good idea. I just glanced over the code and the first problem I came across is that you cache the managers in a static map. How do you clean up the map when the bundle is going down? If i read the code correct you produce not only an (image) handle leak but also a classloader leak.
(In reply to Thomas Schindl from comment #12) > I agree with Dirk that the proposed solution is not a good idea. I just > glanced over the code and the first problem I came across is that you cache > the managers in a static map. Understood. This was my attempt to inhibit that a consumer (by accident creates multiple instances of his subclass. We can remove that part - it's not a vital part of my solution. Do you have an alternative solution for inhibiting this multiple instanciation? > How do you clean up the map when the bundle is going down? If i read the > code correct you produce not only an (image) handle leak but also a > classloader leak. Good point. I provided a dispose method to clean up the images in the registry. But to call that when the bundle is goind down you have to call in the stop() method of your activator. But if you don't have one / want to get rid of your activator I don't know how to do it. Is there another mechanism how a bundle get's notified that it's going to be stopped?
What about just using declarative services? This way, the component will follow the bundle lifecycle, you could still init on the first call on getImageRegistry, and dispose when the component is being deactivated. You may need to introduce an interface for your AbstractImageManager. Alas, it means it needs to stay somewhere in org.eclipse.ui and not in jface.
Maybe we should simply pull out the AbstractUIPlugin#imageDescriptorFromPlugin into a utility class? This would for example allow to replace the activator usage of org.eclipse.ui.tests.views.properties.tabbed. Gerrit upcoming for discussion.
New Gerrit change created: https://git.eclipse.org/r/144705
New Gerrit change created: https://git.eclipse.org/r/144873
Gerrit change https://git.eclipse.org/r/144873 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=e7d0e8d747afa702dd1b2a23e919b5c140fbac66
Thanks, Alexander. Please add to N&N
New Gerrit change created: https://git.eclipse.org/r/147251
Gerrit change https://git.eclipse.org/r/147251 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=17bc81d471404f27693faf71def21a92a5314bf4
