Community
Participate
Working Groups
Build ID: I20070621-1340 Steps To Reproduce: More information: It seems that when several plugins define a contentMergeViewer for the same file extension (e.g. .java) only one contentMergeViewer will be finally registered with that file extension. Moreover the behavior will be dependent on the order of the IConfigurationElement[] array resulting from registry.getConfigurationElementsFor(PLUGIN_ID, CONTENT_MERGE_VIEWER_EXTENSION_POINT); CompareUIPlugin.registerExtensions() will take this IConfigurationElement[] array and will map the file extension (taken from IConfigurationElement.getAttribute(EXTENSIONS_ATTRIBUTE)) to the corresponding viewer, if one file extension was already mapped to a viewer that previous mapping is lost. This make almost imposible to add a new contentMergeViewer for .java files for examples as JDT already provides one. Depending on the actual set of installed plugins the contentMergeViewer for .java that you define will override or not the one defined by JDT.
This is how it works. When two viewers for .java files are registered, you will never know which one will be used when app is started. Maybe one of solutions is to have an extendable viewer for .java.
On second thoughts, I think that we should allow user to switch between those merge viewers inside of the compare view. A preference page should do the trick too, but having it inside of the compare view will be more user-friendly.
I also agree that the user should be able to select which merge viewer he wants to use. Having a way to switch the merge viewer inside the compare viewer would be more user-friendly. Although I think the preference page mechanism is used for similar purposes in Preferences->General->Editors->File Associations. I will look at the preference page mechanism myself and try to provide a patch for it.
Created attachment 77774 [details] Compare Viewer File Associations Preference Page screenshot screenshot of a new tab in the Compare Preference Page that allows to change the association between file extensions and contentMergeViewer implementations.
Created attachment 77776 [details] Compare ContentType association tab in ComparePreferencePage screenshot of a new tab in the Compare Preference Page that allows to change the association between contentTypes and contentMergeViewer implementations.
Created attachment 77777 [details] patch to add the two new tabs to the compare preference page Patch to add the two tabs in the compare preference page. See screenshot at Attachment #77774 [details] and Attachment #77776 [details]
I've created a patch (see Attachment #77777 [details]) to add a preference page to switch the contentMergeViewer whe there is more that one contentMergeViewer for a given file extension or contentType. I added content type associations alos because "Compare with local revision" will require to change the setting at ContentType associations because it uses contentType to find the viewer. Please tell me if this is the way to go and I will try to polish this preliminar implementation.
I have a couple of big concerns with the patch: 1) It shows class names in a dialog. This has really poor usability and is not something I would be willing to introduce. For this to work, the content merge viewer definition would have to have an appropriate name or label added. Unfortunately, at this stage, this is not really helpful since no existing extensions have the name (i.e. such a requirement would be viewed as breaking backwards compatibility) 2) I think content merge viewer selection should work the same as editor selection. That is, there should be a default that is used for the basic Open (or double-click) and perhaps a Compare As menu that contained the list of all available compare types (just like the Open With) menu. 3) Any solution to this would also need to take into consideration the contentViewers and structureViewers. Given the current state of the API, I think we need to hold off until 4.0 on this one (since, with 4.0 we can justify the requirement that the providers of contentMergeViewers need to also provide a label). We could also figure out how to properly consolidate editors, contentViewers, structureViewers and contentMergeViewers.
(In reply to comment #8) > I have a couple of big concerns with the patch: > 1) It shows class names in a dialog. This has really poor usability and is not > something I would be willing to introduce. For this to work, the content merge > viewer definition would have to have an appropriate name or label added. > Unfortunately, at this stage, this is not really helpful since no existing > extensions have the name (i.e. such a requirement would be viewed as breaking > backwards compatibility) I left class names in the dialog because as you point out there is no name or label in the viewer definitition. Maybe it's possible to replace class names with the friendly name of the plugin that is providing it or something like that. Could be that acceptable? > 2) I think content merge viewer selection should work the same as editor > selection. That is, there should be a default that is used for the basic Open > (or double-click) and perhaps a Compare As menu that contained the list of all > available compare types (just like the Open With) menu. Totally agree. The default viewer should be selected through a Preference Page but the user should be able to select which viewer to use via a context menu. That functionality can be added with today's api, can't it? > 3) Any solution to this would also need to take into consideration the > contentViewers and structureViewers. You are right. A comprehensive solution for contentViewers, contentMergeViewers and structureVierwers is needed. I definetively want to look into that also. > Given the current state of the API, I think we need to hold off until 4.0 on > this one (since, with 4.0 we can justify the requirement that the providers of > contentMergeViewers need to also provide a label). We could also figure out how > to properly consolidate editors, contentViewers, structureViewers and > contentMergeViewers. If the main reason to hold it off until 4.0 is because the lack of label/names in the viewer definition I think it can be worked around using the name of the providing plugin instead (as currently there is only one contentMergeViewer per plugin per file extension/content type).
The plug-in name is no better and, although unlikely, it is still possible to have two contentMergeViewers in the same plug-in. We really need to be aware that Eclipse will be used by a wide range of users and showing something like a class name or a plug-in name to represent a merger viewer is just not acceptable. About the only think I can see being acceptable for 3.4 is the following: 1) A "Compare As" submenu is added to the Synchronize view, History view and other appropriate places that launch compares. 2) An additional optional field is added to contentMergeViewer that specifies the label to appear in the Compare As menu 3) Only those content merge viewers that provide the label appear in the Compare As menu.
(In reply to comment #10) > ... showing something like a class name or a plug-in name to represent a merger > viewer is just not acceptable. I would think it's more acceptable to show a class name than to confuse a wide range of users by having Eclipse behave be randomly. Showing the class name is unacceptable, but not knowing which viewer will be used is more unacceptable.
You are assuming that showing class names will make things clearer for the user. I would suggest that, for most users, it would confuse the issue further. Also, it would appear that this is a rare situation so the issue is only causing grief for a small number of users.
(In reply to comment #12) > You are assuming that showing class names will make things clearer for the > user. I would suggest that, for most users, it would confuse the issue further. > Also, it would appear that this is a rare situation so the issue is only > causing grief for a small number of users. Correct - I assume users find predictable behaviour with bad aesthetics clearer than random behaviour regardless of how good it looks. Since it's only causing grief for a small number of users, the class name aesthetic will also only affect a small number of users. We're choosing between 2 evils - poor aesthetics are bad, but buggy behaviour is worse. I'm not voting *for* bad, confusing UI design - I'm voting *against* buggy, unpredictable behaviour.
The reality of the situation is that showing class names instead of meaningfull labels is something that is just never done in the Eclipse Platform. If you don't agree with this, then perhaps you could make a post to the eclipse.platform newsgroup to get a discussion started on the pros and cons of this. As for this particular issue, I have shared my opinion in previous comments on how I would solve the issue. If others have opinions on possible solutions, please share them. However, I suspect that arguing that class names are appropriate as labels in an item list won't get you anywhere unless you can get a general concensus for the Eclipse Platform UI development team that this is an acceptable thing to do.
Guys, imo there is nothing to argue about. In comment 9, Ruben admitted that class names might not be the best solution, at the same time he suggested that using plug-in can be _one_ of other options. I think we all agree that a custom label would be perfect here. Colin, we don't have to "choose between 2 evils". Let's make up a solution which will make satisfied all users (or at least most of them). I'm sure that Ruben is capable of doing that. I would be happy to help him achieving that (as far as I can).
(In reply to comment #15) Agreed - there is no argument. Fixing buggy behaviour trumps everything else, and there's more than one way to skin a cat. I'm not in favour of displaying class names, I'm in favour of fixing the issue - whatever that means.
(In reply to comment #10) > > About the only think I can see being acceptable for 3.4 is the following: > > 1) A "Compare As" submenu is added to the Synchronize view, History view and > other appropriate places that launch compares. > > 2) An additional optional field is added to contentMergeViewer that specifies > the label to appear in the Compare As menu > > 3) Only those content merge viewers that provide the label appear in the > Compare As menu. This sounds good - the behaviour and mechanism I would expect in Eclipse (like "Open With").
Created attachment 125139 [details] Patch v02 The patch contains another approach for this issue. Description: Compare editor opens with the default content merge viewer for the type/extension but the user has now an option to switch it to a different one. He or she can choose either a content merge viewer registered for the same content type/extension or select a viewer for a parent type (e.g. text merge viewer used in text compare is a parent viewer for the one in Java compare). When there is no alternative content viewer available there will be no drop down menu neither. Limitations: - when a content merge viewer has been switched to an alternative one there are cases when this selection won't be restored when opening the Compare Editor again for the same input. In other words, when comparing local files, your preferred content merge viewer will be saved (during session) but when comparing for instance with HEAD it won't. This is caused by the fact that your selection is kept in a map keyed by ICompareInput objects, and not all of them have equals() methods. - switching is limited to content merge viewers, ie when comparing Java files only Java compare part of Compare Editor can be switched. The upper, structure compare pane will stay the same. - selecting a node in a structure comparison pane will open the bottom part of Compare Editor with the default content merge viewer. But, switching it to a preferred one will be remembered during the session. Issues: - the new drop down menu doesn't fit the toolbar well. There is an issue with setting height of a tool item[1]. Using a dummy image does help a little but the menu is still cropped. - when the user tries to switch and the editor is dirty a save confirmation dialog will pop up. This may be found disturbing. [1] http://dev.eclipse.org/newslists/news.eclipse.platform.swt/msg20401.html
Created attachment 125140 [details] mylyn/context/zip
Created attachment 126971 [details] Szymon's patch I made some changes to what I have from Tomasz.
Created attachment 126973 [details] Patch v04 The patch looks good, I've just simplified the code in CompareUIPlugin. There are still some minor issues with this fix, like this one: bug 266471.
Anyway, thanks for the patch/review Szymon I've released the last patch to HEAD. Available in builds >N20090226-2000.
Reopening to assign to myself.
Fixed.
*** Bug 131347 has been marked as a duplicate of this bug. ***
