Community
Participate
Working Groups
Created attachment 106813 [details] Adding d'n'd support "Compare with other resource" dialog enables user to add files for comparison conveniently. One can d'n'd files from Package Explorer or enter a relative path to desired resource. I attach an initial patch with this functionality.
* Remove annotations and set compiler compliance level to 1.4 * I would suggest moving your email address to the Contributors section, this is how I've seen it in Eclipse. Use "(" brackets instead of "<" * Add @since 3.4 tag * I guess that the org.eclipse.compare.internal.Messages class isn't used anywhere else, that's why you removed it? Am I right? Other than the above, I can see you're on the right track.
Created attachment 106961 [details] D'n'd and other modifications.
> * I guess that the org.eclipse.compare.internal.Messages class isn't used > anywhere else, that's why you removed it? Am I right? Yes, as well as messages.properties. These files aren't necessary so I removed them. > Other than the above, I can see you're on the right track. Thank you. I added a few modifications to the dialog (some of the ones mentioned in bug 224562). Check the attachment.
To simplify things, I've released the patch to branch "bug_20080710_CompreWithDialog". That way we can incrementally improve things more easily. You should switch to the branch and provide patches from that. Here are my latest comments: * I didn't remove the Messages.java class, I've noticed we filed a separate bug for it (bug 240298). You can provide this part of the patch there. * I suggest to reuse methods in CompareAction and ResourceCompareInput, instead of creating new ones. Try to create a new ISelection/IStructureSelection objects with the Resources you need to pass. Would that work?
(In reply to comment #4) > To simplify things, I've released the patch to branch > "bug_20080710_CompreWithDialog". The branch's name is "branch_20080710_CompareWithDialog", sorry.
Created attachment 107452 [details] Suggested modifications applied.
I've applied the latest patch to the branch (with some minor modifications), and as usually I do have some suggestions :) * The action is enabled for any number of selected items greater then 0 (ie enablesFor="+"), that's fine. What will happen when the user selects 4 or 5 files? Any idea how to handle such case? Maybe we could add them to a drop down menu for every field? This would mix them with history items :/ A separator might come in handy here. Anyway, I think we should log a new bug for it. * Opened bug 241061 to consider the way the dialog opens in certain circumstances * The dialog's title should be without ellipsis, in opposite to the menu item * I think you have switched titles for left and right section * I guess "Left" and "Right" would be enough for a section's title * Dialog size is reset when expanding Ancestor * Location of the "Clear All" button should be reconsidered. Maybe we should keep the clear option only for the Ancestor. What do you think?
One more thing: there are 3 keys CompareMessages.properties which I'm not sure what they are for. The ones related to clipboard.
Created attachment 107717 [details] Latest changes.
(In reply to comment #7) > * The action is enabled for any number of selected items greater then 0 (ie > enablesFor="+"), that's fine. What will happen when the user selects 4 or 5 > files? Any idea how to handle such case? Maybe we could add them to a drop down > menu for every field? This would mix them with history items :/ A separator > might come in handy here. Anyway, I think we should log a new bug for it. So, here it is: bug 241077. > * Location of the "Clear All" button should be reconsidered. Maybe we should > keep the clear option only for the Ancestor. What do you think? I wasn't sure either whether "Clear all" is necessary (see bug 224562, comment 4). Besides, "Clear" buttons in left and right section make dialog a bit wide, so I removed them. Clearing selected file/resource is also possible by deleting it's path so maybe even the button in ancestor section isn't necessary? (In reply to comment #8) > One more thing: there are 3 keys CompareMessages.properties which I'm not sure > what they are for. The ones related to clipboard. I forgot to remove them before creating a patch. Now they are gone.
Created attachment 107944 [details] Latest patch improved
(In reply to comment #10) > > * Location of the "Clear All" button should be reconsidered. Maybe we should > > keep the clear option only for the Ancestor. What do you think? > > I wasn't sure either whether "Clear all" is necessary (see bug 224562, comment > 4). Besides, "Clear" buttons in left and right section make dialog a bit wide, > so I removed them. Clearing selected file/resource is also possible by deleting > it's path so maybe even the button in ancestor section isn't necessary? I see your point, but using a button is much faster. To make it smaller you could use a button with an icon (like the Remove button from the Console view). > (In reply to comment #8) > > One more thing: there are 3 keys CompareMessages.properties which I'm not sure > > what they are for. The ones related to clipboard. > > I forgot to remove them before creating a patch. Now they are gone. That's what I thought. Some comments: * I know that are some conventions how dialogs should be titled/described. I hope changing dialog's title to "Compare with Other Resource", title in the title area to "Select resources to compare", adding a full sentence describing what the user should or can do as a message to the title area would get us closer to meeting the standard. Take a look at other dialogs based on TitleAreaDialog. * A more descriptive information how the dialog work should be placed under Help (available by hitting F1, or click the icon on the bottom left) => separate bug, P5 * I would change comparePossible to isComaprePossible * If you want to make InternalSection not to be instantiate using a non argument constructor you may want to make the constructor private * Not sure if this is possible but imo resizing the dialog after the Ancestor panel gets expanded/collapsed would look better comparing to the empty, gray area we're seeing now => if possible, separate bug * After selecting 3 files to compare, and clicking OK on the dialog I'm still getting a second dialog which asks me to choose which of the three files should be used as a common ancestor in the 3-way compare. Keep up the good work Ola, with every patch we're getting closer to fixing this bug and merging it to the main trunk.
The latest patch released to the branch.
(In reply to comment #13) > The latest patch released to the branch. It looks like I released it to the "old" branch for the org.eclipse.compare, not org.eclipse.compare/plugins/org.eclipse.compare[1] as I should. Did it again to the proper branch. [1] See bug 207704 for more details.
Created attachment 108052 [details] Next step Main change: SelectAncestorDialog doesn't open from "Compare with other" dialog.
(In reply to comment #12) > * I know that are some conventions how dialogs should be titled/described. I > hope changing dialog's title to "Compare with Other Resource", title in the > title area to "Select resources to compare", adding a full sentence describing > what the user should or can do as a message to the title area would get us > closer to meeting the standard. Take a look at other dialogs based on > TitleAreaDialog. So there is one. I think it also solves issues mentioned in bug 241061. > * A more descriptive information how the dialog work should be placed under > Help (available by hitting F1, or click the icon on the bottom left) => > separate bug, P5 Bug 241643 opened for that. > * Not sure if this is possible but imo resizing the dialog after the Ancestor > panel gets expanded/collapsed would look better comparing to the empty, gray > area we're seeing now => if possible, separate bug I think it's possible. (bug 241649)
Created attachment 108194 [details] patch #6
Created attachment 108533 [details] patch #7 * CompareWithOtherAction is now a subclass of CompareAction. * CompareWithOtherDialog is now displayed from ResourceCompareInput#setSelection instead of SelectAncestorDialog if appropriate parameter is passed.
(In reply to comment #18) > * CompareWithOtherAction is now a subclass of CompareAction. > * CompareWithOtherDialog is now displayed from ResourceCompareInput#setSelection > instead of SelectAncestorDialog if appropriate parameter is passed. Great! I applied the latest patch to the branch. Did some minor modifications, added comments mostly. I think it's ready to merge it with main stream. I will put up a patch against HEAD, ask for IP review and when approved I will commit it.
Ola, could *you* attach the final patch so it would be clear that you're the author? ;)
Created attachment 108951 [details] patch #8 So, here it is :)
I confirm that I wrote 100% of the code in the provided patches. This code may be contributed to Eclipse. All file headers in files created by me contain the EPL License header.
Latest patch released to HEAD. We are leaving the branch.
Verified in I20080918-0100, but logged bug 247841.
