Community
Participate
Working Groups
I20090311-0100 When looking at the new dropdown menu to switch compare editor view, I found a few UI issues: - default entry should have the radio checkmark initially - "Default" label does not match other labels I've seen in the menu ("Text Compare", "Image Compare"). Could the default be changed to "Default Compare")? - tooltip "Switch" is a bit terse. Maybe "Switch Compare Viewer" or "Switch Presentation"? - the menu should open on mouse down, not on mouse up. Until bug 193318 is fixed, you can do this using a mouse listener on the toolbar, see e.g. org.eclipse.jface.dialogs.PopupDialog.createDialogMenu(Composite)
(In reply to comment #0) > - default entry should have the radio checkmark initially The default entry is not checked initially by design. The checkmark indicates that the item next to it has been selected by user. Let me explain it using a hypothetical example: Consider you have content merge viewers for xml and exsd files (let's call them XML and Schema Compare). When comparing two folders with xml schema (*.exsd) files you'll get Compare Editor with Structure Compare on top. When you select an xml file the XML Compare will open below. The new dropdown menu will allow you to switch to Text Compare if you wish. The default entry (XML Compare) won't be checked as you mentioned in comment 0. If you now click on the schema file in Structure Compare the Schema Compare will open since it's the default viewer for these files. Options available in the dropdown menu would be: Schema Compare (bold, not checked), XML Compare, Text Compare. However, if you clicked the Default Compare (or Text Compare and then went back to Default, which is XML) then when opened the schema file you would get XML Compare as this is what you selected. Options available in the dropdown menu would be: Schema Compare (bold), XML Compare (checked), Text Compare.
Created attachment 129086 [details] Patch v01 Patch addresses other UI issues mentioned by Markus.
Created attachment 129087 [details] mylyn/context/zip
(In reply to comment #1) I see. It still feels a bit strange that I cannot switch back to default any more after I selected a specific compare. And making the default item bold is also slightly abusive: Bolding the default item is usually just for the default action on a *context* menu, and it's not used to show a default *state* but to mark the default *action* that's invoked when the item is doubleclicked or Enter is pressed. Maybe you could add an explicit 'Default Compare' menu item that can also have the radio and that is checked initially?
(In reply to comment #4) > Maybe you could add an explicit 'Default Compare' menu item that can also have the radio and that is checked initially? You're right Markus, here is a couple of scenarios that came to my mind with the default item applied (I'm referring to the same hypothetical example from comment 1, obvious cases omitted): === scenario #4 (compatible selection) open exsd > [v Default*] ------------ [ Schema* ] [ XML ] [ Text ] select XML > [ Default*] ------------ [ Schema* ] [v XML ] [ Text ] open XML > [ Default*] ------------ [v XML* ] <= XML selected (by user), not Default [ Text ] open Schema > [ Default*] ------------ [ Schema* ] [v XML ] [ Text ] === scenario #5 (incompatible selection) open exsd > [v Default*] ------------ [ Schema* ] [ XML ] [ Text ] select Schema > [ Default*] ------------ [v Schema* ] [ XML ] [ Text ] open XML > [ Default*] ------------ [v XML* ] <= XML selected (a fallback from Schema selected by user), not Default [ Text ] open Schema > [ Default*] ------------ [v Schema* ] [ XML ] [ Text ] Would this work for you?
Forgot the legend: v indicates selection * means that the same content merge viewer would be used when selected ------------ is a separator between menu items
Scenarios from comment 5 look fine to me, but adding the explicit 'Default' item would not allow us follow the steps from comment 1 (select XML Compare, go back to Schema with XML Compare selected): === scenario #6 (no label provided) open exsd > [v Default*] ------------ [ Schema* ] <= no label for XML content merge viewer, not in the menu [ Text ] open xml > [v Default*] ------------ [ Current*] <= it's XML but we have no label to show [ Text ] select Current > [ Default*] ------------ [v Current*] [ Text ] open Schema > [ Default*] ------------ [ Schema* ] [v Current*] <= cannot display the item with no label, thus cannot be selected [ Text ] I guess we can get away with dropping this scenario.
Sounds good. For the case where the label is missing, I think it would be better to just "invent" a name than taking the merge viewer completely away from the user just because the label is missing. The invented name could be something like "Unnamed Compare" (with "Unnamed Compare 2" etc. if there are multiple), or even "Compare (*.exsd)" where stuff in parenthesis is taken from the contentMergeViewers contribution's viewer>extensions attribute or the name of the contentTypeBinding>contentTypeId.
Szymon pointed out that in scenario #5 from comment 5 we do not distinguish between situations when XML has been selected by user and it's the result of an incompatible selection made previously. In consequence the user cannot predict what will happen if he open schema comparison again. Szymon suggested to fallback to the "Default Compare" option in such cases.
Created attachment 129590 [details] Patch v02 This patch contains changes made in Patch v01 +discovering label for merge viewers which don't provide any (as suggested by Markus in comment 8) +fallback to the Default Compare option in situations described in comment 9
Patch v02 adds a new method called findContentTypeOrExtensionName which duplicates a lot of code from findContentViewerDescriptor, I should at least extract methods from the common parts.
Created attachment 129823 [details] Patch v03 Updated version of Patch v02.
(In reply to comment #11) > Patch v02 adds a new method called findContentTypeOrExtensionName which > duplicates a lot of code from findContentViewerDescriptor, I should at least > extract methods from the common parts. => bug 269959
Released to HEAD, with a minor change (renamed findContentTypeOrExtensionName to findContentTypeNameOrType). Available in builds >N20090324-2000. Thanks for your comments Markus. If there is still anything you would like me to improve feel free to open a new bug.
