Community
Participate
Working Groups
Not yet in HEAD, reproducible on branch_20090126_LocalDiff branch As stated in bug 260452, GenerateDiffFileWizard from Compare is a modified copy of GenerateDiffFileWizard from CVS UI. We agreed this is temporarily acceptable... but will be fixed in bug 260452. Until then, both wizards should be identical except these places that meant to be different. In other words, here are some things I would keep the same as in the original GenerateDiffFileWizard: * INITIAL_WIDTH and INITIAL_HEIGHT should be private * LocationPage class should be left public * Buttons in OptionPage should be left private * Don't change whitespaces, even if they are misplaced * If there is a comment out code in the orignal wizard I guess it's to have it in the copy as well. Comparing will show less diffs, and bug 260452 will remove it eventually. * Don't change comments, if it says "ignore" let's keep it this way I'm pretty sure there is more, so make sure you go through the changes carefully.
Created attachment 130552 [details] Patch_v1 > * INITIAL_WIDTH and INITIAL_HEIGHT should be private > * LocationPage class should be left public > * Don't change whitespaces, even if they are misplaced > * If there is a comment out code in the orignal wizard I guess it's to have it in the copy as well. Comparing will show less diffs, and bug 260452 will remove it eventually. > * Don't change comments, if it says "ignore" let's keep it this way fixed. * Buttons in OptionPage should be left private we changed diffTypeRadioGroup and unifiedRadioGroup modifier to protected, because in method addPages we create a sublass of OptionsPage, where we override some methods, to create custom patch root controls. We chose to do this like that, beacuse it will be easier in the future to resolve bug 260452. Common (for both wizards) features of OptionsPage are kept in declared class OptionsPage, and only different features are added by overriding some methods. The cvs wizard will be also able to make his own subclass of OptionsPage, taken from local diff wizard and implement his specific features.
Tomasz, could you please review this bug as soon as possible and release it to the branch. It will be much easier for us to update properly bug 270474 and continue with work. You also mentioned that in comment to bug 270474. Thanks.
One more comment. In this patch I remove method performSpecificActions(), which you mentioned in your comment to bug 270474. This method belonged to totally bad idea we had, now it is corrected. I thought, that removing it in a refactoring bug is a good idea.
Thanks for the patch Kacper. I'm fine with removing the performSpecificActions() method here but I don't like the idea of preparing the patch in the way, so it will be easier to fix another bug. The fix for this bug should stick to the issue we're dealing here with. This may complicate the patch for bug 260452, but at least we will have a clear view at the code until then, and what's more important the patch attached to this bug will have no references to other issues. Having said that, I still think you should have a second look at the patch, because it did manage to observe some places that can be corrected. On the other hand, if you think something should be kept the way you did it in the new wizard, you could open a bug against the old one and attach a patch there.
Thanks for the fast comment Tomasz, I understand your point of view. I totally agree that we shouldn't put changes for other bugs to the patch. However, patch which I have attached to this bug, does not provide any changes connected to bug 260452. The only changes it makes is the refactoring you proposed in the comment above and removal of method performSpecificActions() and moving it's code to a different place. The changes in the local diff wizard, which I was talking about in my previous comment (separating common features of OptionsPage, and adding diffrent featrues in overriden methods), were done some time ago (in january) in the patches to bug 71374, and this solution eventually got to the branch. So, I would be grateful if you could give me some clues on how to improve this patch, because I'm not quite sure what should I correct in it.
The patch should remove all the changes made to the copy of the wizard in Compare that are not required by it to work properly. By changes I mean differences between the copy and the original. This includes whitespaces, comments, access modifiers and so on. If they can be keep intact comparing to the original and still work, this is what the patch should do. I doesn't matter if some changes are already in the branch, the patch is supposed to change it if needed. Keep in mind that we're talking about a branch not the HEAD, and there is couple of reasons why the code is still in a branch. This may one of them.
Created attachment 130675 [details] Patch_v2 I rechecked the patch, and removed all the unnecessary changes I could find. I applied a patch on local diff wizard from branch, and did "compare with" wizard from cvs to check changes. The only major changes that are left: - messages CompareMessages instead of CVSUIMessages - diferrent constructors and run() methods - no PatchWizardParticipant and changes area (cvs related components) in LocationPage from local diff wizard - no calculatePatchRoot() method in OptionsPage from local wizard - it checks if resources that are to be patched have the same parent, to calculate proper patch root - not related to local diff wizard - diffrent addPages() method - in local diff wizard we added new page, and added new features to OptionsPage. - different performFinish() - the patch is generated in different way for every wizard
(In reply to comment #7) > I applied a patch on local diff wizard from branch, and did "compare with" wizard > from cvs to check changes. Great, I would do it the same way. I did apply the patch you sent, but I got a compilation error saying "The method getPath() undefined...". Also make sure you adopt the newest constants from IWorkbenchCommandConstants. Anyway, it's still a no go. Why don't you give it another try?
Created attachment 131921 [details] Patch_v3 Sorry about that stupid mistake with undefined method. It's corrected now. We've also changed new constants from IWorkbenchCommandConstants in TextMergeViewer and MergeSourceViewer, as you requested Tomasz.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
