Comment 1 Marc Khouzam 2011-02-10 17:06:16 EST

Nice improvement.  It will greatly help for the hardware bp addition (I assume that is your motivation :-))

One question.  In the new AbstractToggleBreakpointAdapter, you no longer handle the DisassemblyEditor case.  I hadn't put support for it in Tracepoints, but do we want to remove the support for it in breakpoints?

I did notice that you still check for DisassemblyEditor in AbstractToggleBreakpointAdapter#canToggleLineBreakpoints() only

Comment 2 Nobody - feel free to take it 2011-02-10 17:14:21 EST

(In reply to comment #1)
> Nice improvement.  It will greatly help for the hardware bp addition (I assume
> that is your motivation :-))
> 

I would say that's one of my motivations.
 
> One question.  In the new AbstractToggleBreakpointAdapter, you no longer handle
> the DisassemblyEditor case.  I hadn't put support for it in Tracepoints, but do
> we want to remove the support for it in breakpoints?
>

DisassembltEditor is a remnant of my failed attempt to implement disassembly similar to other debug views. You probably remember that story.

> I did notice that you still check for DisassemblyEditor in
> AbstractToggleBreakpointAdapter#canToggleLineBreakpoints() only

I'll remove it, thanks.

Comment 3 Marc Khouzam 2011-02-10 17:24:40 EST

(In reply to comment #2)

> DisassembltEditor is a remnant of my failed attempt to implement disassembly
> similar to other debug views. You probably remember that story.

Oh right.  I got confused with
org.eclipse.cdt.dsf.debug.internal.ui.disassembly.DisassemblyEditor

Comment 4 Nobody - feel free to take it 2011-02-10 17:33:02 EST

Created attachment 188727 [details]
Removed references to DisassemblyEditor

Comment 5 Nobody - feel free to take it 2011-02-10 18:47:45 EST

Created attachment 188729 [details]
Modified patch.

No need to pass the breakpoint type to createLineBreakpoint() and createFunctionBreakpoint().

Comment 6 Marc Khouzam 2011-02-10 20:25:15 EST

(In reply to comment #5)
> Created attachment 188729 [details]
> Modified patch.
> 
> No need to pass the breakpoint type to createLineBreakpoint() and
> createFunctionBreakpoint().

Nice, that should make it very easy for the hw bp stuff.

It would be worth doing the same refactoring for DisassemblyToggleBreakpointsTarget and DisassemblyToggleTracepointsTarget, which are almost identical.  Those two are in the dsf.ui and dsf.gdb.ui plugins because they use the code from the DSF Disassembly view.  I think putting the abstract class in dsf.ui and extending it in both dsf.ui and dsf.gdb.ui would be good.

Comment 7 Nobody - feel free to take it 2011-02-14 11:14:29 EST

(In reply to comment #6)
> It would be worth doing the same refactoring for
> DisassemblyToggleBreakpointsTarget and DisassemblyToggleTracepointsTarget,
> which are almost identical.  Those two are in the dsf.ui and dsf.gdb.ui plugins
> because they use the code from the DSF Disassembly view.  I think putting the
> abstract class in dsf.ui and extending it in both dsf.ui and dsf.gdb.ui would
> be good.

Marc, this will require a new API because the affected classes are in two different plug-ins. I think it is better to move it to a separate bug.

Comment 8 Nobody - feel free to take it 2011-02-15 20:17:28 EST

Marc, is it OK to commit this patch?

Comment 9 Marc Khouzam 2011-02-16 06:47:34 EST

(In reply to comment #7)
> (In reply to comment #6)
> > It would be worth doing the same refactoring for
> > DisassemblyToggleBreakpointsTarget and DisassemblyToggleTracepointsTarget,
> > which are almost identical.  Those two are in the dsf.ui and dsf.gdb.ui plugins
> > because they use the code from the DSF Disassembly view.  I think putting the
> > abstract class in dsf.ui and extending it in both dsf.ui and dsf.gdb.ui would
> > be good.
> 
> Marc, this will require a new API because the affected classes are in two
> different plug-ins. I think it is better to move it to a separate bug.

I opened Bug 337303 about this.

(In reply to comment #8)
> Marc, is it OK to commit this patch?

Of course, sorry for the delay.

Comment 10 Nobody - feel free to take it 2011-02-16 13:43:51 EST

Committed to HEAD. Marc, asking you for a review is a formality - you have already reviewed the patch :)

Comment 11 CDT Genie 2011-02-16 14:21:48 EST

*** cdt cvs genie on behalf of mkhodjai ***
Bug 336875: Refactor ToggleBreakpointAdapter and ToggleTracepointAdapter classes

[*] ToggleBreakpointAdapter.java 1.46 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/actions/ToggleBreakpointAdapter.java?root=Tools_Project&r1=1.45&r2=1.46
[*] ToggleTracepointAdapter.java 1.2 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/actions/ToggleTracepointAdapter.java?root=Tools_Project&r1=1.1&r2=1.2
[+] AbstractToggleBreakpointAdapter.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/actions/AbstractToggleBreakpointAdapter.java?root=Tools_Project&revision=1.1&view=markup