Bugzilla – Full Text Bug Listing
| Summary: | [misc] Support marker undo | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Susan McCourt <susan> | ||||||||
| Component: | Text | Assignee: | Platform-Text-Inbox <platform-text-inbox> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||
| Severity: | enhancement | ||||||||||
| Priority: | P3 | ||||||||||
| Version: | 3.2 | ||||||||||
| Target Milestone: | 3.3 M2 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows XP | ||||||||||
| Whiteboard: | |||||||||||
| Bug Depends on: | 156865 | ||||||||||
| Bug Blocks: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Susan McCourt
Created attachment 48425 [details]
org.eclipse.ui.editors.patch
This patch changes the AddMarkerAction and MarkerRulerAction to use the new API for undoable marker operations. Note that this API is currently experimental and may evolve in 3.3. However, it would be nice to get these changes in so that we can start promoting the use of marker operation undo.
Note: A separate issue not addressed in these patches is whether the undo history for an editor should show the marker operations performed in the editor's ruler. ie, if you delete a task or bookmark from the ruler, should you see "Undo Delete Task" in the undo menu for the editor? Currently you will only see it in the Package Explorer and Task List. Note that showing these operations in the undo history for the editor would take additional code. Currently the document undo manager responds only to changes in the document. To accomplish this, we would have to add code in the text editor classes that watch for operation history changes to the workspace and add the editor's undo context to those that affect the editor's underlying input. I can provide patches that do this, but did not pursue until I know whether this is a desirable feature from a platform text point of view. See bug #7691 (particularly comments 13 & 14) for explanation of the new feature. I assume that this patch has similar problems as bug 156864. Susan, does this affect this patch? I prefer to put this on hold until bug 156865 is resolved. yes, it's best to wait for bug #156865 to get fixed. I should have an interim solution for M2. thanks! Susan, thanks for the patch. I see several problems with it: 0. it does not work - at least not for me: I installed your patch and ran against I20060912-0800). Each time I use Edit > Add Bookmark... or Edit > Add Task... while the editor has focus I get an IllegalAccessError (see full stack trace at the bottom). Did you test the code at all? ;-) 1. the label you use in AddMarkerAction will look bad even after removing the accelerator: it then says e.g. "Undo Bookmark" or "Redo Bookmark" instead of "Undo Add Bookmark" or "Redo Add Task". Maybe we could use the tool tip instead. 3. potential NPE (if a client calls setText(null) on the action: Action.getText() can return 'null' but the CreateMarkersOperation (or AbstractWorkspaceOperation to be more precise) will fail on that. 4. why do you log the same error twice in AddMarkerAction? Is that really needed? I'd prefer to log it just once. 5. why did you remove the code that informs the user? Does executing the operation now do this? --- full stack trace when executing Edit Add Task/Bookmark --- !ENTRY org.eclipse.ui 4 4 2006-09-13 11:38:46.305 !MESSAGE Unhandled event loop exception !ENTRY org.eclipse.ui 4 0 2006-09-13 11:38:46.315 !MESSAGE tried to access method org.eclipse.ui.texteditor.TextEditorAction.getTextEditor()Lorg/eclipse/ui/texteditor/ITextEditor; from class org.eclipse.ui.texteditor.AddMarkerAction$1 !STACK 0 java.lang.IllegalAccessError: tried to access method org.eclipse.ui.texteditor.TextEditorAction.getTextEditor()Lorg/eclipse/ui/texteditor/ITextEditor; from class org.eclipse.ui.texteditor.AddMarkerAction$1 at org.eclipse.ui.texteditor.AddMarkerAction$1.getAdapter(AddMarkerAction.java:136) at org.eclipse.ui.internal.operations.AdvancedValidationUserApprover.getShell(AdvancedValidationUserApprover.java:390) at org.eclipse.ui.internal.operations.AdvancedValidationUserApprover.computeOperationStatus(AdvancedValidationUserApprover.java:231) at org.eclipse.ui.internal.operations.AdvancedValidationUserApprover.access$1(AdvancedValidationUserApprover.java:225) at org.eclipse.ui.internal.operations.AdvancedValidationUserApprover$1.run(AdvancedValidationUserApprover.java:201) at org.eclipse.swt.widgets.Synchronizer.syncExec(Synchronizer.java:152) at org.eclipse.ui.internal.UISynchronizer.syncExec(UISynchronizer.java:28) at org.eclipse.swt.widgets.Display.syncExec(Display.java:3805) at org.eclipse.ui.internal.operations.AdvancedValidationUserApprover.proceedWithOperation(AdvancedValidationUserApprover.java:197) at org.eclipse.ui.internal.operations.AdvancedValidationUserApprover.proceedExecuting(AdvancedValidationUserApprover.java:167) at org.eclipse.core.commands.operations.DefaultOperationHistory.getExecuteApproval(DefaultOperationHistory.java:840) at org.eclipse.core.commands.operations.DefaultOperationHistory.execute(DefaultOperationHistory.java:477) at org.eclipse.ui.texteditor.AddMarkerAction.run(AddMarkerAction.java:133) at org.eclipse.jface.action.Action.runWithEvent(Action.java:499) at org.eclipse.ui.actions.RetargetAction.runWithEvent(RetargetAction.java:229) at org.eclipse.jface.action.ActionContributionItem.handleWidgetSelection(ActionContributionItem.java:539) at org.eclipse.jface.action.ActionContributionItem.access$2(ActionContributionItem.java:488) at org.eclipse.jface.action.ActionContributionItem$5.handleEvent(ActionContributionItem.java:400) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:66) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:928) at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:3390) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3009) at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:1914) at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:1878) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:419) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149) at org.eclipse.ui.internal.ide.IDEApplication.run(IDEApplication.java:95) at org.eclipse.core.internal.runtime.PlatformActivator$1.run(PlatformActivator.java:78) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:104) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:74) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:348) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:165) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:585) at org.eclipse.core.launcher.Main.invokeFramework(Main.java:341) at org.eclipse.core.launcher.Main.basicRun(Main.java:285) at org.eclipse.core.launcher.Main.run(Main.java:987) at org.eclipse.core.launcher.Main.main(Main.java:962) Note that testing "getTextEditor() != null" is not needed since getTextEditor() can't be 'null' at that point. Hi, Dani. Oops.....There have been substantial changes in the advanced validation user approver since these patches were first made, and I did not make another pass over this code. I will attach new patches.
At the time of the patches, everything worked, no exceptions or log entries, the labels showed "Undo Add Bookmark", etc...I need to see what else may have affected this in the meantime.
The NPE risk (#3) will be fixed, I did not consider that.
#4/#5 - The strategy for workspace operations is that the operation itself now handles the workspace runnable, error handling, etc. So this code is removed from the actions. Logging is up to each action. While converting all the actions, my decision process was that if the action had defined context-specific error messages, I continued to log those. The operation error messages are somewhat generic ("Could not add bookmark") whereas some of the actions might have a wording that is more specific. I did not want to lose information, so I've left it up to the client whether to log the original error message (and you can remove the logging if you see no value).
New patches shortly...
Hmmm...I'm not sure what's going on here. My local files for the action changes haven't changed since posting the patches, and everything is working properly. I tried these scenarios: - add bookmark, remove bookmark, add task, remove task from text editor ruler while in resource perspective. All actions have proper labels and are undoable/redoable from both the navigator and the task or bookmarks view. - add bookmark, remove bookmark, add task, remove task from java editor ruler while in resource perspective. Everything fine. - same scenarios from text and java editors while in java perspective, undoing and redoing from the package explorer and appropriate marker view. The log is clean, the undo/redo works as expected, the labels are good. I am running I20060906-1200, but I have the latest org.eclipse.ide, org.eclipse.ui.workbench, org.eclipse.ui.workbench.texteditor, org.eclipse.ui.editors, org.eclipse.jface.text, (among others) etc. from HEAD. I noticed a change made just after I20060912 to AdvancedValidationUserApprover, but using either revision of the class works properly. So I can't account for what you are seeing. I am still going to repost the patches to address point #3 in comment #7. It avoids the NPE, but to be honest, I don't like what I did (a hardwired string). If the client sets the action text to null, there will be problems elsewhere. And if the resource bundle/label key are missing, then I don't like defaulting the string to some other string from the same bundle, hence the hardwired string. Unless you are proposing that I change the operations to handle a null string? Finally, I left the getTextEditor() null check (comment #8) in on the same premise (what if the client uses setEditor(ITextEditor) to set it to null before calling run()). I don't see that the check is harmful. Meanwhile, I am downloading I20060912, but I still recommend you run the latest from HEAD. Created attachment 50080 [details]
org.eclipse.ui.editors.patch
patch that addresses null action text. Please note the TODO: tag, as I'm not sure how you would expect to handle this.
Dani, I tried the scenarios on I20050912-0800 with only the editor patches. The scenarios have problems, but not those described, and of course the fix for #156865 was not in the integration build. So I'm assuming you must have loaded some projects from HEAD in order to get the required fix. It's possible that you loaded the latest org.eclipse.ui.ide without the corresponding org.eclipse.ui.workbench?? This could have caused some mismatch. At any rate, if you try the patches along with the latest workbench and ide from HEAD, all is well. Let me know if you still have problems. Susan, I don't wanna be picky here but please note that - as written in comment 7 - your patch affects the behavior for [MAIN MENU] > Edit > Add [Task | Bookmak]... That's where exceptions will be thrown and where the labels are messed up. You are right that once I had all the latest from HEAD the exception does not occur BUT only because your code no longer requests the adapter now. As soon as any operations code will do, it will fail. Simply look at the code in AddMarkerAction.run(): it accesses a protected method getTextEditor() outside its class hierarchy and hence this code is doomed to fail should it ever be executed. The question is: will it be executed ever? If so, it has to change. If not, why do we need to provide the adapter at all. Regarding the label: I did not change them and hence they could never have been correct for the [MAIN MENU] > Edit > Add Bookmaker... scenario ;-) Dani, you are not being picky, just specific. I realize now that we were on different scenarios. I assumed that you were referring to the scenario described in this bug title (undo support to the actions that add bookmarks and tasks from the editor ruler.) When I saw your annotation in this bug, I jumped straight into the editor ruler scenario, without realizing that you had clearly described the problem as still being in Edit...Add Bookmark, etc. And now I realize neither are you describing the Edit..Add Bookmark scenario from bug #156865, but instead the scenario where the editor is still open and the Edit... menu is used. Now, not to be picky ;-) but the issues 0 and 1 from comment #7 only occur when you choose Add Bookmark. Choosing Add Task does not exercise the problematic code, because it overrides the run() method to open a dialog. That is probably why you didn't see the IllegalAccessError again, but it is still there for Add Bookmark, as is the problematic label. And you are also right, I must not have tested the "add bookmark with editor open" code path. I must have tried only tasks, which do not have the problem. The latest patches address these problems. I've just successfully added tasks and bookmarks from the editor ruler, from the editor's Edit... menu, and from the Navigator's Edit... menu when the editor is closed. In all cases, the labels are good, undo/redo work, etc. Sorry for the miscommunication. Created attachment 50183 [details]
org.eclipse.ui.editors.patch
Note there are three TODO:s in this patch concerning the extra logging and the possible null tooltip text.
Also note that the protected method getTextEditor() is called from the IAdaptable created in MarkerRulerAction, but it does not cause an IllegalAccessError. I stepped through the code to ensure that it is called. I do not understand why and will open a separate core bug on this issue cc'ing you.
Dani, you likely remember this from the original bug, but the reason for the inconsistencies in the IllegalAccessError is documented in bug #152568. My workaround (using a final variable) is different than that adopted in platform text elsewhere (using a dummy method that calls the protected method) so you may choose to handle it differently. I assume you'll address this when you incorporate the patches. reopening to resolve I released the code along the lines of the patch. I liked to use a final local var better than adding another method but directly assigned the shell to it. |