Bug 175811 - [Apply Patch] 'Ignore WhiteSpace' in Apply Patch wizard does not work
Summary: [Apply Patch] 'Ignore WhiteSpace' in Apply Patch wizard does not work
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Compare (show other bugs)
Version: 3.3   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.3 M7   Edit
Assignee: Krzysztof Michalski CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed
Depends on:
Blocks:
 
Reported: 2007-02-28 06:00 EST by Markus Keller CLA
Modified: 2007-06-22 13:13 EDT (History)
0 users

See Also:

Attachments
Proposition of changes (2.11 KB, patch)
2007-03-13 11:02 EDT, Krzysztof Michalski CLA 		no flags Details | Diff
Proposition of changes (3.84 KB, patch)
2007-03-19 09:56 EDT, Krzysztof Michalski CLA 		no flags Details | Diff
Patch is response for Michael suggestions (4.80 KB, patch)
2007-04-24 10:42 EDT, Krzysztof Michalski CLA 		no flags Details | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Keller CLA 2007-02-28 06:00:52 EST 
I20070227-0800

- have org.eclipse.jdt.ui v20070227-0800
- Apply Patch...
- choose attachment 58261 [details] from bug 172883
- enable 'Reverse patch'
- SuperInterfaceSelectionDialog.java has a lot of whitespace changes
- enable 'Ignore WhiteSpace' (=> label misses space between 'White' and 'Space')

=> The 'Patch Contents' tree collapses and the merge viewer is closed. Expected: Tree should refresh, and previous expansion & selection should be restored if possible.

- open SuperInterfaceSelectionDialog.java
=> Whitespace not filtered.
Comment 1 Krzysztof Michalski CLA 2007-03-13 11:02:18 EDT 
Created attachment 60670 [details]
Proposition of changes
Comment 2 Michael Valenta CLA 2007-03-14 13:44:48 EDT 
Unless I am missing something, I don't think we need to have a property listener on the action since a change in the checked state should only occur through the run method. Therefore, I would expect the code you added to appear in the "if (getPatcher().setIgnoreWhitespace(isChecked())" if statement. You should just be able to set the configuration property before calling rebuildTree().
Comment 3 Krzysztof Michalski CLA 2007-03-19 09:56:24 EDT 
Created attachment 61281 [details]
Proposition of changes
Comment 4 Michael Valenta CLA 2007-03-19 10:54:35 EDT 
That looks better. The one comment I have is that it would appear that the Ignore Whitespace action results in a double invocation of syncExec (once for the setting of the property and once for the rebuilding of the tree. One thing we could do to fix this would be to only set the property in the action and register a property change listener on the configuration that detects and Ignore Whitespace change on the configuration and rebuilds the tree in response. Since the event would be fired from the UI thread, we would only end up with a single syncExec.
Comment 5 Krzysztof Michalski CLA 2007-04-24 10:42:39 EDT 
Created attachment 64744 [details]
Patch is response for Michael suggestions
Comment 6 Michael Valenta CLA 2007-04-24 11:40:11 EDT 
I've released the portion of the patch related to Ignore Whitespace. There were some changes in the Reverse functionality which I did not include. Were these related to another bug?
Comment 7 Krzysztof Michalski CLA 2007-04-24 12:22:06 EDT 
Changes in reverese functionality are part of this bug. I made it beacause rebuildTree() not refresh merge viewer. If we have selected file in patch content and next to we mark or unmark "reverese patch" merge viewer and java structure are still the same (not refreshed) Thats why i made it deselect on patch content.  We can make quck fix as in patch or open new bug to reverese functionality.
Comment 8 Michael Valenta CLA 2007-04-24 12:27:45 EDT 
I would prefer that you open a separate bug.