Bugzilla – Full Text Bug Listing
| Summary: | [Patch] Files selected in synchronize perspective are often not checked by default in create patch dialog | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Krzysztof Michalski <krzysztof.michalski> | ||||
| Component: | CVS | Assignee: | Tomasz Zarna <tomasz.zarna> | ||||
| Status: | VERIFIED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | benno.baumgartner, bugzilla, daniel_megert, dj.houghton, krzysztof.daniel, Szymon.Brandys, tomasz.zarna | ||||
| Version: | 3.4 | Flags: | Szymon.Brandys:
review+
|
||||
| Target Milestone: | 3.4 RC1 | ||||||
| Hardware: | PC | ||||||
| OS: | Windows XP | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Krzysztof Michalski
It looks like the selected changes (resources) are saved between dialogs, but not used when creating a patch itself. They are not used to calculate dialog's enablement either. *** Bug 201756 has been marked as a duplicate of this bug. *** I didn't check the code behind that dialog but note that bug 201756 reports two different problems - both related to the dialog initialization: 1. lots of flickering in the tree 2. wrong initial state. *** Bug 204578 has been marked as a duplicate of this bug. *** *** Bug 205780 has been marked as a duplicate of this bug. *** (In reply to comment #0) > Build ID: I20070809-1105 > > Steps To Reproduce: > 1. Select project. > 2. Open create patch wizard. > 3. Uncheck all changes and click cancel. > 4. Open it once again and now project changes are unchecked. > 5. Click Finish. > 6. Suprise: patch was created with all project changes. > Check/uncheck changes doesn't work properly. > Although I can not prove it: I'm almost certain that a couple of my patches have been create without all changes lately. But this aside, IMHO this is a must fix for 3.4. Let me take care of this, but I can't promise I'll make it for 3.4. *** Bug 230027 has been marked as a duplicate of this bug. *** This is fairly bad - we almost lost some changes today because the patch didn't capture all the changes. Most people I believe just select a set of projects, then create patch and immediately click finish. By default everything in the selection before the dialog was opened just be included in the dialog selection. Persisting checked state across invocations of the dialog doesn't make sense because the new patch may be completely unrelated to the one created last time. (In reply to comment #9) > This is fairly bad ... True, I think it should be fixed in 3.4. Apparently, GenerateDiffFileWizard.LocationPage.initCheckedItems() method is called before syncInfoCollector from the SubscriberParticipant[2] returns all/any the changes. Which is strange since this what WaitForChangesJob[3] is for. Maybe the model is changed and new changes appear because of an additional collector/subscriber running in the background? Investigating... [1] org.eclipse.team.internal.ccvs.ui.wizards.GenerateDiffFileWizard.LocationPage.initCheckedItems() [2] org.eclipse.team.ui.synchronize.SubscriberParticipant / org.eclipse.team.internal.ccvs.ui.subscriber.CreatePatchWizardParticipant [3] org.eclipse.team.internal.ccvs.ui.wizards.GenerateDiffFileWizard.LocationPage.getAllOutOfSync().WaitForChangesJob Created attachment 100188 [details]
Fix
Here is one of possible fixes for this issue. The patch extends the time wizard waits before checking items in the change tree. It also adds extra checks whether the event handler job is done. In fact, if I understood the code correctly, the job might reschedule itself, so determining whether it's done or not (processed all changes) is quite hard. This is why it's not the best solution for this issue, nevertheless with the patch applied I didn't manage to get the wizard in the invalid state.
As a consequence, of increasing the "wait for collector" time it's now much easier to observe that the tree is created without any selection and then, after a while, all items are checked. The same thing with initial error message saying that "No changes has been selected" which shows up for a moment. The later is rather easy to fix (no dialog should start up with an error message), but the first one is made intentionally and I'm not sure if there is something we can do about it without changing the logic of events queuing and handling.
Szymon has also looked at the code, so I'm sure he will have some comments... as the reviewer.
What do you think Sim, will it make for RC1? This is an improvement indeed. Hoverer we both know that this is not a complete fix. I would release the fix since the create patch dialog works better with it. However the bug should be left open. +1 for the improvement. Fix applied to CVS HEAD. Leaving the bug opened, awaiting for a final solution. The fix is in 3.4RC1, however as mentioned above, I'm leaving the bug opened. Looking at the CC list I can see there is couple of people interested in having this fixed. I would like to ask you guys if the change made in RC1 is acceptable in your opinion. As I said, this is not the perfect solution, but my intention was to lower the chance of getting the erroneous state. Let me know if any of you bump into this issue again. If not I will mark this bug as fixed and open a new one in search of an optimal solution. Please adjust the target milestone, so it does not point at a closed milestone in the past. The quick fix was released in RC1. We can't reproduce the issue with the applied fix, however there can be still some exceptional cases hard to reproduce for us (see comment 16). I think that we can mark it as FIXED. If the fix was not sufficient and the issue still existed, we would REOPEN the bug for 3.5 and/or raise a new one for major refactoring in the create patch dialog. Verified, haven't seen it in neither RC2 nor RC3. (In reply to comment #14) > This is an improvement indeed. Hoverer we both know that this is not a complete > fix. I would release the fix since the create patch dialog works better with > it. > However the bug should be left open. (In reply to comment #15) > Fix applied to CVS HEAD. Leaving the bug opened, awaiting for a final solution. Sorry, I am confused. The bug is marked "Fixed" but I can't find what the "real solution" was. Would disabling the tree and "next"/"finish" buttons until checkmarks are retrieved help here? See also bug 294650 comment 24: > bug 201714 was a timing issue that still may be reproduced by creating a > breakpoint somewhere in the code updating the tree in create patch wizard. |