Community
Participate
Working Groups
As discussed in Bug 475604, the use of an ImageRegistry to cache images based on an ImageDescriptor is a good way of avoiding creating images until they are required; however, if there may be multiple variations of an image (based on windowing system, operating system, or even localised for a different locale) then there may be multiple calls to the filing system to determine the file name to use for the image. Given that we know we don't need that image immediately we can defer the cost of the file lookup and perform the search only when the image data is requested. This can be used to optimise access in places like WorkbenchImages.declareImages() and JFaceResources.initializeDefaultImages(), which currently perform a lookup using a FileLocator.find(Bundle) first and then fallback to a createFromFile operation instead.
The code might look something like: static class DefferedImageDescriptor { DefferedImageDescriptor(String bundle, String path) { this.bundle = bundle; this.path = path; } ImageData getImageData() { return ImageDescriptor.createFromURL(BundleUtility.find(PlatformUI.PLUGIN_ID,path).getImageData(); } } and then be called like: private final static void declareImage(String key, String path, boolean shared) { ImageDescriptor desc = new DefferedImageDescriptor(String bundle, String path); declareImage(key, desc, shared); }
I'd think this should be rolled into ImageDescriptor since its purpose is to defer the actual image creation? If needed, we could have a debug flag / property to force resolving on creation to surface unresolved files earlier.
(In reply to Brian de Alwis from comment #2) > I'd think this should be rolled into ImageDescriptor since its purpose is to > defer the actual image creation? If needed, we could have a debug flag / > property to force resolving on creation to surface unresolved files earlier. Great idea Brian. This would maximize the benefit of this development without creating any additional API. @Alex, I assigned this bug to you, as I assume you plan to work on this.
Does JFace target Java 8 yet? If so we could take a Supplier<URL> which would only attempt to look up the image when requested.
(In reply to Alex Blewitt from comment #4) > Does JFace target Java 8 yet? Yes :-)
OK - in that case we can have a ImageDescriptor.createFromURL(Supplier<URL>) which could defer the lookup until the image data is requested. It might be tricky to integrate with the URL descriptor because if the supplier returns null we need to use the MissingInstanceDescriptor instead. Given the URL often has a bundle and file component, which is created to form a URL and then decomposed to look it up, it might make sense to have a separate one that takes a Bundle and a path as well. I'll do some prototypes and measure the results before I submit a Gerrit review.
Re: comment 6 I think it's much better to write a descriptor that does something specific for Bundle + path (as mentioned in comment 1) than to take a Supplier. A critical part of the *Descriptor classes are their hashCode + equals methods, since they're used as hash keys everywhere. If we wrote one that took Supplier, callers would be very tempted to invoke it with lambdas (which won't have hashCode and equals implemented on them). That means we'd have all sorts of *Descriptor instances floating around the system that we aren't refcounting properly. A custom descriptor that is hardcoded to work with bundle + path can also have a good hashCode + equals implementation, ensuring that everyone that uses it will always do the right thing.
I'd also suggest that we use Strings, IPaths, or URIs rather than URLs since the hashCode and equals method on URL is severely broken.
No plans for me to work on this at the moment, re-assigning to inbox.
I'll take this.
*** Bug 475604 has been marked as a duplicate of this bug. ***
Actually, now that I think about it more, lambdas aren't a bad idea. As long as you cached and reused the same ImageDescriptor instance (which we do in most cases), there would be no problem with ref counting. ...but perhaps we should use something more like ImageDescriptor.createFromURL(Supplier<ImageData>)
New Gerrit change created: https://git.eclipse.org/r/62028
Gerrit change https://git.eclipse.org/r/62028 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=f7f4c7551200215c13364a563f63bab1fbfa81bf
Fixed, using the lambda approach.
Thanks for the patch, Sergey!
This change broke a few things and will be reverted for M5. It came in too late and has apparently not been tested. For one, there's bug 483880. And then I saw a "Pin Editor" spelled out in the workbench toolbar. Steps: - new workspace - paste package p; public class C { public static void main(String[] args) { int i= 2; System.out.println(i); } } - Preferences > General > Editors: Enable "Close editors automatically"
Reverted with http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=ea794195a261df3a624579d4b070f5c1415c051a I tried to debug the problem with the "Pin Editor" button and had to file bug 484032 for JDT Debug to fix conditional breakpoints for lambdas. I manually stepped over the delayed image creations, and I didn't see a request for a "pin" icon. I guess the problem is that infamous E4 model adapter layer bridge still inspects the ImageDescriptor instances and only "works" if they are actually URLImageDescriptors.
org.eclipse.ui.internal.menus.MenuHelper#getIconURI(ImageDescriptor, IEclipseContext) looks like it's the bad guy.
Okay. Thanks for catching and reverting this, Markus. We'll look into a fix early post-M4. E4 should be respecting ImageDescriptor's API contracts and shouldn't be breaking the abstraction layer.
3.x icons were specified as paths or URLs in extension points. The grottiness in MenuHelper wasn't an unreasonable assumption. There are a bunch of things we could do here (and we could do several of them): 1. Provide a clean way to adapt an ImageDescriptor to a File or URL We could have ImageDescriptor support IAdaptable? That doesn't help the Supplier approach. 2. Do what I suggested in comment 2 and move the deferral into ImageDescriptor instead. 3. We could add support for data: URLs and encode non-URL & non-File ImageDescriptors 4. We could add support for a custom URL type to do an XPath-style lookup against the extension registry. E.g., x-registry:/org.eclipse.ui.views/view[@id=%27org.eclipse.ui.console%27]/@icon Supporting lambda/Suppliers is definitely useful, but I don't think it makes sense to wrap everything within them. In fact we could get most of bang for this bug just by changing WorkbenchImages#declareImage() and remove the call to FileLocator: @@ -107,10 +105,14 @@ public/*final*/class WorkbenchImages { */ private final static void declareImage(String key, String path, boolean shared) { - declareImage(key, ImageDescriptor.createFromSupplier(()-> { - URL url = FileLocator.find(Platform.getBundle(PlatformUI.PLUGIN_ID), new Path(path), null); - return ImageDescriptor.createFromURL(url).getImageData(); - }), shared); + try { + URL url = new URL("platform:/plugin/" + PlatformUI.PLUGIN_ID + "/" + path); //$NON-NLS-1$ //$NON-NLS-2$ + ImageDescriptor desc = ImageDescriptor.createFromURL(url); + declareImage(key, desc, shared); + } catch (MalformedURLException e) { + // TODO Auto-generated catch block + e.printStackTrace(); + } } And remove
(In reply to Brian de Alwis from comment #21) > In fact we could get most of bang for this bug just by changing > WorkbenchImages#declareImage() and remove the call to FileLocator: I really meant the following, which removes the unnecessary call to BundleUtility.find(): @@ -103,9 +104,14 @@ public/*final*/class WorkbenchImages { */ private final static void declareImage(String key, String path, boolean shared) { - URL url = BundleUtility.find(PlatformUI.PLUGIN_ID, path); - ImageDescriptor desc = ImageDescriptor.createFromURL(url); - declareImage(key, desc, shared); + try { + URL url = new URL("platform:/plugin/" + PlatformUI.PLUGIN_ID + "/" + path); //$NON-NLS-1$ //$NON-NLS-2$ + ImageDescriptor desc = ImageDescriptor.createFromURL(url); + declareImage(key, desc, shared); + } catch (MalformedURLException e) { + // TODO Auto-generated catch block + e.printStackTrace(); + } }
Re: comment 21 If we wanted to use the adapter system to provide an opt-in interface to certain subclasses, we could use the adapters extension point. No need to implement IAdaptable on ImageDescriptor. However, a new method on ImageDescriptor might be best if we really wanted to be in the business converting ImageDescriptors to URIs. That would make it easier for people to understand the API contracts. MenuHelper.getIconURI would then reduce to a simple call to the new method. If we were to do this then the new Supplier-based API should be returning a URI rather than an ImageData. (While we're on the subject, I notice that MenuHelper is using the wrong algorithm for requesting adapters. It should really be calling Adapters.adapt). > 3. We could add support for data: URLs and encode non-URL & non-File > ImageDescriptors. I guess we may need to do something like this in order to respect the API contracts on ImageDescriptor (which isn't required to point to a file on disk). Othewise we'd end up with two types ImageDescriptor, those that can be used with the workbench model-based APIs and those that cannot. I'm not fond of the memory cost this may create, since we may end up with objects holding onto a URI that don't realize they're retaining a large amount of memory by doing so. Temp files are another approach, but I'm not sure how we could safely clean them up. What was the motivition for making the E4 workbench pass around URIs rather than using the ImageDescriptors themselves for describing images?
(In reply to Stefan Xenos from comment #23) > Re: comment 21 > What was the motivition for making the E4 workbench pass around URIs rather > than using the ImageDescriptors themselves for describing images? E4 is not dependent on SWT or JFace. For example Tom Schindl offers a solution to develop e4 RCP with JavaFX.
> E4 is not dependent on SWT or JFace. I get it. Well, if we're going to fix this without breaking the barrier between e4 and JFace, I think it would make sense to put a getURL method right on ImageDescriptor. That would make the contract explicit and it would mean that any user-defined ImageDescriptor classes would at least have the option of working correctly with e4. I also like Brian's suggestion in comment 22, but I'm not sure it would work for JFaceResources since osgi might not be running.
(In reply to Stefan Xenos from comment #23) > However, a new method on ImageDescriptor might be best if we really wanted > to be in the business converting ImageDescriptors to URIs. That would make > it easier for people to understand the API contracts. MenuHelper.getIconURI > would then reduce to a simple call to the new method. If we were to do this > then the new Supplier-based API should be returning a URI rather than an > ImageData. Not all URLs/URIs are created equal. Some of the contortions of MenuHelper are to rewrite URLs to a more stable format as the URLs produced from a URLImageDescriptor are bundle resource: or bundleentry: URLs that may change between executions or on restarts of the OSGi framework. I mentioned being able to adapt to a java.o.File as URLs aren't necessarily sufficient for all cases either. Some of the work I'm doing on bug 466370 with enhancing the Welcome/Intro require writing out references to images in HTML that can be loaded by the SWT Browser. > (While we're on the subject, I notice that MenuHelper is using the wrong > algorithm for requesting adapters. It should really be calling > Adapters.adapt). I don't disagree. And where it receives an IEclipseContext, it should be trying to pull out the org.eclipse.e4.core.services.adapter.Adapter instance too. > What was the motivition for making the E4 workbench pass around URIs rather > than using the ImageDescriptors themselves for describing images? In addition to what Lars said, E4 supports serialization out of the UI model, currently implemented using EMF. data: URLs work for a (literally) small class of images as they are effectively limited to 2038 characters.
One thing to keep in mind when starting to cache images is the ongoing work for HDPI support. Markus, please comment if the proposed solution makes it hard to choose the right image for a given resolution as per the current approach that we're taking.
(In reply to Dani Megert from comment #27) A solution that relies on URIs is fine. A solution that ultimately caches URLImageDescriptor#getImageData() is not. Converting a file-based Image/ImageDescriptor to ImageData and then creating an Image from the ImageData would also be a bad idea (unnecessary overhead; wouldn't use efficient native image loading). The HiDPI support that is already in URLImageDescriptor [1] requires clients to use createImage(..) and would be broken by any use of ImageData. ImageDatas only contain a single resolution and are not appropriate in any code that need multi-resolution images. [1] Currently disabled. I'll work on that for M5 via bug 382972. The SWT APIs are not yet ready, so there's nothing I can currently recommend for testing.
Please put this bug on hold for now. I've moved the target milestone to M6 to make sure it doesn't fall through the cracks. The BundleUtility/FileLocator#find(..) call in WorkbenchImages#getWorkbenchImageDescriptor(String) will most likely be removed as part of bug 382972, and the responsibility for calling FileLocator#find(..) will be moved into URLImageDescriptor#createImage(..). This is required because HiDPI icons can be contributed by separate bundles, and therefore the platform:/... URLs must not be resolved upfront. I'll work on this in M5 with top priority. The best way to avoid delays is not to release other bad changes to the SDK that absorb my attention.
Re: Comment 29 Wouldn't what I proposed, above (adding a getURL() method to ImageDescriptor), fit in nicely with what you're working on?
(In reply to Markus Keller from comment #29) > Please put this bug on hold for now. I've moved the target milestone to M6 > to make sure it doesn't fall through the cracks. > I'll work on this in M5 with top priority. The best way to avoid delays is > not to release other bad changes to the SDK that absorb my attention. Markus, is your HDPI work still blocking this bug? Having a deferred way to create ImageDescriptors in JFace is highly desired.
(In reply to Lars Vogel from comment #31) > Markus, is your HDPI work still blocking this bug? Having a deferred way to > create ImageDescriptors in JFace is highly desired. The SWT changes were only done very late in M6. I'll work on the adoption in M7.
Removing Stefan as assignee, AFAIK he is busy with his JDT core work.
Alex, are you interested in providing a new Gerrit here?
Sure, let me take a look
New Gerrit change created: https://git.eclipse.org/r/163889
This approach uses a URLSupplier to take no arguments and then create a URL. When combined with the mechanism to look up the images in Workbench we have a means to speed things up. I do wonder whether using an IntFunction would be better, because then we could pass the zoom through to the underlying locator, so that it could choose a more appropriate version instead.
The only outstanding design decision is whether to continue to use URLs, or use this opportunity to have Supplier<URI> instead. Thoughts?
(In reply to Alex Blewitt from comment #38) > The only outstanding design decision is whether to continue to use URLs, or > use this opportunity to have Supplier<URI> instead. Thoughts? Sounds good to me.
(In reply to Lars Vogel from comment #39) > (In reply to Alex Blewitt from comment #38) > > The only outstanding design decision is whether to continue to use URLs, or > > use this opportunity to have Supplier<URI> instead. Thoughts? > > Sounds good to me. I looked into it and while we could do that change it is slightly more invasive to do properly. We could make the change here but it would ultimately convert into a URL shortly afterwards. If we want to roll out URI more generally, we probably need FileLocator and BundleLocator updated to return URIs instead of URLs, which will double the API space. Having URIs instead of URLs is beneficial specifically because the URI doesn't involve DNS lookups for host names; however, I'm not aware of any other specific differences between the two that would help us. However, the URL has the scheme (http/bundlefwk/ftp) and a means to openStream, wihch the URI doesn't -- and it seems that we would need that to resolve resources. So I think mass migration from URL to URI is potentially something that needs greater discussion, and not something we should conflate with this change.
(In reply to Alex Blewitt from comment #40) > So I think mass migration from URL to URI is potentially something that > needs greater discussion, and not something we should conflate with this > change. +1
Alex, please set target (4.17 M1 is the next)
Gerrit change https://git.eclipse.org/r/163889 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=1d4fade4761f4bf75baf5f495a079da46fdcc01a
Alex, thanks for working on this. Can you please add to N&N for 4.17? See https://git.eclipse.org/r/#/c/163509/ for an example.
Will look at the N&N to add this.
New Gerrit change created: https://git.eclipse.org/r/164745
Gerrit change https://git.eclipse.org/r/164745 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=c6bebb4cd3f801bc61b58c71a42c481ab45856ff
