Community
Participate
Working Groups
Bug 520080 provides API in JFace, we should deprecate the AbstractUIPlugin#imageDescriptorFromPlugin method to reduce the need to use AbstractUIPlugin.
What about all the facility around AbstractUIPlugin#initializeImageRegistry and AbstractUIPlugin#getImageRegistry? This is widely used. Do we have a replacement for this? Or is it no longer recommended to register images upfront?
(In reply to Matthias Becker from comment #1) > What about all the facility around AbstractUIPlugin#initializeImageRegistry > and AbstractUIPlugin#getImageRegistry? > > This is widely used. Do we have a replacement for this? Or is it no longer > recommended to register images upfront? Mentioned functionality does not specify a way how to obtain ImageDescriptor from bundle and should be discussed separately. Some statement to ignite the discussion (please extract to another ticket if it makes sense): I would expect that JFace resources can be declared in a manifest and then managed by a service according to the SCR lifecycle. It should be similar to Android resources, with auto-generated class that contains Java constants to reference images, fonts and so on.
(In reply to Matthias Becker from comment #1) > What about all the facility around AbstractUIPlugin#initializeImageRegistry > and AbstractUIPlugin#getImageRegistry? > > This is widely used. Do we have a replacement for this? Or is it no longer > recommended to register images upfront? Opened Bug 549928 for that discussion. I think the solution is super simple for that.
(In reply to Alexander Fedorov from comment #2) > Mentioned functionality does not specify a way how to obtain ImageDescriptor > from bundle and should be discussed separately. Some statement to ignite the > discussion (please extract to another ticket if it makes sense): > I would expect that JFace resources can be declared in a manifest and then > managed by a service according to the SCR lifecycle. It should be similar to > Android resources, with auto-generated class that contains Java constants to > reference images, fonts and so on. This sound like a solution which we could do as a second step. Moving the existing methods to a new class should IMHO already allow us to reduce activator usage and therefore improve startup time.
Thanks, Alexander.
This is kind of an annoying change. Anyone trying to maintain some level of support for older versions of the platform (as I do in Oomph and EMF) is faced with this new warning, and the only way to get rid of it properly is to stop supporting older versions of the platform. That's just not an option for me and others will face the same issue. So I would tend to just suppress the deprecation warning for now, but then at some point you'll feel compelled to delete this thing, and which point, to maintain compatibility I would need to resort the reflection, making my code progressively uglier. That makes me unhappy. It seems to me that it's all find a great to provide new improved ways, but is it necessary to deprecate this method? Will you feel compelled to delete it? Also, there is logic in the deprecated method that I don't see in the replacement: IWorkbench workbench = PlatformUI.isWorkbenchRunning() ? PlatformUI.getWorkbench() : null; ImageDescriptor imageDescriptor = workbench == null ? null : workbench.getSharedImages().getImageDescriptor(imageFilePath); if (imageDescriptor != null) { return imageDescriptor; // found in the shared images } I suppose that no one ever actually hits this case?
Ed, I think without deprecation nobody will notice the new method. > Will you feel compelled to delete it? We are currently marking stuff for deletion in 2 years which were deprecated 10 years ago. So this newly deprecated API is definitely not planned to be marked for deletion in the next years. IMHO we would be great if AbstractUIPlugin could be deprecated as a whole at some point, activators are IMHO an antipattern and OSGi services provide a much better way for handling initialization. > I suppose that no one ever actually hits this case? The deprecated API will continue to support this case.
And why should anyone notice? :-P Is the new API so much better or is it just the same thing moved elsewhere? I'm not a big fan of Optional, but that's a personal thing. In any case, if I can "safely" deprecate it then likely I'll create a utility method in on central place and call that instead so that the crappy suppression isn't sprinkled too broadly... Thanks for your responsiveness.
(In reply to Ed Merks from comment #8) > And why should anyone notice? :-P Is the new API so much better or is it > just the same thing moved elsewhere? I'm not a big fan of Optional, but > that's a personal thing. With this API some people may be able to remove their activator. IIRC we could remove 1-2 activators with this change. And we see that activators contribute a lot to the initial startup time.
@Ed I don't think this very method is something special from the support perspective that you mentioned. Please share your vision how we can organize the platform code _development_ better as _any_ change may look annoying from the LTS perspective.
(In reply to Alexander Fedorov from comment #10) > @Ed I don't think this very method is something special from the support > perspective that you mentioned. > > Please share your vision how we can organize the platform code _development_ > better as _any_ change may look annoying from the LTS perspective. It is sort of special because it's a static method so Lars' comment "With this API some people may be able to remove their activator." doesn't really seem so applicable. After all, the new replacement method takes exactly the same arguments, so what would actually compel me to consider removing an activator... The deprecation is not itself such a big deal, it's the concern about removal that's a bigger deal. So for now I suppress the deprecation so I can maintain compatibility with more versions of Eclipse; I suspect many will do this (or then again, many just ignore warnings). So I won't notice the next problem until suddenly the method disappears. And looking at this method, I don't see this as a big maintenance burden that needs removal. And, as I mentioned already, the replacement appears to be missing some logic to deal with workbench.getSharedImages(). And finally, the replacement uses in its API something that's only in Java 8, and older versions of Eclipse did not require Java 8. Note I didn't complain about the FilterTree constructor being deprecated because there it seems more obvious that you're trying to draw attention to a new constructor that will enable fast hash lookup. But there too I would question the need for actual removal of the API because there I could not use reflection to maintain compatibility with older versions. In the end, my consumers are more concerned about stability and broad compatibility than the latest and greatest features, so deprecation is just annoying and deletion is a major concern for me.
(In reply to Ed Merks from comment #11) > (In reply to Alexander Fedorov from comment #10) > > > > Please share your vision how we can organize the platform code _development_ > > better as _any_ change may look annoying from the LTS perspective. > > In the end, my consumers are more concerned about stability and broad > compatibility than the latest and greatest features, so deprecation is just > annoying and deletion is a major concern for me. @Ed As I suppose that Eclipse platform is not yet frozen I'm trying to extract something that can help us to handle the situation like this. Can we think about some kind of "Support Library Packages" (https://developer.android.com/topic/libraries/support-library/packages)? In our case it may be [a set of] fragments with "deleted" types to support older versions. From this point we should not allow to delete particular method in the future but rather move some set of related types to that "compat" fragment. Do you think this approach can make sense?
My biggest concern is the extent to which disruptive changes could potentially slowly drive the community away. I can't imagine it making sense to delete AbstractUIPlugin. Moving it to a fragment would be okay I suppose, if that's effectively invisible to the consumer, but I'm not sure what the advantage would be. To be able to ship something that doesn't include the fragment? That could only happen if all the downstream users at Eclipse eliminate their use of it...
(In reply to Ed Merks from comment #13) > My biggest concern is the extent to which disruptive changes could > potentially slowly drive the community away. I can't imagine it making sense > to delete AbstractUIPlugin. Moving it to a fragment would be okay I > suppose, if that's effectively invisible to the consumer, but I'm not sure > what the advantage would be. To be able to ship something that doesn't > include the fragment? That could only happen if all the downstream users at > Eclipse eliminate their use of it... It is really not easy to find a common point in this discussion :) Well, if Eclipse will ever have enough investments of all kind to complete 4.x work that was started many-many years ago ... In that case I can easily imagine AbstractUIPlugin deprecated and deleted. But currently it looks like Eclipse will continue to live in the door between 3.x and 4.x until its peaceful death. From this point your are absolutely right: we need to better focus on comfortable living out with what we have today.
(In reply to Alexander Fedorov from comment #14) > > It is really not easy to find a common point in this discussion :) > > Well, if Eclipse will ever have enough investments of all kind to complete > 4.x work that was started many-many years ago ... In that case I can easily > imagine AbstractUIPlugin deprecated and deleted. > > But currently it looks like Eclipse will continue to live in the door > between 3.x and 4.x until its peaceful death. From this point your are > absolutely right: we need to better focus on comfortable living out with > what we have today. I appreciate that we have a discussion. Certainly there are indeed a great many things could be dramatically improved by a complete overhaul to clean up old design patterns/solutions for which there are now much better patterns and solutions. That would be the case for EMF as well, and others even started discussions about an EMF 3.0. But breaking changes with the good indention of dramatic improvement is just kind of a non-starter for EMF. It would be possible if I used new package names so that the two incompatible versions of EMF could co-exist, but I could not realistically expect that entire infrastructure of downstream technologies would migrate from one to the other. Leading me to your same final conclusion about the platform. It's just sort of the curse of long term success: Once you successfully establish a large ecosystem, you are bound and restricted by the maintenance of its long stability.
(In reply to Ed Merks from comment #6) > This is kind of an annoying change. > > Anyone trying to maintain some level of support for older versions of the > platform (as I do in Oomph and EMF) is faced with this new warning, and the > only way to get rid of it properly is to stop supporting older versions of > the platform. That's just not an option for me and others will face the > same issue. Super-annoying. I foolishly reacted to the deprecation and switched to ResourceLocator with the result that all my regression builds failed. Confusingly this collided with successive weekends of Jenkins maintenance so the failures were out of mind once a debug session started.. Is is only possible to provide mildly portable code by introducing an exception handler to test what the platform provides or some other needless rewrite. Googling AbstractUIPlugin imageDescriptorFromPlugin finds that https://www.programcreek.com/java-api-examples/?class=org.eclipse.ui.plugin.AbstractUIPlugin&method=imageDescriptorFromPlugin has no fewer than 40 code examples using this API. THEREFORE this must be PERMANENT API. It is not an irrelevant bloat to be eliminated for maintenance convenience. There is no previous long term equivalent.
The API is not marked for deletion, it is only deprecated. For for new code we recommend the new API.
Yes, but it's a deprecation you should never actually progress, so it's only there to confuse/irritate users. The replacement code: return ResourceLocator.imageDescriptorFromBundle(PLUGIN_ID, path).orElse(null); is not hinted at in the deprecation note and is as ugly as hell through use of an orElse and Optional bloat. Far far worse.
(In reply to Ed Willink from comment #18) > Yes, but it's a deprecation you should never actually progress, so it's only > there to confuse/irritate users. > > The replacement code: > > return ResourceLocator.imageDescriptorFromBundle(PLUGIN_ID, > path).orElse(null); > > is not hinted at in the deprecation note and is as ugly as hell through use > of an orElse and Optional bloat. Far far worse. Thanks for the useful feedback, Ed! You are right, the code is far from perfect. It is still contaminated with "static" keyword. Moreover, it still push clients to use "null" keyword, that is also a sign of poor API quality. The work that leads to the creation of this replacement was dedicated to reduce the usage of AbstractUIPlugin, as a part of effort to improve the E4 story. Your comments like "THEREFORE this must be PERMANENT API" may give another breath to this discussion. Please share your vision how do you see the Platform API evolution.
Narrowly, I'm strongly inclined to Ed Merks' very strong view of no breaking API change without a major version change. Practically, I can sympathize with cleaning up offensive/obvious garbage. Something that is in widespread use, 40 examples on a user site, is clearly not offensive or obvious garbage. It must stay for many many (ten) years until the much improved replacement functionality does indeed render the original offensive. If you really want to redirect, I suggest a new @redirect that can push users to better functionality without inflicting prolific warnings on massive numbers of legacy users. @redirect should be accompanied by a quickfix that applies an algorithm encoded in the @redirect to rewrite the original code; no lazy comments to redo something that are just pure fiction. REOPENing discussion. The @deprecated should be removed.
(I'm still pretty het up, since this deprecation has cost me four hours of wasted development time debugging failing builds much aggravated by Jenkins / proxy failures.) Obviously in order to allow users to respond to an @deprecated / @redirect, it is absolutely essential that there is an accompanying @deprecated-since to indicate at what release the replacement functionality became available, thereby ensuring that users wait till their compatibility threshold has passed the @deprecated-since before actually responding. An API tooling setting could identify what that threshold is and automatically report the @depecated-since that are now available for quick-fixes.
New Gerrit change created: https://git.eclipse.org/r/156629
Gerrit change https://git.eclipse.org/r/156629 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=0f092895ff25fb62d806ea2c9e004fb4ed7f8cd5
@deprecated tag has been removed
Thanks.
