Bug 315446

Comment 1 Patrick Chuong 2010-06-02 14:35:25 EDT

Created attachment 170856 [details]
Corrected the bundle name

Comment 2 Marc Khouzam 2010-06-02 14:57:11 EDT

I got this one.

Comment 3 Patrick Chuong 2010-06-02 15:00:26 EDT

(In reply to comment #2)
> I got this one.

I updated the title, I didn't realize the text was from the last bug that I filed.

Comment 4 John Cortell 2010-06-02 15:02:16 EDT

I was just looking at why we have two *near* identical implementations of IToggleBreakpointsTargetFactory. One in cdt.debug, on in dsf:

org.eclipse.cdt.debug.internal.ui.actions.ToggleCBreakpointsTargetFactory
org.eclipse.cdt.dsf.debug.internal.ui.ToggleBreakpointsTargetFactory

Was going to see if there was some way to consolidate these two, but I'll let you take it from here.

Comment 5 Marc Khouzam 2010-06-02 15:03:49 EDT

Committed the fix to HEAD.
Thanks Patrick.

I'll answer John separately.

Comment 6 Pawel Piech 2010-06-02 15:05:33 EDT

Comment on attachment 170856 [details]
Corrected the bundle name

I think this means that we need to update the IP log for the Helios release.

Comment 7 Marc Khouzam 2010-06-02 15:07:37 EDT

(In reply to comment #4)
> I was just looking at why we have two *near* identical implementations of
> IToggleBreakpointsTargetFactory. One in cdt.debug, on in dsf:
> 
> org.eclipse.cdt.debug.internal.ui.actions.ToggleCBreakpointsTargetFactory
> org.eclipse.cdt.dsf.debug.internal.ui.ToggleBreakpointsTargetFactory
> 
> Was going to see if there was some way to consolidate these two, but I'll let
> you take it from here.

The first is the original one and works for the CEditor.
The second was added for the DSF Disassembly view.  This new one could not be added to cdt.debug because it needs access to IDisassemblyPart which is part of the DSF plugins.

After the refactoring of the DSF Disassembly view that you did, maybe there is a better way?

Comment 8 Marc Khouzam 2010-06-02 15:08:16 EDT

(In reply to comment #6)
> (From update of attachment 170856 [details])
> I think this means that we need to update the IP log for the Helios release.

Woops.  I'll email Doug.

Comment 9 John Cortell 2010-06-02 15:20:37 EDT

(In reply to comment #7)
> The first is the original one and works for the CEditor.
> The second was added for the DSF Disassembly view.  This new one could not be
> added to cdt.debug because it needs access to IDisassemblyPart which is part of
> the DSF plugins.
> 
> After the refactoring of the DSF Disassembly view that you did, maybe there is
> a better way?

Well, it seems we could have the DSF one simply extend the cdt.debug one. This would allow it to reuse at least some of the implementation

* getToggleTargetDescription(String)
* getToggleTargetName(String)
* getDebugContext(IWorkbenchPart)
* TOGGLE_C_BREAKPOINT_TARGET_ID

No need to have the name and description stored in two places. This bug would not have happened had that been the case. Also, note the big old comment at the DSF version that would become mostly irrelevant.

Not a big deal though.

Comment 10 CDT Genie 2010-06-02 15:23:16 EDT

*** cdt cvs genie on behalf of mkhouzam ***
Bug 315446: Wrong bundle name makes NLS strings fail to be found.

[*] Messages.java 1.2 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/Messages.java?root=Tools_Project&r1=1.1&r2=1.2

Comment 11 Marc Khouzam 2010-06-02 15:28:15 EDT

(In reply to comment #9)

> Well, it seems we could have the DSF one simply extend the cdt.debug one. This
> would allow it to reuse at least some of the implementation
> 
> * getToggleTargetDescription(String)
> * getToggleTargetName(String)
> * getDebugContext(IWorkbenchPart)
> * TOGGLE_C_BREAKPOINT_TARGET_ID
> 
> No need to have the name and description stored in two places. This bug would
> not have happened had that been the case. Also, note the big old comment at the
> DSF version that would become mostly irrelevant.
> 
> Not a big deal though.

Hadn't thought of it.  We can do it when the plugin goes to 3.0...