Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 77575

Summary: [typing] Undo is not grouped after Ctrl+X or Ctrl+Z in linked mode
Product: [Eclipse Project] Platform Reporter: Markus Keller <markus.kell.r>
Component: TextAssignee: Dani Megert <daniel_megert>
Status: VERIFIED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: benno.baumgartner, daniel_megert, eclipse, koen.van.dijken, martinae, Mike_Wilson, mlists
Version: 3.0Flags: markus.kell.r: review+
Target Milestone: 3.8 RC1   
Hardware: PC   
OS: Windows XP   
Whiteboard:
Bug Depends on:    
Bug Blocks: 185043    
Attachments:
Description Flags
Fix none

Description Markus Keller CLA 2004-11-03 05:19:54 EST 
I20041102-2000, found while verifying bug 74948.

String m() {
  String string= null;
  return string;
}

- put caret into first 'string' and invoke "local rename" quick assist
- select 'st' in linked field and press Ctrl+X
- press Ctrl+Z

=> was: selection on 'st' in second 'string' with text:
  String ring= null;
  return string;

=> expected: caret before first 'ring' with text:
  String ring= null;
  return ring;
Comment 1 Dani Megert CLA 2004-11-03 09:19:01 EST 
expected should be (talked to Markus):
  String string= null;
  return string;

See also bug: 31813.

Note: Works if Delete is hit instead of Ctrl+X
Comment 2 Dani Megert CLA 2005-06-16 06:23:15 EDT 
Resetting priority to P3. Will be reassessed for the next release.
Comment 3 Markus Keller CLA 2006-03-30 13:11:29 EST 
See also bug 31123 for wrong selection after undo.
Comment 4 Dani Megert CLA 2006-03-31 02:07:18 EST 
*** Bug 94272 has been marked as a duplicate of this bug. ***
Comment 5 Dani Megert CLA 2007-05-02 06:32:19 EDT 
*** Bug 185043 has been marked as a duplicate of this bug. ***
Comment 6 Dani Megert CLA 2007-05-02 12:43:02 EDT 
*** Bug 185125 has been marked as a duplicate of this bug. ***
Comment 7 Dani Megert CLA 2007-05-18 05:48:31 EDT 
*** Bug 187488 has been marked as a duplicate of this bug. ***
Comment 8 Mike Wilson CLA 2008-04-12 11:33:50 EDT 
Is this still on the radar for 3.4?
Comment 9 Dani Megert CLA 2008-04-14 06:19:17 EDT 
Sorry, no time for 3.4 unless we get a patch.
Comment 10 Dani Megert CLA 2012-05-09 08:19:50 EDT 
Created attachment 215323 [details]
Fix
Comment 11 Dani Megert CLA 2012-05-09 08:21:25 EDT 
In the current code change were only group as a result of key events being processed. The attached patch ensures that any modification to the document opens a compound change.

All Text tests pass.
Comment 12 Markus Keller CLA 2012-05-11 11:59:32 EDT 
The "if (!fHasOpenCompoundChange)" is unnecessary and should be removed.

The patch fixes the issue in comment 0, but the multi-undo scenarios from e.g. bug 94272 and bug 185125 are still not fixed.

+1 for adding the beginCompoundChange();, but only if this bug or the other bugs stay open.
Comment 13 Dani Megert CLA 2012-05-11 13:37:53 EDT 
(In reply to comment #12)
> The "if (!fHasOpenCompoundChange)" is unnecessary and should be removed.

Yes, but the code is more understandable. I removed it now, but renamed the method to beginCompoundChangeIfNeeded().


> The patch fixes the issue in comment 0, but the multi-undo scenarios from e.g.
> bug 94272 and bug 185125 are still not fixed.

Did you just pick them randomly, or do you know which ones are not fixed?

I'm inclined to consider the other cases as WONTFIX. They don't happen often and don't justify further investigation.


Committed, pushed and released the fix:
http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=ba8f3c1d6bfc90f4b454edbcb7e3d6579aa8dd83
Comment 14 Markus Keller CLA 2012-05-11 14:40:25 EDT 
> Yes, but the code is more understandable. I removed it now, but renamed the
> method to beginCompoundChangeIfNeeded().

Then you should also rename endCompoundChange().


> > bug 94272 and bug 185125 are still not fixed.
> 
> Did you just pick them randomly, or do you know which ones are not fixed?

No, I tested them, and these two had clear test cases that were easily reproducible (though not entirely deterministic). They're actually the same problem, just not a dup of this bug.
Comment 15 Dani Megert CLA 2012-05-14 02:30:22 EDT 
Reopened the other two bugs.
Comment 16 Dani Megert CLA 2012-05-14 02:34:16 EDT 
Verified in 3.8-I20120512-2100 and 4.2-I20120513-1300.

>Then you should also rename endCompoundChange().
Will be done in next build.