Community
Participate
Working Groups
Created attachment 90801 [details] Patch implementing multiple content-types support When multiple content-types are defined on the same, given resource, Compare simply ignores all but the first defined. It would be better to seek through the defined content-types to find one that is bound to either a ContentMergeViewer, StructureMergeViewer, or both. Let's take my own issue as an example: I had defined a content-type 'org.eclipse.compare.EMFCompare' which was defined on files of type (among others) '*.ecore'. In Eclipse 3.4, EMF has introduced a new content-type 'org.eclipse.emf.ecore' which is also defined on files '*.ecore'. Now, when I try and compare two ecore files, CompareUIPlugin#getContentType() will simply return 'org.eclipse.emf.ecore' and ignore the fact that another could be better. Indeed, my 'EMFCompare' content-type had bindings to both a ContentMergeViewer and StructureMergeViewer, whereas EMF's did not define either of those bindings. I am attaching a patch to this bug that is aimed at the latest CVS version of org.eclipse.compare 3.4. This patch alters CompareUIPlugin#getContentType() and CompareUIPlugin#getCommonType() to implement this change. Does this seem like something you could commit on HEAD or are there foreseeable problems I have overlooked? Another problem still remaining with this patch is the way compare should handle when not only two (or more) content-types are defined on a given element, but each content-type also defines binding to both types of viewers. The best option would probably be to add a "priority" attribute to the org.eclipse.compare.ContentMergeViewers and org.eclipse.compare.StructureMergeViewers extension points.
Bug 201116 is related to this (perhaps even a duplicate) and has a discussion on possible solutions. The problem with the patch presented here is that it is a point solution and has the potential to break clients (i.e. what if a product ships a merger associated with the EMF content type and the users installs your plug-in. The user may prefer one over the other but the choice of the winner is essentially random and the user wouldn't be able to pick one without uninstalling the other conflicting plug-in).
Bug 201116 is indeed similar, but this one remains slightly distinct : Bug 201116 deals with multiple content-type all shipped with a binding to (Content|Structure)MergeViewer. This bug deals with the issue of multiple content-types (say two) with one of the two useless for compare (has no (Content|Structure)MergeViewer bound to it) yet this useless one is the one that is chosen. With this patch, the "random" way of selecting a content-type when there are multiple content-types defining a viewer is still present (this needs to be dealt with too, but this is why bug 201116 has been filled in), but we no longer take useless content-types into account. As for the example you give ... Uninstalling conflicting plug-ins is *already* how it must be done with the current compare in order to choose another viewer; this is what #201116 must fix.
Created attachment 90836 [details] Patch implementing multiple content-types support I tested the previous patch in debug, and din't realize I wasn't initializing the (Content|Structure)MergeViewers registry on the first comparison, preventing the search for a StructureMergeViewer to conclude successfuly.
Michael, This is also related to https://bugs.eclipse.org/bugs/show_bug.cgi?id=104005 where I don't think it's feasible for us to define a common "EMF" base content type for the model compare framework to use.
Hi, Any news on this issue? Tested this in 3.5M3 and compare still takes a random content-type ("random" issue is to be handled by bug 201116 ) within the elligible candidates even if that candidate isn't bound to (Content|Structure)MergeViewer(s). This causes us trouble for EMF Compare in many cases because of WTP and other plugins that define content types on some generic extensions (xmi comes to mind). This patch hasn't (couldn't ? Doesn't break API and only alters a non documented yet random behavior) been applied for 3.4. Is there any chance it will be for 3.5?
Laurent, have you tried to associate the new "org.eclipse.emf.ecore" content type from comment 0 with a project nature? An excerpt of "org.eclipse.core.resources.natures" extension point description[1]: "Starting with release 3.1, natures can declare affinity with arbitrary content types, affecting the way content type determination happens for files in the workspace. In case of conflicts (two or more content types deemed equally suitable for a given file), the content type having affinity with any of the natures configured for the corresponding project will be chosen." Your patch looks fine, but with a little change on your side we could probably get the same effect basing only on the result from the Content Type Manager. Would that work in your case? [1] http://help.eclipse.org/stable/topic/org.eclipse.platform.doc.isv/reference/extension-points/org_eclipse_core_resources_natures.html btw, bug 201116 has been fixed allowing to switch to a different content merge viewer when Compare Editor is open.
Hi Tomasz, I haven't tried associating my content-type with a nature ... and this seems way too intrusive, not to mention it would have chances to break the behavior of other plugins relying on such associations. Regardless of the intrusive nature of this change, seems to me it would only delay the fixing of this bug : it would only takes two content-types associated on the same project nature to break my fragile fix.
I see your point Laurent and I must admit you're right, this would not be a complex solution. Even though your patch looks reasonable it doesn't cover all the cases as well, imho. You mentioned in the code that adding priorities to content/structure merge viewers would help here. That's definitely true, but I'm pretty sure that this would be simply a matter of time before we meet a situation with two equals priorities. What if we add an option to select a structure merge viewer[1] to use and a way to remember the selection. The only thing you would have to do then would be providing a human-readable label for your content viewer(s)[2]. How does it sound? [1] a similar thing to what was done in bug 201116 for content merge viewers [2] so far an optional attribute "label" has been added to contentMergeViewer ext. point definition
Hi Tomasz, Indeed this latest solution seems to be for the best, that would most likely fix the issue I am facing as well
Laurent, I've just filed bug 271120 which is about providing a way to prioritize available merge viewers so the user can decide which one will be used in compare. The solution proposed there is more about setting preferences (and remembering them) rather then giving and option to change default selection made by Compare (which is basically what bug 201116 did for Content Merge Viewers and what I suggested here for Structure Merge Viewers in comment 8). The question I would like to ask you is: do you think the new bug should be set as a blocker to this one or are you fine with adding an option to select a structure merge viewer only? If you say that to mark this bug as fixed adding such an option is enough, I guess, it can be done in 3.5. If not, I will open a separate bug for it and mark this one as blocked by both bug 271120 and the add-switch-to-structure-viewer one. It's up to you.
Hi Tomasz, bug 271120 seem like a truly nice addition, yet I don't think it should be set as a blocker to this bug. The issue here is only to have compare select (or let the user select) the accurate viewers when there is one registered against one of the file's content-types. Your comment 8 suggestion is enough for this issue. Still looking forward for the resolution of bug 271120 even though it won't be blocking anymore :). Laurent
Created attachment 131114 [details] Initial patch The patch the user to select the accurate structure viewer when there is more then one registered. Laurent is there are chance you could give it a try before I apply it to HEAD? It's not a final version, but gives the idea, so it would be great if I could hear from you if it works as you would expect.
Created attachment 131115 [details] mylyn/context/zip
Hi Tomasz, I had quite a bit of trouble applying the patch on HEAD (mostly because of incompatibilities of your code with M6 brought by bug 268950) but eventually managed :p. The patch looks good and indeed fixed our issues with multiple compare and structure viewers. I'll have to adapt my code a little though ... but not too much and due to internal usage from my side (see my last comment on bug 210001). Will this be present in the final compare 3.5 release? Thanks a lot!
PS : just a side note : dependencies mentionned in the org.eclipse.compare MANIFEST have wrong version constraints (there may be more, but at least those caused me some trouble) : ### Eclipse Workspace Patch 1.0 #P org.eclipse.compare Index: META-INF/MANIFEST.MF =================================================================== RCS file: /cvsroot/eclipse/org.eclipse.compare/plugins/org.eclipse.compare/META-INF/MANIFEST.MF,v retrieving revision 1.24 diff -u -r1.24 MANIFEST.MF --- META-INF/MANIFEST.MF 23 Feb 2009 16:50:02 -0000 1.24 +++ META-INF/MANIFEST.MF 8 Apr 2009 09:40:32 -0000 @@ -13,12 +13,12 @@ org.eclipse.compare.internal.patch;x-internal:=true, org.eclipse.compare.patch, org.eclipse.compare.structuremergeviewer -Require-Bundle: org.eclipse.ui;bundle-version="[3.3.0,4.0.0)", - org.eclipse.core.resources;bundle-version="[3.3.0,4.0.0)", +Require-Bundle: org.eclipse.ui;bundle-version="[3.5.0,4.0.0)", + org.eclipse.core.resources;bundle-version="[3.4.0,4.0.0)", org.eclipse.jface.text;bundle-version="[3.3.0,4.0.0)", org.eclipse.ui.ide;bundle-version="[3.3.0,4.0.0)", org.eclipse.ui.views;bundle-version="[3.2.0,4.0.0)", - org.eclipse.ui.workbench.texteditor;bundle-version="[3.3.0,4.0.0)", + org.eclipse.ui.workbench.texteditor;bundle-version="[3.5.0,4.0.0)", org.eclipse.core.runtime;bundle-version="[3.2.0,4.0.0)", org.eclipse.core.expressions;bundle-version="[3.2.0,4.0.0)", org.eclipse.ui.editors;bundle-version="[3.3.0,4.0.0)",
(In reply to comment #14) > Will this be present in the final compare 3.5 release? Hopefully it will. I will give the patch a second look soon and if you're happy with it release it to HEAD. (In reply to comment #15) > PS : just a side note : dependencies mentionned in the org.eclipse.compare > MANIFEST have wrong version constraints Szymon is working on the issue, thanks for pointing this out. Our bad.
(In reply to comment #16) > PS : just a side note : dependencies mentionned in the org.eclipse.compare MANIFEST have wrong version constraints Filed bug 271612.
Created attachment 132074 [details] Patch v02 Updated version of the patch, fixed TODOs related to unchecked casts to ICompareInput in CompareStructureViewerSwitchingPane.
The latest patch has been released to HEAD. Available in builds >N20090416-2000. Filed bug 272668 for a potential selection issue and updated bug 269959 about duplicated code (see bug 269959, comment 3). Any comments from adopters/clients are mostly welcome... Laurent feel free to ping me/file a bug if come across any issues.
