Community
Participate
Working Groups
I have following command declaration <extension point="org.eclipse.ui.menus"> <menuContribution locationURI="popup:pl.some.menu"> <command commandId="pl.some.page.delete" icon="icons/delete.gif" label="%page.delete"> </command> .... I am using my copy of delete icon. I would like to use shared image instead, like: PlatformUI.getWorkbench().getSharedImages().getImage(ISharedImages.IMG_TOOL_ DELETE)
The platform:/plugin syntax is already supported for the action/menu contribution icons, although proper support is missing from org.eclipse.ui.internal.commands.CommandImagePersistence http://help.eclipse.org/ganymede/index.jsp?topic=/org.eclipse.platform.doc.isv/reference/misc/platform-scheme-uri.html One option would be to publish the SharedImages platform URLs, although that would tie them to the plugin they're declared in (but should still support NLS) we don't really seem to be moving the icons about much. Another option would be to simply allow some of the constants from org.eclipse.ui.ISharedImages to be used, kinda hack it into MenuPersistence and CommandImagePersistence.. PW
It seems to me there are a couple of solutions here, each with various levels of tooling support. No tooling option 1 =================== We publish platform URIs for all the shared images we wish to support, dependent on bug 250432. They're simply constant strings published in ISharedImages or a new ISharedImageUrls interface, that the client looks up and then copies into their icon attribute. The pro here is that these work for any URL based attribute, and there's no code involved. No tooling option 2 =================== We add a step to org.eclipse.ui.plugin.AbstractUIPlugin.imageDescriptorFromPlugin(String, String) that will look in ISharedImages before trying to turn it into a plugin resource or platform URI. Then users can simply copy standard ISharedImages constants into their attribute. This will work for all of the platform extensions, as they all use imageDescriptorFromPlugin(*). This would accurately reflect the contents of the ISharedImages registry even for RCP apps that add their own shared images. Tooling option 3 ================ Either of option 1 or option 2, but we also create an extension point to fill in cross-linked identifiers. The icon attribute would specify /org.eclipse.ui.sharedImages/images/@uri The notion of extension contributed constants that could be used by identifier attributes could be extended to a more general PDE constant publishing extension (maybe down in core.runtime) that could be used by any extension that wanted an extensible set of constants. maybe /org.eclipse.core.runtime.sharedConstants/category[@id = 'org.eclipse.ui.sharedImages']/constant/@id where sharedConstants defines multiple categories that can be scoped by ID and then filled with constant elements of the form <constant id="platform:/plugin/full/path" name="shared.images.constant"/> For this to work for us, though, the icon attribute would need to be able to support both identifiers (constants) and resources since the common case is selecting an icon from your own plugin. Tooling option 4 ================ Option 2, but PDE provides a way for us to programmically provide constants to the PDE editor (similar to org.eclipse.core.commands.IParameterValues provides values to cheatsheets). PW
I like option 3, PDE can and will enhance the tooling to meet this scenario if we decide it's the best way to go. I say let's prototype something.
Created attachment 116534 [details] Patch v01 I might have gone a little far than the original suggestions. I've added a new scheme sharedImages to the platform URL (*) The new scheme would enable the following URL: platform:/sharedImage/<category>/<id> (*) The category and id are contributed thru an extension point (Option 3), so that RCP apps can add their own images easily (*) The scheme would act as a proxy to the original URL, which could be *any* other resolvable protocol/scheme (*) Since its implemented as a platform scheme, we can use this protocol anywhere in the code as well with ImageDescriptor.createFromURL( new URL("platform:/sharedImage/myCategory/myId")) Review and comment. If its over-engineered, I can revert the scheme idea.
Adding the URL handler is something we might consider doing if we were going do option 2 (no tooling) and allow the image/image descriptor to be returned directly from org.eclipse.ui.IWorkbench.getSharedImages() ... it has the advantage of running from the runtime shared images, but also has no tooling support. What's actually in the runtime shared images is determined by the application's WorkbenchAdvisor, which may or may not contain the image (which may or may not be a problem :-) But we would probably deal with the shared images registry directly, not simply redirect to another URL. One of the things that has me leaning towards using the platform:/plugin support is that we don't have to augment it (with the exception of bug 250432), it already works. If it is acceptable to publish the platform:/plugin URLs as the declarative shared images support, then we would need to decide how best to augment the tooling so that the URLs aren't that hard to find. PW
Created attachment 117425 [details] Patch v02 I've sticked to the Option 3. (*) Plugins who want to contribute to shared images have to extend the extension point. (*) If one needs the shared image in any of the icon attribute, he has to specify org.eclipse.ui.sharedImages/images/<category>/<id> (*) For the icon attribute, tooling can give an option of either browsing for a resource in the plugin or browsing for the shared image Comments/suggestions?
Created attachment 117426 [details] Test Project Project to test the menu & tool bar images
Would it also be possible to allow tooling to browse for all icons in the current workspace and target platform? So that you can use icons that are not part of SharedImages but just lay in other bundles?
(In reply to comment #6) > Created an attachment (id=117425) [details] > Patch v02 > > I've sticked to the Option 3. > With the org.eclipse.ui.sharedImages extension point as you have defined, that's a more specific case of the general constants extension. i.e. we should probably either do: <extension point="org.eclipse.ui.sharedImages"> <constant name="IMG_ETOOL_PRINT_EDIT" value="platform:/plugin/org.eclipse.ui/icons/full/etool16/print_edit.gif"> </constant> <constant name="IMG_ETOOL_SAVE_EDIT" value="platform:/plugin/org.eclipse.ui/icons/full/etool16/save_edit.gif"> </constant> </extension> or <extension point="org.eclipse.ui.idConstants"> <category name="org.eclipse.ui.sharedImages"> <constant name="IMG_ETOOL_PRINT_EDIT" value="platform:/plugin/org.eclipse.ui/icons/full/etool16/print_edit.gif"> </constant> <constant name="IMG_ETOOL_SAVE_EDIT" value="platform:/plugin/org.eclipse.ui/icons/full/etool16/save_edit.gif"> </constant> </category> </extension> If we didn't have to deal with resources as well, then using this in the command element icon attribute in menus.exsd for the first case would be org.eclipse.ui.sharedImages/constant/@value and for the second case would be a minor enhancement to the identifier syntax to support org.eclipse.ui.idConstants/category[name = 'org.eclipse.ui.sharedImages']/constant/@value The only reason to do it the second way is if we see value in allowing other plugins to contribute to our "list of constants" and want to generalize the practice. We (in theory) wouldn't have to modify any of our code, since it always calls org.eclipse.ui.plugin.AbstractUIPlugin.imageDescriptorFromPlugin(String, String) which handles the platform URL values for free (and possibly $nl$ with bug 250432). What we're doing here by filling in o.e.ui.sharedImages is providing shared images to the plugin.xml by bypassing ISharedImages itself. Now add in the fact that we need to handle resources. Question: Do we enhance the <meta.attribute kind="resource"/> to allow it to select resources from other plugins in the workspace (phase 1) and any entry from the target platform (phase 2)? This is a PDE direction. If we don't bother with that, then we would have to use the PDE custom attribute support to supply a UI for this that would allow (at the very least) selection of a resource from the containing plugin as well as choosing from an identifier. I'm not sure how difficult this is, but we'll need to come to some consensus between us and PDE. PW
Created attachment 121422 [details] Patch v03 (In reply to comment #9) > <extension point="org.eclipse.ui.idConstants"> > <category name="org.eclipse.ui.sharedImages"> > <constant name="IMG_ETOOL_PRINT_EDIT" > value="platform:/plugin/org.eclipse.ui/icons/full/etool16/print_edit.gif"> > </constant> > <constant name="IMG_ETOOL_SAVE_EDIT" > value="platform:/plugin/org.eclipse.ui/icons/full/etool16/save_edit.gif"> > </constant> > </category> > </extension> Done this way. Patch attached.
We'll make some decisions and get this into the beginning of M6 PW
Also see Bug 262173 for another issue in the SharedImages...
OK, we had a discussion about this at the last UI call, and here's the summary. 1) we don't want to publish the bundle+path as API ... that would mean never being able to move anything (like the o.e.ui split, some images moved to o.e.ui.workbench) 2) we want to use the final constants in the attribute field. i.e. you use $nl$/icons/full/etool16/save_edit.gif or IMG_ETOOL_SAVE_EDIT Everybody that goes through org.eclipse.ui.plugin.AbstractUIPlugin.imageDescriptorFromPlugin(String, String) should get this for free (and all Platform UI extensions should be using this). You can still use the platform syntax, but that's the same as using an internal class. platform:/plugin/org.eclipse.ui/icons/full/etool16/save_edit.gif 3) We should look at supporting this by using a custom PDE attribute, so the user can either pick a resource in their plugin or pick one of the ISharedImages registered images. We'll have to go to the list of shared images to get the keys (possible API or other potentially marking the PDE attribute plugin as a friend). PW
Created attachment 127448 [details] Patch v04 (In reply to comment #13) > 1) we don't want to publish the bundle+path as API ... that would mean never > being able to move anything (like the o.e.ui split, some images moved to > o.e.ui.workbench) With the idConstants extension, the clients will be using only the contant ids, so we are should be able to move the images internally and update the extension > Everybody that goes through > org.eclipse.ui.plugin.AbstractUIPlugin.imageDescriptorFromPlugin(String, > String) should get this for free (and all Platform UI extensions should be > using this). AbstractUIPlugin.imageDescriptorFromPlugin has been modified to look up into this shared images now. > 3) We should look at supporting this by using a custom PDE attribute, so the > user can either pick a resource in their plugin or pick one of the > ISharedImages registered images. We'll have to go to the list of shared images > to get the keys (possible API or other potentially marking the PDE attribute > plugin as a friend). Should we be doing this tooling support or PDE? Attached is the patch v04. Its essentially v03 + AUIP.imageDescriptorFromPlugin change + tests
(In reply to comment #14) > Created an attachment (id=127448) [details] > Patch v04 > > With the idConstants extension, the clients will be using only the contant > ids, so we are should be able to move the images internally and update the > extension I don't believe we'll have to go this route (this was useful if we were not going the PDE custom attribute route and enhancing the "identifier" support). > AbstractUIPlugin.imageDescriptorFromPlugin has been modified to look up into > this shared images now. This is good, with minor mod. It should handle the constants like IMG_ELCL_COLLAPSEALL or IMG_ELCL_REMOVEALL > > 3) We should look at supporting this by using a custom PDE attribute, so the > > user can either pick a resource in their plugin or pick one of the > > ISharedImages registered images. We'll have to go to the list of shared images > > to get the keys (possible API or other potentially marking the PDE attribute > > plugin as a friend). > > Should we be doing this tooling support or PDE? Yes, we should look at providing the custom attribute at the PDE level: use a "file" like icon currently does, or choose from the list of shared images available. We would probably have to enhance ISharedImages with API or provide some internal class that we could reference in the PDE custom attribute to get the current list of shared images. PW
Created attachment 127613 [details] Patch v05 > I don't believe we'll have to go this route (this was useful if we were not > going the PDE custom attribute route and enhancing the "identifier" support). Ok. reverting back, similar to Patch 02. Now it will be o.e.u.sharedImages extension > This is good, with minor mod. It should handle the constants like > IMG_ELCL_COLLAPSEALL or IMG_ELCL_REMOVEALL Done this way. Also, now the extension allows the plugin relative path like icons/something.png along with the platform:/plugin:/ syntax. I guess most of the time clients will be using the plugin relative path will be used rather than the platform syntax.
(In reply to comment #16) > Created an attachment (id=127613) [details] > Patch v05 I've checked in the org.eclipse.ui.plugin.AbstractUIPlugin.imageDescriptorFromPlugin(String, String) changes. PW
The change in AbstractUIPlugin causes a NPE on startup. I have commented out the changed lines for now, Paul will have a look at this later tonight.
Some more details... the NPE happens here: JFaceResources.getResources(Display) line: 207 ImageRegistry.<init>(Display) line: 152 ImageRegistry.<init>() line: 124 WorkbenchImages.initializeImageRegistry() line: 595 WorkbenchImages.getDescriptors() line: 484 WorkbenchImages.getImageDescriptor(String) line: 520 SharedImages.getImageDescriptor(String) line: 47 AbstractUIPlugin.imageDescriptorFromPlugin(String, String) line: 668 CommandImagePersistence.readImagesFromRegistry(IConfigurationElement[], int, CommandImageManager, ICommandService) line: 116 CommandImagePersistence.read() line: 200 CommandImageService.readRegistry() line: 144 Workbench.initializeDefaultServices() line: 1615 Workbench.init() line: 1324 Workbench.access$32(Workbench) line: 1284 Workbench$57.run() line: 2287 while the main thread is here: OS.WaitMessage() line: not available [native method] Display.sleep() line: 4281 Workbench.runUI() line: 2302 Workbench.access$4(Workbench) line: 2204 Workbench$5.run() line: 499 Realm.runWithDefault(Realm, Runnable) line: 332 Workbench.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 492 PlatformUI.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 149 IDEApplication.start(IApplicationContext) line: 113 EclipseAppHandle.run(Object) line: 194 EclipseAppLauncher.runApplication(Object) line: 110 EclipseAppLauncher.start(Object) line: 79 EclipseStarter.run(Object) line: 368 EclipseStarter.run(String[], Runnable) line: 179 NativeMethodAccessorImpl.invoke0(Method, Object, Object[]) line: not available [native method] NativeMethodAccessorImpl.invoke(Object, Object[]) line: 39 DelegatingMethodAccessorImpl.invoke(Object, Object[]) line: 25 Method.invoke(Object, Object...) line: 597 Main.invokeFramework(String[], URL[]) line: 556 Main.basicRun(String[]) line: 511 Main.run(String[]) line: 1284
If we knew that WorkbenchImages.initializeImageRegistry() is being called anyway, even for RCP apps, we could pull it out and do a sync exec in Workbench.init() for it, just before the call to initializeDefaultServices().
(In reply to comment #20) > If we knew that WorkbenchImages.initializeImageRegistry() is being called > anyway, even for RCP apps, we could pull it out and do a sync exec in > Workbench.init() for it, just before the call to initializeDefaultServices(). > See bug 234252, I think we have been down this path with that bug (after a casual look).
(In reply to comment #19) > Some more details... the NPE happens here: > > JFaceResources.getResources(Display) line: 207 > ImageRegistry.<init>(Display) line: 152 > ImageRegistry.<init>() line: 124 > WorkbenchImages.initializeImageRegistry() line: 595 I too hit this NPE. The ImageRegistry() should be called in the display thread (The javadoc explicitly mentions it). In the patch I would have wrapped creatingthe image registry and initializing the images in the Display.syncExec() in the WorkbenchImages.initializeImageRegistry() method. Is that fine? Looking at Bug #234252, I'm again thinking why can't we have the sharedImages extension point(Patch v05)? While the pde custom attribute would easily fix this bug, the extension point would apply broadly. Almost every RCP app would have its own set of shared images and it would be much easier for them to declare it in the extension and then custom attribute would pick that image where ever an icon attribute is required in the plugin.xml. Does it makes sense? What are the issues that we would face in this approach?
Created attachment 127798 [details] Project for PDE custom attribute The project for custom attribute
Created attachment 127799 [details] Patch for command images schema to use PDE attribute
Created attachment 127800 [details] The UI
(In reply to comment #22) > Looking at Bug #234252, I'm again thinking why can't we have the > sharedImages extension point(Patch v05)? While the pde custom attribute would > easily fix this bug, the extension point would apply broadly. Almost every RCP > app would have its own set of shared images and it would be much easier for > them to declare it in the extension and then custom attribute would pick that > image where ever an icon attribute is required in the plugin.xml. Does it makes > sense? What are the issues that we would face in this approach? If we were pursuing this, then we would look at org.eclipse.ui.commandImages. Would it be a simple enhancement, like an ID other than the commandId (or the commandId is fine)? Would it be another type of element added that allows for declaring key'ed images? However any new extension point will not be done in M6, it will have to be deferred to 3.6. I'd still like to support the lowest level option, providing the ability to type icon="IMG_ELCL_COLLAPSEALL" or icon="shared:IMG_ELCL_COLLAPSEALL" and have it work. But if we can't find a solution to the NPE then we'll have to defer the entire bug. I liked the PDE custom attribute UI (although we wouldn't do it with reflection), but that depends on bug 227055 (I hadn't realized that didn't make it in yet). I'll see if we have a strategy for the NPE. PW
Created attachment 127840 [details] Patch v06 (In reply to comment #26) > I'll see if we have a strategy for the NPE. To me the obvious & dumb solution is to wrap the calls into Display.syncExec(). If not then probably we can try this hack till we get a correct solution.
Created attachment 127858 [details] Provide ISharedImages image descriptors to the icon attributes v07 This will initialize the image registry as part of startup, and then it is available for calls to imageDescriptorFromPlugin(*) We will investigate further optimizations (either updating SharedImages so that it can be initialized off the UI thread or a better UI optimization so that actually declaring images is only done on demand). PW
(In reply to comment #28) > Created an attachment (id=127858) [details] > Provide ISharedImages image descriptors to the icon attributes v07 > Released to HEAD >20090306 (plus an update to some of the schema docs) PW
Another problem caused by this - the JFace test suite did not run (ExceptionInInitializerError). I have added some checks to AbstractUIPlugin so that the tests run again.
Will it be possible to browse icon resources from other projects loaded into the workspace using the "platform:/" protocol too or only shared images?
(In reply to comment #31) > Will it be possible to browse icon resources from other projects loaded into > the workspace using the "platform:/" protocol too or only shared images? > Right now there is no tooling support. If we were to provide a custom attribute, it would provide the current capability (browse the contributing plugin) plus the ability to browse shared images from the currently running eclipse. As was pointed out, we do not publish our icon locations as API, both because they can change from release to release as well as the need for i18n. PW
We'll now honour the shared images constants in any extension attribute that goes through org.eclipse.ui.plugin.AbstractUIPlugin.imageDescriptorFromPlugin(String, String). We'll have to consider tooling support in a future release. PW
(In reply to comment #33) > We'll have to consider tooling support in a future release. I noticed that plugin.xml gives a warning that the image is not found. If full tooling support isn't provided, maybe a way to disable that warning?
Is this bug fixed? While verifying bug 391232 I happened upon this one and part of verifying 391232 I could successfully add shared images to menus, etc. using constant name, $nl$/<path> or plugin:/
It looks like the support is in org.eclipse.ui.plugin.AbstractUIPlugin.imageDescriptorFromPlugin(String, String) and Brian fixed bug 391232 PW
Looks like the mechanism is there, but whether i.e. for which extension points it works depends on whether that API is used when reading an extension point. Best would be that those extension points who support it, explicitly document it in their documentation.
