Comment 1 Robert Munteanu 2011-10-12 07:42:49 EDT

Created attachment 205018 [details]
Compare editor with incorrect java structure compare

On the left side the local file ( revision 3 ) has a skeleton content, and on the right side the file has no content.

Nevertheless, Java Structure Compare shows all structural elements as removed, not as added.

Comment 2 Robert Munteanu 2011-10-12 07:45:42 EDT

The relevant code snippet in my CompareEditorInput implementation is


pre.. 
    private Object findDifferences(IProgressMonitor monitor, IFilePatchResult patchResult) throws IOException {
        
        byte[] baseContent = IOUtils.toByteArray(patchResult.getOriginalContents());
        byte[] targetContent = IOUtils.toByteArray(patchResult.getPatchedContents());
        
        ByteArrayInput baseInput = new ByteArrayInput(baseContent, getFile().getBase().getPath());
        ByteArrayInput targetInput = new ByteArrayInput(targetContent, getFile().getTarget().getPath());
        
        Object differences = new Differencer().findDifferences(false, monitor, null, null, baseInput, targetInput);
        
        monitor.worked(1);
        
        return differences;
    }

Comment 3 Steffen Pingel 2011-10-12 10:06:56 EDT

Thanks for the bug report. The same problem exists with the Gerrit connector.

Comment 4 Steffen Pingel 2011-10-12 17:14:40 EDT

I swapped the inputs and fixed a number of bugs in ReviewAnnotationModel and ReviewCompareAnnotationModel. The compare editor should now follow the expected Eclipse behavior even though this is reverse from the layout in the Gerrit web UI.

Comment 5 Robert Munteanu 2011-10-14 07:45:38 EDT

This has caused a regression in how comment annotations are added. Even though I add comment annotations to the target, they are displayed on the base. Reverting the commit fixes the problem.

Comment 6 Steffen Pingel 2011-10-14 08:11:59 EDT

I have tested this with Gerrit and comments were added as expected. Can you check the implementation of your behavior?

Comment 7 Robert Munteanu 2011-10-14 08:44:06 EDT

Created attachment 205191 [details]
Annotations reversed after patch

I'm unable to find the flaw ; I can easily reverse my logic, but I just wanted to verify that I'm not missing something obvious.

The code in question is

pre..
        IFileRevision base = getFile().getBase();
        IFileRevision target = getFile().getTarget();
        
        ByteArrayInput baseInput = new ByteArrayInput(baseContent, base.getPath());
        ByteArrayInput targetInput = new ByteArrayInput(targetContent, target.getPath());
        
        System.out.println("Topics for BASE - " + base.getRevision() + " : " + base.getTopics().size());
        System.out.println("Topics for TARGET - " + target.getRevision() +" : " + target.getTopics().size());
        
        Object differences = new Differencer().findDifferences(false, monitor, null, null, baseInput, targetInput);
        
p. 
The result is

pre..

Topics for BASE - 3 : 0
Topics for TARGET - (working copy) : 3

p.
The title of the compare editor shows that the base is displayed on the left and the target on the right, but the annotationsa are switched.

Do you see any problem with this implementation?

Comment 8 Steffen Pingel 2011-10-14 09:24:29 EDT

Based on your input I reverted the implementation in the framework. ReviewCompareEditorInput now computes the differences like this to find the right additions and removals:

		Differencer differencer = new Differencer();
		Object diff = differencer.findDifferences(false, monitor, null, null, new ByteArrayInput(targetContent,
				targetPath), new ByteArrayInput(baseContent, basePath));

Comment 9 Robert Munteanu 2011-10-14 09:46:33 EDT

Did you push the change to git.eclipse.org? There seem to be no changes in the reviews repository.

Comment 10 Steffen Pingel 2011-10-14 09:47:41 EDT

(In reply to comment #8)
> Based on your input I reverted the implementation in the framework.
> ReviewCompareEditorInput now computes the differences like this to find the
> right additions and removals:

Just to clarify, this was part of the change that I originally committed for this bug. I haven't made any further changes so far.

After reviewing the behavior of CVS and Git my understanding is that Eclipse displays the newer content on the left be default hence it seems right to display the target content on the left. Does that make sense to you?

Comment 11 Robert Munteanu 2011-10-14 09:53:43 EDT

Yes, it makes sense. I have changed my implementation to act the same. Feel free to close this again.

Comment 12 Steffen Pingel 2011-10-14 10:09:38 EDT

Thanks! Please feel free to reopen if we should tweak this further.