Community
Participate
Working Groups
See https://www.eclipse.org/lists/platform-dev/msg02013.html One issue is that one cannot define multiple adapters for a given adaptableType/class couple so we cannot easily get different adapters depending on finer grain data, such as the content-type of the editor input. This is causing issues like https://github.com/eclipse/wildwebdeveloper/issues/294 or what's discussed in https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/185418/2#message-e8945960c940452a2b3d109df2cc9240657013ca A proposal would be that we change the AdapterManager so it allows to use multiple adapter factory when queried for adapters, until one doesn't return a null adapter.
This still will fail if two like to contribute one adapter for the same type as it would be required with https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/185418/2#message-e8945960c940452a2b3d109df2cc9240657013ca
(In reply to Christoph Laeubrich from comment #1) > This still will fail if two like to contribute one adapter for the same type > as it would be required with Why would it fail?
Because both plugins could/would return a non null value if both apply to the same content-type or alike. One can't assume that they are always exclusive to each other.
(In reply to Christoph Laeubrich from comment #3) > Because both plugins could/would return a non null value if both apply to > the same content-type or alike. One can't assume that they are always > exclusive to each other. In that case, the 1st one wins. We don't really need multiple IToggleBreakpointTarget for 1 editor so far, do we?
(In reply to Mickael Istria from comment #4) > (In reply to Christoph Laeubrich from comment #3) > > Because both plugins could/would return a non null value if both apply to > > the same content-type or alike. One can't assume that they are always > > exclusive to each other. > > In that case, the 1st one wins. Who should be "the first", this will result in the same random behavior as before.. > We don't really need multiple IToggleBreakpointTarget for 1 editor so far, > do we? One can't know in advance, the IToggleBreakpointTarget is very very open and could be used for various break-point types.
(In reply to Christoph Laeubrich from comment #5) > Who should be "the first", this will result in the same random behavior as > before.. Yes, but it becomes the first one whom adapterFactory returns non-null; that makes a great difference with the current state. In the current world, we have this case: there are 2 adapterFactories with same adaptableType/targetObject, the 1st one applies to .c files, the 2nd to any file that has a Debug Adapter coming from LSP4E; when queried for an adapter, the 1st factory is loaded because CDT is active and queried, but it returns null for .js file; end of game => no adapter for .js file. Now, with allowing to invoke an (active) adapterFactory the 1st adapterFactory tries to process the editor and returns null because it's not a C file; then we'll try the next one and hurray, we have an adapter! > > We don't really need multiple IToggleBreakpointTarget for 1 editor so far, > > do we? > > One can't know in advance, the IToggleBreakpointTarget is very very open and > could be used for various break-point types. The "one can't know in advance" is actually more a reason to *not* add complexity until we have enough certainty that we need it.
If you explicitly like to craft the Adapter framework for this *one* particular case for sure, I just thought this ticket was for improve the adapter framework in a general fashion so sorry for disturbance. I think I recently has seen in another eclipse project an alternative Adapter framework that outlines the restrictions of current approach and provides a real good alternative but I sadly can't find it anymore...
(In reply to Christoph Laeubrich from comment #7) > If you explicitly like to craft the Adapter framework for this *one* > particular case for sure, I just thought this ticket was for improve the > adapter framework in a general fashion so sorry for disturbance. I think this case is a perfect example of the issue with multiple adapters and the "winner" problem. Currently, the "winner" can be a bad one, that's a general problem I hope my proposal can resolve and it think it can be profitable for the case of LSP4E debug and other IToggleBreakpointAdapters as well. > I think I recently has seen in another eclipse project an alternative > Adapter framework that outlines the restrictions of current approach and > provides a real good alternative but I sadly can't find it anymore... Good to know. I'll try to look for it, but if you remind more precisely in the meantime, please share ;)
In E4, it could make sense to at least make the adapter framework context sensitive. This would resolve a class of problems, still its an open issue how this define this in the extension points. It seems that the first implementation in E4 was trying be context sensitive. While working on Bug 574409 I noticed that in the E4 context there is the org.eclipse.e4.core.services.adapter.Adapter service. The current implementation EclipseAdapter forwards the call to Adapters.adapt. The Adapters class uses the default singleton AdapterManager, which only uses one factory for each class. The implementation of EclipseAdapter before Bug 480098, did not forward to Adapaters.adapt, but used similar code. Main difference is that it did not use the singleton AdapterManger, but used the AdapterManager which was used in the context. If even more fine grained control is needed, adapters could be extended to have an enabed-when expression. i.e. Use a similar mechanism is also used for handlers. This would move responsibility to the plugin that is providing the adapterfactory. But as said, the interface of IAdaptable returns 'null' when it cannot adapt to the given class. It would make sense to just try another factory, if the first failed. Ordering would be an issue here. BTW, the other project with adapters might be the EMF project.
(In reply to Rolf Theunissen from comment #9) > In E4, it could make sense to at least make the adapter framework context > sensitive. Even though I still can't find the project with the alternative adapters implementation (Adapter is just a to general term to be useful for a google search) I can try to rephrase some points here that are currently limiting the usage of adapters: The Adapters where original intended (If I understand the historic documents correctly) to actually adapt one *Model* object to one *View* object, e.g. if I have a Person and a Table, I'd like to get a LabelProvider for rendering the person Object. For this original use-case it is quite well suited as the adapted object is the only context required here (e.g. I can return the Name as text and the avatar as image). For more advanced usages, this already fails because it is missing the surrounding context (e.g. I have no chance to check if the person Object is currently *selected* in a viewer or similar) so this other framework has enhanced the adapt method with a context object so one can call: > Adapters.adapt(source, ILabelProvider.class, context); context could be e.g. a viewer or any other useful context object (even an E4 context) Instead of returning one single adaption, it was possible to get a list/array of adapted objects instead (so I probably can choose to use the first, last, iterate over all, let the user choose, ... if there are more than one successful adaption). Today I would recommend to return a Stream<TargetType> instead... These would be two points I think that could be easily archived with the current Adapter interface as well 1) The adapt method gets a (default) method with a context parameter that simply delegates to the no context one 2) Adapters gets a new method that returns a Stream<TargetType> of all applicable (non null) adapted objects or an empty stream if none apply or none return a non null item For an E4 Adapter, I as a user would expect I could simply have a method with Annotation @Adapter that has a return type and an arbitrary arguments list and E4 chooses the most appropriate one given the current context + my adapt object where the adapt object is a requirement.
(In reply to Christoph Laeubrich from comment #10) > Instead of returning one single adaption, One goal of returning a single adapter is also a matter of performance: as it's easily possible to adapt in expressions (which many of them get evaluated in any context change, so very frequently), one important value of AdapterManager is to minimize the amount of plugins that would be activated and to minimize the amount of factories that are invoked -in case we only want 1 adapter-. It's important to keep that aspect in mind for the current APIs. But that doesn't mean we can't add a collection of adapters as part as another method if we have need for it in some places; but this one would be much more expansive to call and wouldn't fit in expressions for instance.
https://colver1.rssing.com/chan-2816215/article3322-live.html
(In reply to Mickael Istria from comment #11) > (In reply to Christoph Laeubrich from comment #10) > > Instead of returning one single adaption, > > One goal of returning a single adapter is also a matter of performance: as > it's easily possible to adapt in expressions (which many of them get > evaluated in any context change, so very frequently), one important value of > AdapterManager is to minimize the amount of plugins that would be activated > and to minimize the amount of factories that are invoked -in case we only > want 1 adapter-. > It's important to keep that aspect in mind for the current APIs. That's why I propose a Stream<> as it is lazy and findAny() will stop evaluation at the first hit, beside that, the metadata of the AdapterManager already has all necessary information to narrow the search down very fast. > But that doesn't mean we can't add a collection of adapters as part as > another method if we have need for it in some places; but this one would be > much more expansive to call and wouldn't fit in expressions for instance. As described above I don't see much more impact as with only returning one adapter, if the first returns null we need to evaluate the second anyways even if we only return one so there is no much difference to the current approach.
(In reply to Rolf Theunissen from comment #12) > https://colver1.rssing.com/chan-2816215/article3322-live.html Great Rolf, that's exactly what I have had in mind! Here is the official doc: https://github.com/eclipse/gef/wiki/Common#adapt
(In reply to Christoph Laeubrich from comment #13) > That's why I propose a Stream<> as it is lazy and findAny() will stop > evaluation at the first hit, beside that, the metadata of the AdapterManager > already has all necessary information to narrow the search down very fast. I think this is a very good proposal; even to use internally in the AdapterManager.
OSGi contains a "Converter Specification" that could be related as well: https://osgi.github.io/osgi/cmpn/util.converter.html
New Gerrit change created: https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/186063
New Gerrit change created: https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/186067
I confirm this patch fixes https://github.com/eclipse/wildwebdeveloper/issues/294
Gerrit change https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/186067 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=ea28f6ab1fc5b1425b3ebf11dfca3e5c109949b4
Mickael, there are multiple test fails in SDK that are caused by this patch. https://download.eclipse.org/eclipse/downloads/drops4/I20211006-1800/testresults/html/org.eclipse.core.expressions.tests_ep422I-unit-cen64-gtk3-java11_linux.gtk.x86_64_11.html https://download.eclipse.org/eclipse/downloads/drops4/I20211006-1800/testresults/html/org.eclipse.jdt.debug.tests_ep422I-unit-cen64-gtk3-java11_linux.gtk.x86_64_11.html https://download.eclipse.org/eclipse/downloads/drops4/I20211006-1800/testresults/html/org.eclipse.ui.tests_ep422I-unit-cen64-gtk3-java11_linux.gtk.x86_64_11.html and probably others, that I couldn't immediately identify as related.
(In reply to Andrey Loskutov from comment #21) > Mickael, there are multiple test fails in SDK that are caused by this patch. > > and probably others, that I couldn't immediately identify as related. I can't even install any plugins into the SDK anymore following (https://wiki.eclipse.org/Platform/How_to_Contribute#.5B2.5D_Install_the_developent_tools, Eclipse reports tons of warnings (warnings ?) and finally bails out with NPE error. One of ~ 30 warnings: org.eclipse.core.runtime.AssertionFailedException: Adapter factory org.eclipse.ecf.provider.filetransfer.browse.MultiProtocolFileSystemBrowserAdapterFactory@482e7ffc returned org.eclipse.ecf.provider.filetransfer.browse.MultiProtocolFileSystemBrowserAdapter that is not an instance of org.eclipse.ecf.filetransfer.IRemoteFileSystemBrowserContainerAdapter at org.eclipse.core.internal.runtime.AdapterManager.lambda$11(AdapterManager.java:379) at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:176) at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:177) at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195) at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195) at java.base/java.util.stream.SortedOps$SizedRefSortingSink.end(SortedOps.java:361) at java.base/java.util.stream.AbstractPipeline.copyIntoWithCancel(AbstractPipeline.java:503) at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:488) at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474) at java.base/java.util.stream.FindOps$FindOp.evaluateSequential(FindOps.java:150) at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) at java.base/java.util.stream.ReferencePipeline.findFirst(ReferencePipeline.java:543) at org.eclipse.core.internal.runtime.AdapterManager.getAdapter(AdapterManager.java:383) at org.eclipse.core.internal.runtime.AdapterManager.loadAdapter(AdapterManager.java:407) at org.eclipse.ecf.core.AbstractContainer.getAdapter(AbstractContainer.java:78) at org.eclipse.equinox.internal.p2.transport.ecf.FileInfoReader.sendBrowseRequest(FileInfoReader.java:169) at org.eclipse.equinox.internal.p2.transport.ecf.FileInfoReader.getRemoteFiles(FileInfoReader.java:109) at org.eclipse.equinox.internal.p2.transport.ecf.FileInfoReader.getRemoteFile(FileInfoReader.java:125) at org.eclipse.equinox.internal.p2.transport.ecf.FileInfoReader.getLastModified(FileInfoReader.java:130) at org.eclipse.equinox.internal.p2.transport.ecf.RepositoryTransport.getLastModified(RepositoryTransport.java:249) at org.eclipse.equinox.internal.p2.repository.CacheManager.getLastModified(CacheManager.java:283) at org.eclipse.equinox.internal.p2.repository.CacheManager.createCache(CacheManager.java:194) at org.eclipse.equinox.internal.p2.metadata.repository.CompositeMetadataRepositoryFactory.getLocalFile(CompositeMetadataRepositoryFactory.java:77) at org.eclipse.equinox.internal.p2.metadata.repository.CompositeMetadataRepositoryFactory.load(CompositeMetadataRepositoryFactory.java:100) at org.eclipse.equinox.internal.p2.metadata.repository.MetadataRepositoryManager.factoryLoad(MetadataRepositoryManager.java:63) at org.eclipse.equinox.internal.p2.repository.helpers.AbstractRepositoryManager.loadRepository(AbstractRepositoryManager.java:787) at org.eclipse.equinox.internal.p2.repository.helpers.AbstractRepositoryManager.loadRepository(AbstractRepositoryManager.java:685) at org.eclipse.equinox.internal.p2.metadata.repository.MetadataRepositoryManager.loadRepository(MetadataRepositoryManager.java:110) at org.eclipse.equinox.internal.p2.metadata.repository.MetadataRepositoryManager.loadRepository(MetadataRepositoryManager.java:105) at org.eclipse.equinox.internal.p2.importexport.internal.wizard.ImportPage.recompute(ImportPage.java:441) at org.eclipse.equinox.internal.p2.importexport.internal.wizard.ImportWizard.lambda$0(ImportWizard.java:100) at org.eclipse.jface.operation.ModalContext$ModalContextThread.run(ModalContext.java:122) Final NPE: java.lang.NullPointerException at org.eclipse.equinox.p2.operations.RemediationOperation.computeRemedy(RemediationOperation.java:154) at org.eclipse.equinox.p2.operations.RemediationOperation.computeAllRemediations(RemediationOperation.java:113) at org.eclipse.equinox.p2.operations.RemediationOperation.computeProfileChangeRequest(RemediationOperation.java:97) at org.eclipse.equinox.p2.operations.RemediationOperation.lambda$0(RemediationOperation.java:223) at org.eclipse.equinox.internal.p2.operations.RemediationResolutionJob.runModal(RemediationResolutionJob.java:41) at org.eclipse.equinox.p2.operations.ProfileChangeOperation.resolveModal(ProfileChangeOperation.java:118) at org.eclipse.equinox.internal.p2.ui.dialogs.ProvisioningOperationWizard.computeRemediationOperation(ProvisioningOperationWizard.java:314) at org.eclipse.equinox.internal.p2.importexport.internal.wizard.ImportWizard.lambda$0(ImportWizard.java:135) at org.eclipse.jface.operation.ModalContext$ModalContextThread.run(ModalContext.java:122)
I admit I only checks the Equinox test suite and didn't run all tests before merging. That's also one reason I merged it quickly: fast test feedback. I'm reverting this patch and will continue working on it until the following tests are passing: * org.eclipse.core.internal.expressions.tests.ExpressionTests * org.eclipse.ui.tests.api.IWorkingSetTest * org.eclipse.ui.tests.decorators.DecoratorAdaptableTests * org.eclipse.ui.tests.menus.ObjectContributionTest * org.eclipse.jdt.debug.tests.launching.LaunchConfigurationManagerTests
New Gerrit change created: https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/186165
Gerrit change https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/186165 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=4d406c2eceb612311021ace0231e1a0ea701a6eb
(In reply to Mickael Istria from comment #23) > I admit I only checks the Equinox test suite I think it would be crucial to reproduce the issues with the equinox-suite as well as it seems the test-coverage is incomplete there then.
(In reply to Christoph Laeubrich from comment #26) > (In reply to Mickael Istria from comment #23) > > I admit I only checks the Equinox test suite > > I think it would be crucial to reproduce the issues with the equinox-suite > as well as it seems the test-coverage is incomplete there then. Yes, good idea. (In reply to Mickael Istria from comment #23) > I admit I only checks the Equinox test suite and didn't run all tests before > merging. That's also one reason I merged it quickly: fast test feedback. > I'm reverting this patch and will continue working on it until the following > tests are passing: > * org.eclipse.core.internal.expressions.tests.ExpressionTests > * org.eclipse.ui.tests.api.IWorkingSetTest > * org.eclipse.ui.tests.decorators.DecoratorAdaptableTests > * org.eclipse.ui.tests.menus.ObjectContributionTest > * org.eclipse.jdt.debug.tests.launching.LaunchConfigurationManagerTests Beside this: please compare the Linux/Java 11 build with the original one: *all* failures except the one in pde.ui.tests disappeared after revert, so it is more as just the test suites above. Good: https://download.eclipse.org/eclipse/downloads/drops4/I20211007-0350/testResults.php Bad: https://download.eclipse.org/eclipse/downloads/drops4/I20211006-1800/testResults.php
New Gerrit change created: https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/186292
A colleague reported the following exception Caused by: java.lang.NoSuchMethodError: org.eclipse.core.internal.runtime.AdapterManager.getFactories()Ljava/util/Map; at org.eclipse.oomph.p2.internal.core.P2CorePlugin$Implementation.start(P2CorePlugin.java:145) at org.eclipse.osgi.internal.framework.BundleContextImpl.lambda$2(BundleContextImpl.java:808) at java.base/java.security.AccessController.doPrivileged(Native Method) at org.eclipse.osgi.internal.framework.BundleContextImpl.startActivator(BundleContextImpl.java:802) ... 73 more this seems to be related to the change here, signaling that a signature change, even if in an internal package must be coordinated with oomph.
(In reply to Stephan Herrmann from comment #29) This has been reported as Bug 576503
Gerrit change https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/186292 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=6d6407a1b4edc531107697d9fa403ea5b942d656
(In reply to Mickael Istria from comment #8) > I think this case is a perfect example of the issue with multiple adapters > and the "winner" problem. Currently, the "winner" can be a bad one, that's a > general problem I hope my proposal can resolve and it think it can be > profitable for the case of LSP4E debug and other IToggleBreakpointAdapters > as well. We see a regression in our product coming from the new behavior. In our case the "winner" was the *intended* null return, as opposite to the example above. So "good" null values returned from factories are ignored now and the first "bad" non null adapter wins, breaking existing code. Concrete example. By default, org.eclipse.debug.internal.ui.views.launch.DebugElementAdapterFactory contributes an adapter for ISourceDisplay that always tries to open an editor for given JDIStackFrame. Now, in some (internal) stack frames we won't show the editor (because it would open generated or internal class without source) and so we've contributed a factory that returned null adapter object for ISourceDisplay under specific conditions. With null adapter object returned, no attempt was made to open an editor and everyone was happy. That worked before 4.22 but now since the null return from our factory is being ignored, and another one factory returns a non null adapter, we see a regression because an editor is opened where no one should. We have a solution for our concrete problem (we contribute now a non null dummy ISourceDisplay object that does nothing), but two issues remain. 1) Whoever relied on overriding some other adapter factories to return null adapters is broken now with 4.22, there is no way to fix that and depending on what kind of adapter object was expected, it might be not possible to restore pre-4.22 application behavior. 2) The AdapterManager implementation is inherently "unsafe" in the sense, it doesn't specify or guarantee any order in which the multiple factories adapting to the same type are iterated. That's not a new issue, but if we expect that something should be improved with the new patch and start providing new API's on top, that allow to work with multiple factories, we should think if and how we can guarantee *some predictable* order for those factories, so developers can rely on that order being stable in the future. Right now we simply rely on equinox & extensions load order, which is not specified or guaranteed to be stable (it just happened to be stable in the past). I have no solution for 2), but I wonder if we should at least address 1) and provide a system flag (or some other fix) that allows to restore old behavior for those who can't find a solution for "good" factories returning "null" adapters being ignored now.
The issue of unspecified behaviour ;) A workaround is to not return null, but to return Objects that do nothing; so it's an adapter intentionally doing nothing that wins. It seems also clearer to understand from consumer perspective. Maybe we should add to AdapterFactory another new method, and we specify more clearly the behvior of the various method: * getAdapter(..., load=true) => return the 1st available adapter, trying the more specific adapter factories first * getAdapter(..., load=false) => return the 1st available adapter from already loaded factories * getAdapterFromFirstFactory(..., load=true) => picks the preferred adapter factory, loads it if necessary, and return the adapter, or null. Other factories that are "lower-ranked" are ignored * getAdapterFromFirstFactory(..., load=false) => picks the preferred adapter factory which is already loaded and return the adapter, or null. Other factories that are "lower-ranked" are ignored Another approach is to give access to factories directly in the preferred order, as a Stream like it was suggested earlier, and clients who need specific behavior can get it. It wouldn't prevent the regressions based on the fact that former behavior was unspecified and changed; but it allows people who really have a strong reliance on this behavior to workaround the change.
Actually the IAdapterManager-API does not (and has never) guaranteed any order or precedence. The AdapterManager on the other hand has only guaranteed the order where Adapters are searched. Thus any code that has relied on the fact that registering an adapter for the *same* type might override an existing adapter is inherently broken as it is (and was) unpredictable which adapter win the race. For the case of a more specific adapter returning null, its a bit vague (as mentioned on other places before) as the API only states > an object that is an instance of the given class name associated > with the given object. Returns <code>null</code> if no such object > can be found. and > an object castable to the given adapter type, or <code>null</code> > if the given adaptable object does not have an available adapter of > the given type both statement do not limit the query of *different* adapters and do not make any guarantee about order or what happens if an Adapter itself return null. As also mentioned before in the reviews, if we need any kind of order, we need to impose an "getPriority" method but this will only cover the case of two factories for the same(!) type. E4 solves this case with defining a special Object "NOT_A_VALUE" that indicates that others should be queried.
(In reply to Mickael Istria from comment #33) > The issue of unspecified behaviour ;) > ... > It wouldn't prevent the regressions based on the fact that former behavior > was unspecified and changed; but it allows people who really have a strong > reliance on this behavior to workaround the change. It might be less disturbing to existing code if we keep the old behavior of the existing methods and use my Stream<..> approach for code to actively opt-in for the new behavior. The Stream will cover both use-cases of finding multiple or just a single value very well and we really should not add more methods as it's already confusing with the queryAdapter versus getAdapter versus loadAdapter and so on... Maybe it would even be better to actually just return a stream of factories and let client code use Adapters class (as it is already suggested in the method comments) and adding the appropriate helper methods there. If this can get some kind of conses I can adjust my Gerrit accordingly
(In reply to Christoph Laeubrich from comment #35) > It might be less disturbing to existing code if we keep the old behavior of > the existing methods and use my Stream<..> approach for code to actively > opt-in for the new behavior. For the record, the change that happened to AdapterManager was actually fixing some other bugs; so it's both fixing some issues, but causing others for cases where code was *intentionally* relying on unspecified behavior. So it's not clear what should be prioritized here: backward-compatibility for unspecified stuff (for which there is often a workaround), or more "adaptative" and optimistic behavior which had no possible workaround. I would strongly support the 2nd to be preferred and made the 1st-class citizen, but we can still consider how to better handle the previous usage. > Maybe it would even be better to actually just return a stream of factories > and let client code use Adapters class (as it is already suggested in the > method comments) and adding the appropriate helper methods there. I also have the impression it would provide both a simple and powerful approach. However, before adding this API, let's just wonder clearly: do we want to support in our APIs the case of people relying on the former unexpected behavior? Or couldn't we instead provide more guidance and documentation (eg in migration notes and others) to just let people adapt their code?
New Gerrit change created: https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/187084
> The issue of unspecified behaviour ;) > Actually the IAdapterManager-API does not (and has never) guaranteed any order or precedence. I'd like to quote Hyrum's Law (https://www.hyrumslaw.com/) since it's simply true: With a sufficient number of users of an API, it does not matter what you promise in the contract: all observable behaviors of your system will be depended on by somebody. I think keeping the existing behavior for the existing methods is crucial and additional / modified behavior must be confined in their own APIs as proposed in comment #35.
(In reply to Andrey Loskutov from comment #32) > We see a regression in our product coming from the new behavior. By the way: Do you think you can derive a (now failing) test case that shows the changed behavior? I think it is critical that we cover all those 'legacy' cases here to make sure this is not broken unintentionally later on. Because beside weather or not one would claim it 'undefined' behavior it was also 'uncovered' behavior.
And if we read the Javadoc contract on IAdapterManager pedantically, the behaviour was kind of specified, but implementation was not matching it: " @return an object of the given adapter type, or <code>null</code> if the given adaptable object does not have an available adapter of the given type" Before this patch, some adaptable objects that have available adapters for the given type (provided by the 2nd factory) were getting null as output of this request; so the contract was kind of broken compared to how it's stated. The blurry area can be what "available adapter" mean. Is a null output for an adapter factory an "available adapter" ? That brings back to the idea of the NOT_A_VALUE idea or the NoopAdapter as a way to distinguish between a "null available adapter" and "no available adapter". So IMO, the change is really a fix. But it can be disruptive for some cases. I think we should just work on improving the N&N and migration notes to cover it.
(In reply to Mickael Istria from comment #36) > For the record, the change that happened to AdapterManager was actually > fixing some other bugs; so it's both fixing some issues, but causing others > for cases where code was *intentionally* relying on unspecified behavior. > So it's not clear what should be prioritized here: backward-compatibility > for unspecified stuff (for which there is often a workaround), or more > "adaptative" and optimistic behavior which had no possible workaround. I think its easier to let the "old way" broken and encourage everyone to change code that requires the new (fixed, more consistent, well defined, ...) behavior instead. That way existing code won't break but users are still able to migrate to a working approach. > > Maybe it would even be better to actually just return a stream of factories > > and let client code use Adapters class (as it is already suggested in the > > method comments) and adding the appropriate helper methods there. > > I also have the impression it would provide both a simple and powerful > approach. > However, before adding this API, let's just wonder clearly: do we want to > support in our APIs the case of people relying on the former unexpected > behavior? Or couldn't we instead provide more guidance and documentation (eg > in migration notes and others) to just let people adapt their code? I think we simply should fill some gaps (and make people clear about that) so that newer code can deterministically write and use adapters for the common case. All the ones requiring some very special and specific behavior can still use the "low level" APIs ...
(In reply to Mickael Istria from comment #40) > And if we read the Javadoc contract on IAdapterManager pedantically, the > behaviour was kind of specified, but implementation was not matching it: > > " @return an object of the given adapter type, or <code>null</code> if the > given adaptable object does not have an available adapter of the given type" > > Before this patch, some adaptable objects that have available adapters for > the given type (provided by the 2nd factory) were getting null as output of > this request; so the contract was kind of broken compared to how it's stated. > The blurry area can be what "available adapter" mean. Is a null output for > an adapter factory an "available adapter" ? That's what I mentioned before, its a bit of interpretation here and why I think a new API might be more suitable given that there is so much code relying for year on the old behavior. I'll try to propose some smaller patches (like with the synchronization) towards this goal... Question is, should we reopen this?
(In reply to Christoph Laeubrich from comment #41) > I think its easier to let the "old way" broken and encourage everyone to > change code that requires the new (fixed, more consistent, well defined, > ...) behavior instead. No, it is not easier. I've spent months considering this issue from the outline use-case, weeks working on a patch that seems sane for it while still respecting the contracts and making all existing tests pass. If there was something easier, I hope I'd have identified and implemented it instead. And frankly, there is no way to make the multi-adapter story work decently in many existing cases without fixing the implementation of the API to better fit the contract. > That way existing code won't break but users are still able to migrate to a working approach. We really need more examples of code that's broken by that change. There seems to be none in Platform; Andrey got 1 instance and he mentioned it's fixable on the consumer end and acknowledged that it's relying on implementation details that aren't specified, so it was unsafe and fragile code from day 1. As opposed to it, the bug that triggered that development is not fixable without changing how the outline or other adapters are loaded in *many* places. Basically, all places that use getAdapter may need to adapt to the new behavior because it's usually better.
We have many code that is (and was) fine with the current behavior (and some of these are maybe broken or behave different). On the other hand there are just some that where hit by the "multi-adapter-problem" so it seems easier to adjust these few cases to the new API instead of assume that someone will review/adapt all places that use getAdapter and alike.
(In reply to Mickael Istria from comment #43) > We really need more examples of code that's broken by that change. + 100 from my point of view a regression without a test-case is not a regression ;-) > There seems to be none in Platform; One problem is, that these may include *subtile* behavior changes that are not easy to spot... > Andrey got 1 instance and he mentioned it's > fixable on the consumer end and acknowledged that it's relying on > implementation details that aren't specified, so it was unsafe and fragile > code from day 1. The approach is fine, but its just another workaround that will not always fit e.g. it relies on the fact that there is an implementation that "do nothing" and the rest of the code is fine with something that does nothing instead of getting a null to have a clear understanding that something is missing. Just think about a button or view that is only enabled when the adaption is !=null such an approach will always enable the button and user will be wondering why nothing happens there...
I think the original behavior can be restored while keeping the original intend of Mickael here. The actual issue this ticket tries to solve is the case where I have two adapters (A) that adapt to the same target type (T) and the same Source (S) Object. So the solution would be to *only* use the "return first non-null" for adapter with the same T->S mapping. The problem is, that the current datastructure does not holds this information, but instead computes every possible combination and thus at adaption time it is not possible to distinguish between those (as far as I can see).
(In reply to Christoph Laeubrich from comment #46) That is the discussion we had in https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/186540/2/bundles/org.eclipse.equinox.common.tests/src/org/eclipse/equinox/common/tests/adaptable/IAdapterManagerTest.java which mostly concluded with: Jonah said: The original bug was for providing multiple adapter factories for the same pair of adaptable / adapters. Mickael said: That's the title but in practice, from https://github.com/eclipse/wildwebdeveloper/issues/294 , we need it cover the "family". It's frequent for providers of adapter factories to use a sepecialized type as adaptable to keep priority for their own type and let the default adapter apply. --- So I don't think Comment 46 can resolve the issue.
(In reply to Christoph Laeubrich from comment #46) > So the solution would be to *only* use the "return first non-null" for > adapter with the same T->S mapping. IMO, this is too restrictive and not the most helpful implementation, and still not how I interpret the inital contract: imagine I have LSP4E installed, LSP4E contributes a ExtensionBasedTextEditor->IContentOutlinePage adapter factory. This factory returns null when the file being edited doesn't have LS support; this is something that can only be established dynamically and highly depends on what is installed on the Platform, noone can know for sure in advance what this adapter is going to return for a given file. Now imagine someone else creates a less specific adapter from TextEditor->IContentOutlinePage for a specific file format, independently of LSP. If the user open such a file in the ExtensionBasedTextEditor, they can expect that unless another more specific adapter (LSP one) is involved, their adapter is used as fallback. Your proposal would make this case lead to an empty outline page when there could be interesting content. And same applies to many cases of Adapters usage in Platform. The only good and safe way to enforce an adapter over other ones for an object that's owned by the developer is to make it implement IAdaptable and to hardcode the adaptation strategy they want; but basically it removes all the beauty of adapters; however I have the impression that all the cases that have relied on getAdapter() returning null even if there are viable adapters do not really care about the beauty and power of adapters, so that's probably fine for them to hardcode the strategy instead of relying on extensions and adapter factories.
(In reply to Mickael Istria from comment #48) > (In reply to Christoph Laeubrich from comment #46) > > So the solution would be to *only* use the "return first non-null" for > > adapter with the same T->S mapping. > > IMO, this is too restrictive and not the most helpful implementation, and > still not how I interpret the inital contract But that's your interpretation of the API, the implementation on the other hand has always given precedence of more specific AdapterFactories and allows to "hide" default implementations and many codes might rely on this behavior! If WWD (or other code) has different requirements IMO we can't change the exiting implementation. That's why I clearly would recommend my Stream approach from the very beginning as it is more flexible and consumers can implement whatever logic and/or precedence they desire, there is no "one fits all" in my opinion.
(In reply to Christoph Laeubrich from comment #49) > But that's your interpretation of the API I think my interpretation is much closer to the actual specification than... > allows to "hide" default implementations Which is really not even mentioned in the contract nor specification. > and many codes might rely on this behavior! Too bad for them, but it's not Platform that's to take the blame and the burden of what people have done wrong with some unspecified behaviors. Basically you're pushing the idea that "bad" consumers are worth reducing the quality of the features provided by Platform and worth preventing improvements and innovations. I strongly oppose to that; there is a clear API governance in place for Eclipse Platform, and things that are in the scope of the governance are handled as deserved with focus on backward compatibility and keeping contracts fulfilled. The change here is not in this scope because it's not breaking any contract. Codes that have relied on this behavior should have considered how reliable that was from the beginnign and maybe considered contributing test cases or contract clarification earlier instead of just doing nothing and trying to slow down Platform later if this happens to be improved. Please define "many" with some links to some other tickets. > If WWD (or other code) has different requirements IMO we can't change the > exiting implementation. The change is more a bugfix than a new feature in practice.
(In reply to Mickael Istria from comment #50) > Please define "many" with some links to some other tickets. There are two cases (CDT's and Andrey's report) already and the code has only been in M2 (which isn't even released yet as Platform don't even officially release an M2). I think you are underestimating the effect of this change in the ecosystem.
A comment that is prefixed with "Too bad for them" is questionable at best in any context but coming from someone who represents The Platform to The Community, it's definitely a sour note. I would like to suggest wording such statements to be at least a little bit more diplomatic, to reflect at bit of sensitivity to all the parties involved. When I read further about '"bad" consumers', I yet cringe again. Does this really all boil down to just being about the 'good providers' versus the 'bad consumers'? Or worse, about 'my interpretation' versus 'the behavior as it has been for more than a decade'? Surely balance and reason should hold sway. No one questions the good intentions behind the changes nor dismisses the effort involved. Just keep in mind that what's best logically is not necessarily what's best practically, exactly as Sebastian has suggested...
