Bug 171683 - [Sync View] Cannot open two compare editors from Synchronize view at once
Summary: [Sync View] Cannot open two compare editors from Synchronize view at once
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Team (show other bugs)
Version: 3.3   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.3 M7   Edit
Assignee: Tomasz Zarna CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed
Depends on:
Blocks:
 
Reported: 2007-01-25 10:40 EST by Markus Keller CLA
Modified: 2007-06-05 15:14 EDT (History)
1 user (show)

See Also:


Attachments
My screenshot (27.92 KB, image/png)
2007-03-12 11:48 EDT, Tomasz Zarna CLA
no flags Details
Patch file (4.24 KB, patch)
2007-03-13 06:19 EDT, Tomasz Zarna CLA
no flags Details | Diff
Patch file (5.72 KB, patch)
2007-03-15 09:51 EDT, Tomasz Zarna CLA
no flags Details | Diff
Patch file (7.59 KB, patch)
2007-03-16 10:37 EDT, Tomasz Zarna CLA
no flags Details | Diff
Patch file (11.64 KB, patch)
2007-03-18 15:28 EDT, Tomasz Zarna CLA
no flags Details | Diff
Patch file (13.66 KB, patch)
2007-03-19 12:25 EDT, Tomasz Zarna CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Keller CLA 2007-01-25 10:40:18 EST
I20070123-1715

I really appreciate the possibility to open multiple compare editors at the same time. However, I found no way to select multiple changes in the synchronize view and open them all at once:

The Enter key just opens a compare editor for one of the selected changes
=> expected: opens all changes

With multiple incoming CVS changes selected, the context menu contains just a disabled entry "Open Change in Compare Editor"
=> expected:
- should have "Open in Compare Editor" (opens multiple editors)
- "Open Changes in Compare Editor" should either be enabled (and work with multiselection), or it should not appear in the menu at all
Comment 1 Michael Valenta CLA 2007-02-13 10:32:42 EST
The logic for this would need to be implemented in the OpenInCompareAction.
Comment 2 Tomasz Zarna CLA 2007-03-12 11:07:24 EDT
I20070306-1200

When trying to reproduce reprorted behavior I didn't notice the second case you wrote about. In other words, the disabled "Open Change in Compare Editor" entry didn't show up with multiple incoming changes selected. So in I20070306-1200 this action is available only for single selection and now it's called "Open In Compare Editor".

However, the above doesn't make the bug WORKSFORME. I will work on it. 

Comment 3 Markus Keller CLA 2007-03-12 11:16:38 EDT
To exactly reproduce comment 0:
- disable "Allow models [..]" on Preferences > Team > CVS > Synchronize/Compare
- remove all synchronizations from the Synchronize view
- synchronize again
Comment 4 Tomasz Zarna CLA 2007-03-12 11:48:22 EDT
Created attachment 60559 [details]
My screenshot

This is what I have. As you see, there is no disabled entry you mentioned. Although I'm sync'ing two working sets (WS1 and WS2).
Comment 5 Markus Keller CLA 2007-03-12 11:54:28 EDT
The gray "Open Changes in Compare Editor" appears in incoming mode with change sets enabled and a selection comprising incoming changes from multiple change sets.
Comment 6 Tomasz Zarna CLA 2007-03-13 06:19:09 EDT
Created attachment 60651 [details]
Patch file

1. "Open in Compare Editor" (double-click, enter) now opens mulitple compare editors for multiselection. To achieve that I changed run method in OpenInCompareAction to work with collection of objects from a selection. Then I modified a condition in findReusableCompareEditor method in the same class. IMO an editor can be reusable only when we have all conditions met:
* given input equals to the one from an editor
* open editor doesn't have unsaved changes
* an option for reusing editors is enabled in preferences

2. "Open in Compare Editor" is now visible for multiselection. See changes in fillOpenWithMenu method in OpenWithActionGroup

3. I decided to not hide "Open Change in Compare Editor" entry for invalid selection. Here are my points:
a) Actual behavior is intended. Please see comment in updateSelection method in OpenChangeSetAction (line 103): "(...) only enable if the selection is contained within a single change set"
b) Markus, if you're not satisfied with 1) I will open a new bug with this issue. However, it would be much easier for me to fix that after new (declarative) menu support is introduced.
Comment 7 Michael Valenta CLA 2007-03-14 13:17:27 EDT
I've tried that patch and I have encountered a few problems:

1) There are actually two Synchronization types. The fix only works for one type. The other action group that needs to be changed is ModelSynchronizeParticipantActionGroup. Here is a link to the wiki that describes the two synchronization types.

