Bug 203430 - [compare] Ignore java formatting changes when comparing two java files
Summary: [compare] Ignore java formatting changes when comparing two java files
Status: ASSIGNED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.4   Edit
Hardware: All All
: P3 enhancement with 6 votes (vote)
Target Milestone: ---   Edit
Assignee: JDT-UI-Inbox CLA
QA Contact:
URL: http://rubenlaguna.com/wp/2007/08/18/...
Whiteboard:
Keywords:
: 74751 (view as bug list)
Depends on: 74636
Blocks:
  Show dependency tree
 
Reported: 2007-09-14 07:56 EDT by Ruben Laguna CLA
Modified: 2013-08-23 07:53 EDT (History)
9 users (show)

See Also:


Attachments
source code for java formatting compare plugin (29.32 KB, application/octet-stream)
2007-09-14 07:56 EDT, Ruben Laguna CLA
no flags Details
screenshot: before ignore formatting changes (40.67 KB, image/png)
2007-09-14 07:58 EDT, Ruben Laguna CLA
no flags Details
screenshot: after ignore formatting changes (32.79 KB, image/png)
2007-09-14 07:59 EDT, Ruben Laguna CLA
no flags Details
Patch to add ignore java formatting changes to the JavaMergeViewer. (8.75 KB, patch)
2007-09-21 04:29 EDT, Ruben Laguna CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ruben Laguna CLA 2007-09-14 07:56:17 EDT
Created attachment 78416 [details]
source code for java formatting compare plugin

Currently the JavaMergeViewer can ignore whitespace changes but this can be enhanced to ignore all java formatting changes. Sometimes in source code repositories you can find code that has been formatted with different formatter and comparing between revisions in this scenario is quite hard. I´ve developed a plugin that extends javamergeviewer to add this functionality. Unfortunately due to Bug #201116 this plugin it´s not functional in some configurations. So I think this plugin functionality can be added to the current JavaMergeViewer. 

Please find the attached source code for the plugin and some screenshots
Comment 1 Ruben Laguna CLA 2007-09-14 07:58:28 EDT
Created attachment 78417 [details]
screenshot: before ignore formatting changes
Comment 2 Ruben Laguna CLA 2007-09-14 07:59:16 EDT
Created attachment 78418 [details]
screenshot: after ignore formatting changes
Comment 3 Martin Aeschlimann CLA 2007-09-17 05:34:07 EDT
Would it be possible to have a toolbar item to toggle this functionality?
Comment 4 Martin Aeschlimann CLA 2007-09-17 06:13:24 EDT
After some thinking I think we should use the already existing 'Ignore whitespace' button on the toolbar to do this. Formatting is nothing else than adding whitespace (spaces, tabs, new lines). At the moment it seems that new lines are not counted as white spaces.

Would you be interested to work on a patch that makes the 'Ignore whitespace' enabling/disabling your functionality? My feeling is that it shouldn't be to hard to fix the current implementation. So a minimal changes patch would be most welcome!
Comment 5 Ruben Laguna CLA 2007-09-17 14:40:04 EDT
(In reply to comment #3)
> Would it be possible to have a toolbar item to toggle this functionality?
> 

The toolbar button it´s already implemented. If you look at the screenshot you´ll notice the little window/red cross at  the top right corner. 
Comment 6 Ruben Laguna CLA 2007-09-17 14:42:06 EDT
(In reply to comment #4)
> After some thinking I think we should use the already existing 'Ignore
> whitespace' button on the toolbar to do this. Formatting is nothing else than
> adding whitespace (spaces, tabs, new lines). At the moment it seems that new
> lines are not counted as white spaces.
> 
> Would you be interested to work on a patch that makes the 'Ignore whitespace'
> enabling/disabling your functionality? My feeling is that it shouldn't be to
> hard to fix the current implementation. So a minimal changes patch would be
> most welcome!
> 

Yeah, I´m interested. I´ll take a look into providing a patch. Stay tuned. 
Comment 7 Ruben Laguna CLA 2007-09-21 04:29:54 EDT
Created attachment 78937 [details]
Patch to add ignore java formatting changes to the JavaMergeViewer. 

This patch applied to the current R3_3_maintenance adds a new functionality to ignore all java formatting changes not only whitespace changes on the same line. This functionality is enabled/disabled through "Ignore whitespace changes" in the Compare Preference Page.
Comment 8 Martin Aeschlimann CLA 2007-09-21 09:16:00 EDT
Thanks Ruben for the patch!

I think the feature makes sense, but I'm not sure if the patch takes the right approach.
What I don't like is that the preview now shows a formated version. It also isn't editable anymore. It's not clear to me what happens when you modify the source in the compare editor (merge or manual edit) and then save.

It would be more consistent with the 'Show Whitespace' feature when the source stays in it's original format, but whitespaces and line delimiters changes are ignored (if  'ignore whitespaces' is enabled).

Note that you can toggle 'Ignore Whitespaces' while the compare editor is open ('Ignore Whitespaces' is on the toolbar).

Looking the code, I think the reason why 'Ignore Whitespace' ends at line delimiters is the usage of the DocLineComparator in the DocumentMerger.

The TextMergeViewer compares documents line by line. Therefore there's always a change when you have different line breaks. 

Adding Michael as the compare expert.

Comment 9 Martin Aeschlimann CLA 2007-09-21 09:16:40 EDT
Michael, do you have an opinion here?
Comment 10 Michael Valenta CLA 2007-09-24 10:03:55 EDT
I think I agree with the assessment in comment 8. The current architecture of the TextMergeViewer restricts any model based involvement (e.g. Java) to involvement at the line level. I think that the proper solution to this problem would be to allow models to participate in the determination of differences at the document level. Last week (but not in 3.4 M2) I split out the document differencing code from TextMergeViewer. I think this is the first step. The final solution would require API that allowed models to override or extend the document differencing algorithm.
Comment 11 Martin Aeschlimann CLA 2007-10-23 09:17:44 EDT
*** Bug 74751 has been marked as a duplicate of this bug. ***