Bug 228063 - ToggleInstructionStepModeActionDelegate should use ISteppingModeTarget instead of ICDebugTarget
Summary: ToggleInstructionStepModeActionDelegate should use ISteppingModeTarget instea...
Status: RESOLVED FIXED
Alias: None
Product: CDT
Classification: Tools
Component: cdt-debug (show other bugs)
Version: 5.0   Edit
Hardware: PC All
: P3 normal (vote)
Target Milestone: 5.0 M7   Edit
Assignee: Nobody - feel free to take it CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed
Depends on:
Blocks: 228062
  Show dependency tree
 
Reported: 2008-04-21 13:37 EDT by Pawel Piech CLA
Modified: 2008-06-22 14:13 EDT (History)
2 users (show)

See Also:


Attachments
Patch with fix. (4.58 KB, patch)
2008-04-21 16:34 EDT, Pawel Piech CLA
bjorn.freeman-benson: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pawel Piech CLA 2008-04-21 13:37:52 EDT
The ISteppingModeTarget and ITargetProperties interfaces are narrower than ICDebugTarget and could be implemented by DSF or another debug model.
Comment 1 Nobody - feel free to take it CLA 2008-04-21 14:00:59 EDT
In the new disassembly ToggleStepModeActionDelegate will be replaced by the new drop down menu with the following options: "Context, Source, Instruction".
The implementation is based on the command framework and will use ISteppingModeTarget instead of ICDebugTarget.
I have been planning to add this code in the next couple of days. Will these changes break your plans?
Comment 2 Pawel Piech CLA 2008-04-21 14:37:55 EDT
(In reply to comment #1)
> In the new disassembly ToggleStepModeActionDelegate will be replaced by the new
> drop down menu with the following options: "Context, Source, Instruction".
> The implementation is based on the command framework and will use
> ISteppingModeTarget instead of ICDebugTarget.
> I have been planning to add this code in the next couple of days. Will these
> changes break your plans?
> 


I don't think it should interfere as long as it still works with ISteppingModeTarget, and I expect your solution to be more general anyway :-)

However, as far as the change itself, I would advise some caution.  The disassembly proposal you wrote at http://wiki.eclipse.org/DSDP/DD/DisassemblyView talked about APIs and interfaces but it didn't talk much about UI changes.  In the past when working on this feature in the Wind River product, I have found it difficult to achieve a smooth workflow when it comes to instruction stepping/dissasembly viewing.  Before making the change you're suggesting above I would first describe the UI changes and workflow you're implementing and ask for feedback from the community, otherwise you may get some backlash.
Comment 3 Pawel Piech CLA 2008-04-21 16:34:52 EDT
Created attachment 96927 [details]
Patch with fix.

Attached patch converts ToggleInstructionStepModeActionDelegate to not use ICDebugTarget.  It's a relatively simple and safe change.
Comment 4 Nobody - feel free to take it CLA 2008-04-21 17:17:41 EDT
(In reply to comment #2)
There are several options for this features:
1. Make the action visible only for the models that provide "disassembly context". Others can contribute their own actions.
2. Make the content of the dropdown menu configurable. In this case the "Context" action can be added only when it is supported. So the UI functionality will be the same. But this would require a special interface.
Comment 5 Pawel Piech CLA 2008-04-21 17:50:47 EDT
I don't I fully understand your whole vision about how the instruction step mode should be enabled.  Specifically I don't understand what "Context" in the drop-down would mean.  I think writing up the whole UI story in a document would be very helpful.  And if you have this already partly implemented, then you could use more screen shots and fewer words :-)
Comment 6 Nobody - feel free to take it CLA 2008-04-21 18:25:22 EDT
(In reply to comment #5)
> I don't I fully understand your whole vision about how the instruction step
> mode should be enabled.  Specifically I don't understand what "Context" in the
> drop-down would mean.  I think writing up the whole UI story in a document
> would be very helpful.  And if you have this already partly implemented, then
> you could use more screen shots and fewer words :-)

I will. Don't forget that I haven't added it yet, you forced me to disclose it :)

As for "Context" see https://bugs.eclipse.org/bugs/show_bug.cgi?id=79872. It is marked as fixed but doesn't seem to work. I wonder how many features have disappeared :(
Comment 7 Pawel Piech CLA 2008-04-21 22:23:22 EDT
> (In reply to comment #5)
I completely understand.  I see some possible problems with this feature, but I'll let you have a chance to explain it fully before I raise any questions.

Comment 8 Nobody - feel free to take it CLA 2008-05-02 08:26:12 EDT
Applied the patch as it is as we decided to move the new disassembly to 5.1.
Comment 9 Pawel Piech CLA 2008-05-02 11:34:26 EDT
Thank you Mikhail :-)