Community
Participate
Working Groups
Build Identifier: emf-compare-SDK-incubation-1.1.0M5 Both org.eclipse.emf.compare.ui.ModelCompareInput and org.eclipse.emf.compare.ui.viewer.content.ModelContentMergeViewer are part of EMF Compare public API (i.e. not in an internal package). However, org.eclipse.emf.compare.ui.viewer.content.ModelContentMergeViewer#createModelCompareInput(ModelComparator, ComparisonSnapshot) and org.eclipse.emf.compare.ui.ModelCompareInput.ModelCompareInput(MatchModel, DiffModel, ModelComparator) reference org.eclipse.emf.compare.ui.internal.ModelComparator which is not public (i.e. located in an internal package) in their signature. Consequently, applications overriding ModelCompareInput and ModelContentMergeViewer are face to a discouraged access warning. In order to get rid of that ModelComparator should be made public or all references to it should go through a public interface. Reproducible: Always
org.eclipse.emf.compare.ui.internal.ModelComparator is internal API, and thus cannot be accessed without discouraged access warnings. Clients extending org.eclipse.emf.compare.ui.viewer.content.ModelContentMergeViewer and trying to override ModelContentMergeViewer#createModelCompareInput(ModelComparator, ComparisonSnapshot) expose themselves to this warning, as highlighted by the javadoc of this method : ----------8<---------- * @noreference This method is not intended to be referenced by clients. * @nooverride This method is not intended to be re-implemented or extended by clients. */ protected ModelCompareInput createModelCompareInput(ModelComparator comparator, ComparisonSnapshot snapshot) { ---------->8---------- The same applies to org.eclipse.emf.compare.ui.ModelCompareInput.ModelCompareInput(MatchModel, DiffModel, ModelComparator) which javadoc clearly states ---------->8<---------- * @noreference This constructor is not intended to be referenced by clients. */ public ModelCompareInput(MatchModel matchModel, DiffModel diffModel, ModelComparator comparator) { ---------->8---------- This might be an API leak, yet it is documented as such. Clients overriding these methods do it at their own risk.
We actually need to access above named methods for the following reason: we need to create a refined implementation of org.eclipse.emf.compare.ui.ModelCompareInput. As the latter is part of public API, this shouldn't be a problem. Your unwillingness to respond to the issue we have raised here means that you force us (and all other potential EMF Compare users) to always use the original ModelCompareInput. Why? Can't it happen that use cases of EMF Compare adopters go beyond what is provided by EMF Compare out of the box? And do makers of EMF Compare really want to deny to help such EMF Compare adopters to get their use cases realized? To my own experience, the effort of doing so would highly pay off. Not only for the EMF Compare adaptors but also for EMF Compare as such. At the end of the day you would have a more flexible and consequently a more broadly usable framework. This is the magic that feedback can do - in case you make the effort of listening to it. So, could you please reconsider our use case (I've renamed the bug to let it appear a little bit more clearly) and help us to find a way of customizing the ModelCompareInput WITHOUT having to access internal API? To be clear: Making the ModelComparator public would be ONE possible way of doing so. Alternative solutions which lead to the same result are for sure also welcome. Thanks.
We value users and adopters feedback as bugs. We're constantly listening to this feedback and trying to adapt the code when needed, bugs 300819, 301395, 301112, 302698, 303039, 303040 are examples among many others of specific needs addressed through adopters contributions, feedback and patches. That's exactly for those adopters that we're concerned about making relevant choices concerning API. The method have been marked as noreference and nooverride when we agreed to export the package in discouraged access in 297305,: we don't consider "opening things up without designing how the user would reuse the component" as being a valid long-term solution. To my own experience, the effort of listening to feedback is highly paying off and we've got many example of this, the effort for adopters to discuss and work with us on a clean and maintainable proposition is also worthing it. Now please listen to our feedback : Changing ModelComparator to make it public is a no-go, it's containing too much logic subject to change and sometime only relevant for the Team API integration. You can go the "Extracting an Interface" road but it will be tricky too as the ModelComparator class has many responsabilities : loading/unloading, calling team specific callback, launch comparison and get those results, if you override something there not being able to re-use the original behavior you'll end up having bug, and I won't ask you to *keep that in mind* as obviously you already forgot what have been said in 297305. To avoid such discouraged access one would need specific Interfaces and Implementation for all of these responsabilities and the modelcomparator would only be a shell delegating to the proper instance, we could even get rid of him in ModelCompareInput easing the UI component reuse. It occurs to me we already discussed that in 298400... We're always open to discussion as to how we can improve the API, but not with the scattershot approach you've taken which is "open it as API so we can use it" ; that is a no-go because of the ModelComparator responsibilities. We'd rather try and find a way to properly delegate some of these responsibilities, then open the new APIs created in the process. You now have a choice to make, red pill, come with a proposition following one or the other path I described, blue pill you live with the discouraged access you asked for in 297305.
(In reply to comment #3) > To my own experience, the effort of listening to feedback is highly paying off > and we've got many example of this, the effort for adopters to discuss and work > with us on a clean and maintainable proposition is also worthing it. Yes, this is exactly what we would like to get instantiated again here. Unfortunately, the initial reaction to this bug was: @nooverride, @noreference, WONTFIX. How does this fit together? > You now have a choice to make, red pill, come with a proposition following one > or the other path I described, blue pill you live with the discouraged access > you asked for in 297305. We have asked for the blue pill in order to buy us more time (i.e. beyond M6 milestone) for finding a clean solution without being blocked or having to use a patched version of EMF Compare until we manage to do so. Now we want to go for the red pill - exaclty this has been the principal motivation for opening up this bug. > You can go the "Extracting an Interface" road but it will be tricky too as the > ModelComparator class has many responsabilities : loading/unloading, calling > team specific callback, launch comparison and get those results, if you > override something there not being able to re-use the original behavior you'll > end up having bug, and I won't ask you to *keep that in mind* as obviously you > already forgot what have been said in 297305. This gives me the impression that you are principally OK with the idea of introducing one or several new interfaces to avoid direct references to ModelComparator. I've drafted an initial version of such an interface - ICompareInputDetailsProvider - and attached it to this bug. It does NOT expose ALL ModelComparator methods but only the subset of methods which is really required by ModelCompareInput. I've also replaced all references to ModelComparator in ModelContentMergeViewer and ModelCompareInput - you can find these files also attached to this bug. As a result, I was able to get completely rid of the discouraged access warnings in our own code and the @nooverride, @noreference restrictions on ModelCompareInput and ModelContentMergeViewer are no longer be necessary. Could you imagine to take this as a basis for implementing a solution to our problem in EMF Compare?
Created attachment 162499 [details] Draft for interface avoiding direct accesses to ModelComparator
Created attachment 162500 [details] Integration of ICompareInputDetailsProvider in org.eclipse.emf.compare.ui
Created attachment 162650 [details] A patch corresponding to Stephan's contribution
I attached a patch version of your two contributions. I changed your new interface ICompareInputDetailsProvider into an abstract class AbstractCompareInputDetailsProvider. Are you ok with this change? I also added your copyright in this new abstract class and removed the non-API tags from ModelConpareInput constructors & ModelContentMergeViewer.createModelCompareInput() method.
(In reply to comment #8) > I changed your new interface ICompareInputDetailsProvider into an abstract > class AbstractCompareInputDetailsProvider. > Are you ok with this change? It is a good idea to introduce an abstract implementation of the interface. This makes it easier to use for applications. However, I wouldn't completely replace the interface by the abstract class and continue use the interface but not the abstract class for referencing the ModelComparator inside ModelContentMergeViewer and ModelCompareInput. This way, we follow a broadly used pattern which can be observed in plenty other locations as well whereas using an abstract class as reference type would be rather uncommon. > I also added your copyright in this new abstract class and removed the non-API > tags from ModelConpareInput constructors & > ModelContentMergeViewer.createModelCompareInput() method. Cool!
> It is a good idea to introduce an abstract implementation of the interface. > This makes it easier to use for applications. However, I wouldn't completely > replace the interface by the abstract class and continue use the interface but > not the abstract class for referencing the ModelComparator inside > ModelContentMergeViewer and ModelCompareInput. This way, we follow a broadly > used pattern which can be observed in plenty other locations as well whereas > using an abstract class as reference type would be rather uncommon. I do aggree that Abstract classes instead of Interfaces are scarcely used ; yet we did this change to your patch to cope with potential future improvements : adding methods to an interface is an API breakage, whereas adding methods to an abstract class is not. You only need to take a look at all the duplicate interfaces within Eclipse to understand why we'd rather have a class (IContentAssistantExtension, IContentAssistantExtension2, IContentAssistantExtension3, IContentAssistantExtension4 are good examples of the interface problems). We can commit an interface instead of the abstract class, that's your call ... but do take note that future evolutions of this interface might be difficult.
(In reply to comment #10) > I do aggree that Abstract classes instead of Interfaces are scarcely used ; yet > we did this change to your patch to cope with potential future improvements : > adding methods to an interface is an API breakage, whereas adding methods to an > abstract class is not. You only need to take a look at all the duplicate > interfaces within Eclipse to understand why we'd rather have a class > (IContentAssistantExtension, IContentAssistantExtension2, > IContentAssistantExtension3, IContentAssistantExtension4 are good examples of > the interface problems). I see your point, but isn't the consequence of this that we eventually should never again use interfaces and always prefer abstract classes for all API? That looks odd to me. > We can commit an interface instead of the abstract class, that's your call ... > but do take note that future evolutions of this interface might be difficult. Yes, I'd like to call for the interface solution. I've designed the ICompareInputDetailsProvider interface such that it exposes not more and not less than the ModelComparator methods which ModelCompareInput needs to access. Consequently, the ICompareInputDetailsProvider interface will be as stable as the interactions between ModelCompareInput and ModelComparator.
(In reply to comment #11) > Yes, I'd like to call for the interface solution. I've designed the > ICompareInputDetailsProvider interface such that it exposes not more and not > less than the ModelComparator methods which ModelCompareInput needs to access. > Consequently, the ICompareInputDetailsProvider interface will be as stable as > the interactions between ModelCompareInput and ModelComparator. Then an interface it will be :). This will be commited on HEAD today and accessible in the M7 build due may, 4.
Created attachment 164024 [details] Updated patch to use interface instead of abstract class
Thanks for the updated patch. This has now been commited on cvs HEAD and will be built with EMF Compare 1.1 M7
