Bugzilla – Full Text Bug Listing
| Summary: | Move DebugCommandAction to an API package | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Pawel Piech <pawel.1.piech> |
| Component: | Debug | Assignee: | Pawel Piech <pawel.1.piech> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | darin.eclipse, john.arthorne, marc.khouzam |
| Version: | 3.5 | Keywords: | plan |
| Target Milestone: | 3.6 M2 | Flags: | darin.eclipse:
review+
|
| Hardware: | PC | ||
| OS: | All | ||
| Whiteboard: | |||
| Bug Depends on: | |||
| Bug Blocks: | 275687, 286256 | ||
| Attachments: | |||
|
Description
Pawel Piech
Created attachment 145005 [details]
Patch with refactored commands.
This patch moves the base actions to a public package. It also adds a command framework handler base class. This is work in progress as I still need to test thoroughly the handler.
Pawel, this patch appears to be the checked tree viewer code rather than the actions :-) Created attachment 145060 [details] Correct and updated patch with refactored commands. (In reply to comment #2) > Pawel, this patch appears to be the checked tree viewer code rather than the > actions :-) > Clearly I did not get enough sleep last night. Here is the correct patch. I also included in this change the base class for IDebugCommandHandler implementations, which was also internal but very useful. Created attachment 145061 [details]
Patch with DebugCommandHandler.
This patch contains just the command framework handler along with some test code in the PDA example. I spent most of the day trying to get this to work correctly, but it still doesn't when multiple workbench windows are shown. The problem is that the command framework assumes that there is only one handler for all commands in all views and all windows, and the handler's state does not depend on the view/window in which it sits, but only on the active view/window. After many frustrating attempts at using the command framework I'm just about ready to give up on it. If we could only hide view toolbar actions based on active action sets....
I agree with openning up the abstract infrastructure classes - but I think this needs some more work before we committ this as API. I think we need to document how these actions are used in javadoc (i.e. the linkage between the action/command extension, the action delegate and command action - which is quite confusing). We have a pattern of contributed action, points to an action delegate which delegates to a command (action). This allows us to re-use the command (action) code for first class actions and contributed actions. It would be nice to be able to do this without having to have API for DebugCommandActionDelegate and ICommandParticipant. We need to explain how the actions update and execute, without committing ourselves to the "update servive" implementation (which is an internal implementation detail that could change...). Created attachment 145200 [details]
Patch eliminating command participant and delegate from API.
This patch removes the delegate and the command participant from the API, though it doesn't make the usage any more clear.
I agree about the confusing usage of actions and delegates and that the usage needs to be very well documented to become API. I wish we could rename some things to avoid confusion with the platform command framework (e.g. IDebugCommandHandler -> IDebugRequestTarget), but oh well..
What do you think about the command framework handler? Is it worth it to have a handler even if it can only operate on the active debug context in the active window? As I mentioned before I was hoping to use it in CDT in the debug view toolbar. I know that the implementation that's in CDT right now has the same limitation, so maybe it's not such a big deal in real-world use cases.
Created attachment 146333 [details]
Updated patch with comments.
I added comments and left in the command framework handler.
Darin, please see if the latest patch addresses your concerns. Created attachment 146696 [details]
updated patch
I updated the javadoc in AbstractDebugCommand, added a few methods, and renamed a few (to hopefully improve the API and make it more consistent).
In the UI side a renamed set/getAction(IAction) to set/getActionProxy(...), and updated the javadoc. It's wordy, but hopefully is more explicit.
However, why is the fInitialized boolean and associated synchronization code removed from DebugCommandActionDelegate? I think this was important for something...
(In reply to comment #9) > Created an attachment (id=146696) [details] > updated patch > > I updated the javadoc in AbstractDebugCommand, added a few methods, and renamed > a few (to hopefully improve the API and make it more consistent). Makes sense to me... though the whole pattern is admittedly still pretty confusing. However, there are so many examples in the platform plugin that it shouldn't be hard for others to follow. > However, why is the fInitialized boolean and associated synchronization code > removed from DebugCommandActionDelegate? I think this was important for > something... Per comment #5, I tried to remove the need to include the DebugCommandActionDelegate, so I moved the synchronization logic to the action itself (see AbstractDebugAction.run() and AbstractDebugAction.setEnable()). I believe that the logic is equivalent, but a second set of eyes would help. BTW, this synchronization logic is a little scary and it has lead to some deadlocks in our product in the past, but I believe the only alternative is to let the model report an error on an action that should not have been enabled... and existing models may not be prepared to do that. OK, I see it now. Looks fine to me. You should also update the buildnotes to include the new API type AbstractDebugCommand in debug.core. +1. Fire when ready. And... there's one string that needs to be externalized: Description Resource Path Location Type Non-externalized string literal; it should be followed by //$NON-NLS-<n>$ DebugCommandHandler.java /org.eclipse.debug.ui/ui/org/eclipse/debug/ui/actions line 194 Java Problem Added the suggested changes and committed the latest patch. Fix already reviewed. Is it possible that the PDA example of this change didn't get committed? Was that intentional? (In reply to comment #15) > Is it possible that the PDA example of this change didn't get committed? Was > that intentional? Make sure you're looking at the platform PDA example (org.eclipse.debug.examples.core/ui, and not the DSF PDA example. (In reply to comment #16) > Make sure you're looking at the platform PDA example > (org.eclipse.debug.examples.core/ui, and not the DSF PDA example. Yeah, I got those checked out but I don't see anything that extends DebugCommandHandler. I believe PopFrameHandler should be there, but I cannot find it. (In reply to comment #17) > (In reply to comment #16) > > Make sure you're looking at the platform PDA example > > (org.eclipse.debug.examples.core/ui, and not the DSF PDA example. > > Yeah, I got those checked out but I don't see anything that extends > DebugCommandHandler. I believe PopFrameHandler should be there, but I cannot > find it. Hi Marc, I'm sorry, I didn't actually commit the test code for the handler in PDA. I only used it for development purposes. What I was thinking of in PDA is the restart handler... but I guess that's a different bug. (In reply to comment #18) > Hi Marc, > I'm sorry, I didn't actually commit the test code for the handler in PDA. I > only used it for development purposes. What I was thinking of in PDA is the > restart handler... but I guess that's a different bug. No problem. I've applied the patch manually to my workspace and it has been very helpful. I'm having a little problem with enablement in DebugCommandHandler. In some situation, my button stays disabled when it should not. I've noticed that in DebugCommandHandler.execute there is the line fCurrentEnabledTarget.setEnabled(enabledAfterExecute) which in my case disables my button. I wasn't able to see what would re-enable it though. Maybe the method postExecute() should take care of that? Thanks (In reply to comment #20) Actions such as resume/suspend/step, etc. become disabled immediately after being invoked. This disabling is controlled by the return value from the IDebugCommandHandler.execute(). It is expected that invoking one of these commands on an element is going to cause a change in the model and the model will post an update event on the element. That in turn should lead to a IDebugContextListener.debugContextChanged() to be called. debugContextChanged() calls DebugCommandService.postUpdateCommand() to request that the command service call IDebugCommandHandler.canExecute(). After canExecute() completes, the command service should call IEnabledTarget.setEnabled(). Yeah, it sounds a little nutty, but that's why we didn't want to make the command service itself become an API ;-) (In reply to comment #21) > (In reply to comment #20) > > Actions such as resume/suspend/step, etc. become disabled immediately after > being invoked. This disabling is controlled by the return value from the > IDebugCommandHandler.execute(). Ok, I saw that. > It is expected that invoking one of these > commands on an element is going to cause a change in the model and the model > will post an update event on the element. That in turn should lead to a > IDebugContextListener.debugContextChanged() to be called. Ah, I see. Ok, so in my case this does not happen. But I probably just need to override postExecute() to call DebugCommandService.postUpdateCommand() > Yeah, it sounds a little nutty, but that's why we didn't want to make the > command service itself become an API ;-) But with your explanations, things are much clearer. Thanks! (In reply to comment #22) > Ah, I see. Ok, so in my case this does not happen. > But I probably just need to override postExecute() to call > DebugCommandService.postUpdateCommand() You shouldn't have to, because an IResumedDMEvent should cause the thread (and actions) to update. Besides DebugCommandService is internal so you can't call it directly. (In reply to comment #23) > (In reply to comment #22) > > Ah, I see. Ok, so in my case this does not happen. > > But I probably just need to override postExecute() to call > > DebugCommandService.postUpdateCommand() > > You shouldn't have to, because an IResumedDMEvent should cause the thread (and > actions) to update. In my case, there is no event that is triggered for that particular action. > Besides DebugCommandService is internal so you can't call it directly. Doh! But I found another workaround. By overriding AbstractDebugCommand.isRemainEnabled() and having it return true, I can keep my button enabled properly. For my particular command, that is not a problem. Thanks for the help. |