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

Bug 94419

Summary: [misc] menu actions disabled after rename and then dirtying editor
Product: [Eclipse Project] Platform Reporter: Tom Hofmann <eclipse>
Component: TextAssignee: Dani Megert <daniel_megert>
Status: VERIFIED FIXED QA Contact:
Severity: normal    
Priority: P2 CC: daniel_megert, david_williams, dirk_baeumer
Version: 3.1   
Target Milestone: 3.1 RC1   
Hardware: PC   
OS: Linux-GTK   
Whiteboard:
Attachments:
Description Flags
AbstractTextEditor patch none

Description Tom Hofmann CLA 2005-05-10 12:45:02 EDT
I20050509-2000 (M7 test pass)

scenario from bug 77302:

- Java browsing perspective
- create an empty class  A.java
- from the types view, refactor>rename to B.java
- go into B.java and make it dirty

> expected: Edit>Undo is enabled
< actual: Edit>Undo is disabled.

Note: the editor's context menu is correctly enabled, but not the global menu
entry. Ctrl+Z/Ctrl+Shift+Z work as expected.
Comment 1 Susan McCourt CLA 2005-05-11 16:49:15 EDT
Cannot reproduce this one.
I see Undo Typing in both the Edit menu and the popup.
Is it possible you changed the active part somewhere along the way?  Some 
parts don't show undo/redo (they are disabled) so that could explain the 
behavior.
Comment 2 Tom Hofmann CLA 2005-05-12 11:07:58 EDT
I believe this is the same as bug 93955
Comment 3 Susan McCourt CLA 2005-05-12 11:43:41 EDT
investigate RC1 - 
I don't think this related to bug #93955 since the problem in this bug is a 
mismatched pulldown/popup menu.  
Comment 4 Susan McCourt CLA 2005-05-16 12:44:37 EDT
Reproduced on M7 final build.

Note that once you are in this state, undo remains disabled in B.java until 
you activate a different editor.  Then switch back to the original editor and 
the undo pulldown is updated.  The behavior is very similar to that of bug 
#92070.
Comment 5 Dani Megert CLA 2005-05-17 06:35:59 EDT
Tom, can you reproduce this by simply "dirtying" the editor as written in the
summary or is it actually the rename that triggers the bug?
Comment 6 Tom Hofmann CLA 2005-05-17 07:13:54 EDT
(In reply to comment #5)
> Tom, can you reproduce this by simply "dirtying" the editor as written in the
> summary or is it actually the rename that triggers the bug?

The rename is required. After simply creating a new class and dirtying it, the
menu is correctly enabled.

Just reproduced on N20050516 with the steps from comment 0.

Note: the menu is permanently wrong for this editor after this happens. Changing
focus and coming back, switching editors and back etc. all don't enable the menu.
Comment 7 Tom Hofmann CLA 2005-05-17 07:14:23 EDT
oops, not true: switching editors does enable it, but not switching view.
Comment 8 Dani Megert CLA 2005-05-17 07:28:50 EDT
adapting summary
Comment 9 Susan McCourt CLA 2005-05-18 00:59:17 EDT
I think I've tracked this down.

As part of fixing bug #89393, the undo/redo action handlers are recreated 
every time the editor input changes.  (See bug #89393 comment #8).  This has 
always bothered me a little, because it leaves multiple actions listening to 
part events and operation history events and updating the action text, but 
since it did not cause any leaks I never really looked into what happens when 
multiple actions are created by a single part and registered for the same 
action definition.

In examining the RetargetAction code, it would seem possible that any of the 
created action handlers could update the action text and enablement for the 
site's action bars.  This means that depending on the order of operation 
history event listeners (and also the property change listeners, etc.), any of 
the previously created actions could update the text and enablement last, 
causing the wrong enablement and/or text (since the old actions are associated 
with an old undo context.)  

I added code in AbstractTextEditor to dispose of any old undo/redo actions 
created before creating new ones, so that the "dead" handlers no longer update 
text and enablement as part of listening to operation history events.  This 
solves the problem.  

Attaching a patch to AbstractTextEditor and moving to platform text.
Comment 10 Susan McCourt CLA 2005-05-18 01:00:27 EDT
Created attachment 21310 [details]
AbstractTextEditor patch
Comment 11 Dani Megert CLA 2005-05-25 18:52:55 EDT
The patch does something good, it disposes the old undo/redo actions but it does
not fix the problem. The cause for this bug was that while we re-create the
undo/redo action handlers we forgot to re-register them with the global action
handler. I've fixed this up.

The fix is ready but I will commit only after having verified all the previously
broken scenarios (thanks Susan sending me the list). This will be done tomorrow.

I also plan to investigate whether there's a way around re-creating the undo
manager upon editor input change.
Comment 12 Dani Megert CLA 2005-05-26 05:48:14 EDT
Verified and committed to HEAD.
For the undo manager reuse investigation item see bug 96756.
Comment 13 Dani Megert CLA 2005-05-30 05:52:33 EDT
start verifying
Comment 14 Dani Megert CLA 2005-05-30 05:55:35 EDT
Verified in 3.1 RC1.