Community
Participate
Working Groups
Build Identifier: M20100211-1343 Consider a new file in the workspace, that is about to be committed to CVS. When double clicking the file in the synchronize view, the file type is determined in order to find the best merge viewer for that file. In this scenario, where the file is new, the type can only be determined for the local side. Unfortunately, the type is not returned -- null is returned instead. See CompareUIPlugin.getCommonType(): if (ancestor != null) { type= getContentType(ancestor); if (type != null) types[n++]= type; } type= getContentType(left); if (type != null) types[n++]= type; else return null; type= getContentType(right); if (type != null) types[n++]= type; else return null; A few lines later, the number of found types is evaluated (see the switch statement) and the types are compared. However the cases where n==0 and n==1 can never be reached, because of the "return null" before. I suggest removing those returns, as I think that was intended and makes the most sense. See the attached patch. Reproducible: Always
Created attachment 167277 [details] Returns the content type if just one side (left,right,ancester) is available
Carsten, thanks for the patch. There won't be any changes in that code in 3.6 though (we're in RC1). I'll try to review it it 3.7.
I thought *now* is the time for bugfixes? Only new features are not allowed anymore. The current situation with this bug is that you won't get the correct content type (and therefore diff viewer) for new or deleted files, unless the correct content type can be guessed. What happens here is that we get the plain text diff viewer for our XML based documents, which is not very helpful.
Ping... any new on the patch?
Carsten, is this bug somehow related to bug 293926? I just glanced at this one, but I think they look similar.
(In reply to comment #5) Indeed they look similar, but as far as I can see, these are two separate issues. This issue here really is just about getCommonType(), which currently only works if both sides are available. If any side is unavailable, it gives up instead of returning the type of the available side.
I proclaim that this is my lucky day, so I've invested some time to update the patch to the current master branch in git and submit a gerrit request. https://git.eclipse.org/r/#/c/17248/ Can somebody please review it? Thanks a lot!
As requested, here's a detailed description of the scenario in question. 1. You have a project under revision control 2. You have a file that is not yet in revision control 3. You attempt to compare the file with the repository => you get a plain text compare viewer instead of the one that can actually display the contents in a logical manner. The same happens when the file is new in the repository, but not available locally. You can try this out by creating a new java file and compare that with the repository. BUT: in this case you *do* get the correct Java compare viewer! The reason is that CompareUIPlugin.findContentViewerDescriptor(Viewer, Object, CompareConfiguration) uses the file extension to find a viewer when it cannot determine the content type: if (type != null) { initializeRegistries(); List list = fContentMergeViewers.searchAll(type); [...] } This works fine for *.java files, but not in our case because we have a generic compare viewer implementation that works with many different kinds of "documents". These documents use XML syntax, but have a specific content-type and different file extensions. The framework needs to find our generic compare viewer based on the content type. We cannot register the generic compare viewer for all kinds of extensions, because we don't even know all existing extensions (they are defined separately in different plugins). Also, I don't see why you would *not* use the viewer determined from the content type, but prefer to use the one determined from the file extension. That just doesn't make sense. The code was even prepared to do this properly, except it failed to execute it because it returned too early.
> This works fine for *.java files, but not in our case because we have a > generic compare viewer implementation that works with many different kinds of > "documents". Can you give an example based on the SDK that allows to verify the fix, or attach a sample plug-in?
(In reply to Dani Megert from comment #9) > > This works fine for *.java files, but not in our case because we have a > > generic compare viewer implementation that works with many different kinds of > > "documents". > > Can you give an example based on the SDK that allows to verify the fix, or > attach a sample plug-in? I see you added a test (plug-in) to the change. Thanks. Can you post the steps on how one can test/see it in the target (I know I could probably learn this by studying the test ;-).
0) In the host workbench, open class TestSample1ViewerCreator and set a breakpoint into the createViewer() method 1) Start a runtime workbench in debug mode that includes the compare.test plug-in 2) Create a project and share it as a git repository 3) In that project, create a file named "foo.testsample1" 4) put the following text into the file: MAGIC BYTES blabla 5) Select the file and invoke Compare with -> HEAD revision With the fix applied, the breakpoint in the createViewer() method should hit. Without the fix, it doesn't hit.
(In reply to Carsten Pfeiffer from comment #11) > 0) In the host workbench, open class TestSample1ViewerCreator and set a > breakpoint into the createViewer() method > 1) Start a runtime workbench in debug mode that includes the compare.test > plug-in > 2) Create a project and share it as a git repository > 3) In that project, create a file named "foo.testsample1" > 4) put the following text into the file: > MAGIC BYTES blabla > 5) Select the file and invoke Compare with -> HEAD revision > > With the fix applied, the breakpoint in the createViewer() method should > hit. Without the fix, it doesn't hit. Sorry for the delay, I was busy with M3. I now see what you mean. Since I didn't want to release the changes to the test bundle, I simply applied your fix with http://git.eclipse.org/c/platform/eclipse.platform.team.git/commit/?id=710e2b0b8dc7efe2a971bc2cec57aeef26ba0932 I've also added your name to the copyright notice and added the signed-off-by. Your example revealed another problem: CompareUIPlugin.getContentType(ITypedElement) returns 'org.eclipse.compare.test.testsample1type' for all files with a 'testsample1' extension even if asked for an IFile that's in the workspace. As a result, comparing such files with local history or HEAD Revision always tries to open "your" viewer. Filed bug 421163 to track that.
