Bugzilla – Full Text Bug Listing
| Summary: | [ModelTooling] Undo/Redo not working in 4.0 | ||
|---|---|---|---|
| Product: | [Eclipse Project] e4 | Reporter: | Thomas Schindl <tom.schindl> |
| Component: | UI | Assignee: | Project Inbox <e4.ui-inbox> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | bokowski, john.arthorne, pwebster, remy.suen, susan |
| Version: | 1.0 | ||
| Target Milestone: | 1.0 RC3 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| Attachments: | |||
|
Description
Thomas Schindl
I'll look at this in more detail today. I double-checked that both text undo and workspace undo (delete a file and then undo the delete) are working in 4.0. So we know the general platform undo mechanism is working, it's a question of what the model editor is doing that is different. Random theories (before looking closer) - the action update timing used by the platform undo handlers (OperationHistoryActionHandler and subclassing) is somehow different than Tom's undo actions - have to figure out how the EMF command stack fits in here. Tom's actions are using the command stack to determine whether the action is enabled. I don't know the details of EMF's implementation but will look into it... Thanks Susan, the command stack stuff is fairly simply every change of the EMF-Resource is recorded as a command you can remove the code and simply always pass true to setEnabled(). Ping me on IRC when you need help from me. I'm available all day long. It's not only me who doesn't have undo/redo support. As I expected other editors like e.g. the Ecore-Editor has no undo/redo (the reason is that my code is derived from how the EMF-Editors implement Undo/Redo). From a usability PoV this is a major problem IMHO so we should try to find out what's going wrong here. So I've now tracked this down to MenuServiceFilter#setEnabled(MHandledMenuItem) which in case for e.g. the Box View-Example returns true but for the ModelEditor false. Ok. Next interesting thing is that HandlerServiceImpl#lookUpHandler the "Box View" has handler::...undo in the localValues but my view has not! Ok. I think I now found the problem. The reason the undo does NOT work is found EditorActionBars in org.eclipse.ui.SubActionBars#setGlobalActionHandler(). When retargeting an Action and the target has NO actionDefinitionId it fails to register the appropriate handler. Has there been a rule that one has to set the actionDefinitionId? The BoxView example does it and that's why it is working! Investigating deeper reveals that the real problem is that RetargetActions are not registered in our ActionBarAdvisor#register(). Commenting out the instance check there makes the undo/redo behave as expected. The only thing not working is the updateing of the Label-Text but that's a minor issue. The change was introduced by Remy with Bug 308593 Some more information. The change was introduced with bug 308593 Comment 19 Created attachment 175152 [details]
SubActionBar-hack to get at least undo/redo
This is a dirty but very simply and not risky workaround so that editors without a setActionDefinitionId() undo/redo support
Created attachment 175153 [details]
ModelEditor only fix
If we are not fixing the generic case this is the fix for the ModelEditor
Not sure which one I like better. Remy, do you have an opinion? Just a comment. RetargetActions can't be part of the handler system, they'll break it. But if the root cause is that there is no action command mapping for "undo" to the UNDO command id, the another possible fix would be to simply add the mapping when we create the action mapping service. That would allow subActionBars.setGlobalActionHandler(*) to work without each having to set their action definition ID. PW Created attachment 175154 [details]
3rd idea make retarget-action available in IActionCommandMappingService
This is the 3rd solution and probably better than SubActionBar hack because it makes all RetargetAction work
Created attachment 175155 [details]
3rd the second
Created attachment 175158 [details] ActionCommandMappingService patch v1 Spot fix to implement Paul's suggested comment 12. (In reply to comment #14) > Created an attachment (id=175155) [details] > 3rd the second The place to do the command mapping and return is where we currently avoid the RetargetAction (if we're not going to go with the spot fix). We want to avoid allowing Menu Bar retarget actions further into the system. PW I've now released at least the local fix for ModelEditor so that it has undo/redo because I need to go to bed for now. ... i still hope we could at least fix undo/redo in a generic fashion because i think it is really important - even if we only fix this undo/redo case with remys patch I'm +1 for it. Created attachment 175168 [details]
make sure RetargetActions generate action mappings v05
(In reply to comment #19) > Created an attachment (id=175168) [details] > make sure RetargetActions generate action mappings v05 I'm fine with this, +1. I also tested editors in two workbench windows to ensure there wasn't any funny business with the retrieval of the service at the window level. +1 from me (In reply to comment #19) > Created an attachment (id=175168) [details] > make sure RetargetActions generate action mappings v05 I've released it for the I20100724-2045 build. Tom, thanks for your investigation!! PW I can confirm that the latest build fixes the Undo/Redo Verified with the ecore editor using I20100726-2152 on Windows XP. |