Community
Participate
Working Groups
The compare editor structure view shows added methods with a - sign and deleted methods with a + sign.
I could swear that we had this in a bug somewhere else. This must have happened with: 12242: 381265: compare editor shows changes on wrong side [I4cbaaaf6] https://git.eclipse.org/r/#/c/12242/
That's kind of funny. Let's look at that for 2.0.1.
This one is harder than it looks! I thought I found a simple change to make this work, but it doesn't seem to accomplish anything.
https://git.eclipse.org/r/#/c/14086/ <- This doesn't actually do anything except clean up some dead code. Sebastien, any ideas about what the issue here might be?
(In reply to comment #4) > https://git.eclipse.org/r/#/c/14086/ <- This doesn't actually do anything > except clean up some dead code. > > Sebastien, any ideas about what the issue here might be? Hmm, no I do not remember this one, sorry. /Sebas
Tomek, do you know what would cause this?
After having a glimpse at the code, my _guess_ would be that StructureDiffViewer [1] swaps sides of the comparison if you have the old version (Base) of the file in your workspace. If you're way behind, i.e. have neither old nor new version checked out, the diff should be displayed correctly. [1] https://git.eclipse.org/c/platform/eclipse.platform.team.git/tree/bundles/org.eclipse.compare/compare/org/eclipse/compare/structuremergeviewer/StructureDiffViewer.java#n477
Created attachment 232975 [details] mylyn/context/zip
(In reply to comment #7) > After having a glimpse at the code, my _guess_ would be that StructureDiffViewer > [1] swaps sides of the comparison if you have the old version (Base) of the file > in your workspace. If you're way behind, i.e. have neither old nor new version > checked out, the diff should be displayed correctly. Thanks for the hint. After playing around with this a bit, the code seems a bit crazy. ;) While DiffTreeViewer respects the "LEFT_IS_LOCAL" property [1], it actually seems to be ignored in the configuration itself. In CompareConfiguration [2] is a static value for fLeftIsLocal. That is used to swap the sides for the comparison kind in a static method. So there is no way to change the value for the swap! Further, if I change the value from default of true, the Java Structure Compare (right) is correct, but the Structure Side (left) is now wrong! [1] https://git.eclipse.org/c/platform/eclipse.platform.team.git/tree/bundles/org.eclipse.compare/compare/org/eclipse/compare/structuremergeviewer/DiffTreeViewer.java#n186 [2] https://git.eclipse.org/c/platform/eclipse.platform.team.git/tree/bundles/org.eclipse.compare/compare/org/eclipse/compare/CompareConfiguration.java#n75
Created attachment 233002 [details] mylyn/context/zip
https://git.eclipse.org/r/#/c/14086/4 Fixes with a relatively straightforward hack. This seems to be the simplest way to retrieve the desired behavior w/o getting too deeply into compare internals. Tomek, please Review. https://git.eclipse.org/r/#/c/14086/4/org.eclipse.mylyn.reviews.ui/src/org/eclipse/mylyn/internal/reviews/ui/compare/ReviewItemSetCompareEditorInput.java are the relevant changes.
If you think it's a bug in Platform/Compare I would recommend at least filing a bug against them. I'm aware that even having them* fix this, will mean we have to wait for Luna to consume it, but at the same time it's going to allow us to understand what's going on there, and potentially provide a proper workaround (instead of a hack). * that includes me as I'm a committer on that project
(In reply to comment #12) > If you think it's a bug in Platform/Compare I would recommend at least filing a > bug against them. I'm aware that even having them* fix this, will mean we have > to wait for Luna to consume it, but at the same time it's going to allow us to > understand what's going on there, and potentially provide a proper workaround > (instead of a hack). > > * that includes me as I'm a committer on that project I'm not sure it's a bug so much as kind of sub-optimal implementation. Anyway, I managed to get it working through this hack. We just need to ensure that there aren't any cases where the original implementation actualy worked. I haven't seen any. Has anyone else?
I haven't either. But it does seem like a bug in Platform/Compare that the structure compare is not consistent with the status line. What will happen if we apply this change and then that is fixed?
(In reply to comment #14) > What will happen if we apply this change and then that is fixed? It'll mean that we're just as broken as every other Compare Editor consumer then will be. ;D Seriously, I have no idea what form a useful fix might take and considering the number of consumers out there, I imaginve platform would be esitant to fix it at this point if the broken behaviour is going to change. Perhaps we can leave that to Tomek as to whether to pursue that one further.
I am OK with applying this fix.
(In reply to comment #16) > I am OK with applying this fix. Just for the sake of process then, could you +1 on review please?
Done.
Thanks Sam. That's my anal process guy comment of the month. Got to turn it over every once in a while or it gets rusty. Merged https://git.eclipse.org/r/#/c/14086/
[I lied: now I'm finding myself commenting on Commit messages which I swore I'd never do. ;D https://git.eclipse.org/r/#/c/14232/5//COMMIT_MSG]
This fixed the problem for the compare with action, but it did not fix it when an individual compare editor is opened from the review.
(In reply to comment #21) > This fixed the problem for the compare with action, but it did not fix it when > an individual compare editor is opened from the review. That's interesting / annoying. I'll take a look.
Please review: https://git.eclipse.org/r/14664 This was an oversight -- I made change to ReviewItemSetCompareEditorInput but not the File version.
(re) fixed with https://git.eclipse.org/r/14664