http://wiki.eclipse.org/index.php/CVS_FAQ#Synchronizing_has_some_regressions_in_Eclipse_3.2._Why.3F

2) In the OpenWithActionGroup.java class, you enable for multiple resources but not for multiple objects that have a compare input available. There is a check near the top of the fillContextMenu that has the condition "elements.length == 1". You would need to modify this to accept open an editor for multiple elements.

3) You should test your fix with the Team>Resuse compare editor preference on and off to ensure that it behaves well for both cases.
Comment 8 Tomasz Zarna CLA 2007-03-15 09:51:59 EDT
Created attachment 60936 [details]
Patch file

1) I've added multi-selection support for ModelSynchronizeParticipantActionGroup as you requested.
2) Also The check in the OpenWithActionGroup.java class has been modified.
3) Finally, I've withdrawn my changes in findReusableCompareEditor method(OpenInCompareAction.java class). I took wrong assumptions about how Reuse should work. Thanks for the subtle suggestion Mike :)
Comment 9 Michael Valenta CLA 2007-03-15 14:13:33 EDT
You're almost there. Here are my comments.

1) I tried the latest patch and if I have the Reuse editor preference on, I can select multiple files but I end up with only a single editor open. Hence, when reuse is on, we either need to disable for multiple elements or not reuse editors. I think the later would be good.

2) Also, in ModelSynchronizeParticipantActionGroup, you enable for multiple elements but you only check the first to see if it has an input. I think a better approach would either be to enable only if all of them have inputs or if at least on of them has inputs. I think I prefer the later.

Comment 10 Tomasz Zarna CLA 2007-03-16 10:37:45 EDT
Created attachment 61100 [details]
Patch file

(In reply to comment #9):

1) I did as you asked me to. Opening compare editors for multiple elements does not not reuse opened editors now. However, this changes introduced small API change - I needed to pass boolean isMulitpleselection argument to public method (please see openCompareEditor method in OpenInCompareAction.java class). Is my solution acceptable? Should we mention that in the checkbox's label in Team preferences?

2) Same as above: Open In Compare Editor entry will appear when at least one of selected elements has inputs.
Comment 11 Michael Valenta CLA 2007-03-16 11:37:38 EDT
Your getting there. You actually didn't introduce an API change because the class you modified is internal. Given that you have the selection in the run method, I would set the boolean there and push it down through the two open method called from there. I would also change the boolean to be called reuseEditorIfPossible since that is what it translates to at the lowest level. As for the preference page, I don;t think we need to change what it says.

One thing I tried was to select a file and folder in the packages explorer. When I do that, the Open is not available. But when I select multiple files it is. We should probably have the same behavior (i.e. if one of the selected items is not openable, then the menu item shouldn't be added).
Comment 12 Tomasz Zarna CLA 2007-03-18 15:28:17 EDT
Created attachment 61220 [details]
Patch file

Boolean parameter's name changed to reuseEditorIfPossible.

To determine whether selected resources are openable, I'm looking for "Open" item in a context menu. I belive this should work for this particular action group. On the other hand, OpenWithActionGroup.java class seems to have conditions for showing "Open" and "Open in Compare Editor" items already defined. There is a situation when only the later is displayed (when an item being synchronized is not available locally, incoming).
Comment 13 Michael Valenta CLA 2007-03-19 09:08:22 EDT
The "Open" menu will only appear on files. The proper check is the one used in the ModelSynchronizeParticipantActionGroup#fillContextMenu method. Specifically, the line "msp.hasCompareInputFor(elements[i])". I think you should loop through all the selected items and only add the openInCompareEditor if all the elements have a compare input. Other than that, the patch looks good.
Comment 14 Michael Valenta CLA 2007-03-19 09:09:15 EDT
One other things. The latest patch causes a compile error in the CVS/Tests plug-in. Please include that fix as part of the patch.
Comment 15 Tomasz Zarna CLA 2007-03-19 12:25:06 EDT
Created attachment 61305 [details]
Patch file

Rule "all or none" applied. Check is made before displaying "Open in Compare Editor" entry in a context menu and before opening Compare Editor itself (isOkToRun is called in run method of OpenInCompareAction.java class). The second is introduced to prevent from opening compare editor(s) (hitting enter), when the "Open in Compare Editor" entry is hidden.
Comment 16 Michael Valenta CLA 2007-03-21 10:57:13 EDT
I modified the isOKToRun method so that it tested either for model-based or resources but not both. Path released to HEAD.
Comment 17 Tomasz Zarna CLA 2007-04-26 05:32:50 EDT
Verified in I20070424-0930.