Community
Participate
Working Groups
Build Identifier: HEAD The property action is missing from the breakpoints view when there is a DSF-GDB debug session active. Using platform 3.6M5, this action is not visible. Reproducible: Always
Created attachment 161161 [details] Fix. This patch converts the brakpoint properties action into a handler that gets the breakpoint object through getAdapter()... so that it works with DSF wrappers in view.
I committed the fix. John, please review.
Some insight into why this wasn't working: DSF makes use of the recently added flexible-hierarchy support in the Breakpoints view. Even though ICBreakpoints are being created (by default in stock CDT), the Breakpoints view's model elements aren't CBreakpoints when a DSF session is the active debug context. Rather, they are DSF BreakpointVMContext objects. The Properties action was an object contribution tied to ICBreakpoint.
Pawel, looks good. Just two points: 1. CBreakpointPropertiesHandler.getBreakpoint(Object) should check that only one thing is selected; return null otherwise. Bringing up the properties dialog while multiple breakpoints are selected is confusing. Note that the previous objectContribution action was enabled for only 1. 2. Maybe I'm paranoid, but I would put an Assert.fail("not expecting this to be called") in each of the no-op'ed ISelectionProvider methods to validate assumptions about how PropertyDialogAction will or will not use the selection provider...if for no other reason, to guard against an unexpected future behavioral change.
(In reply to comment #4) > Pawel, looks good. Just two points: > > 1. CBreakpointPropertiesHandler.getBreakpoint(Object) should check that only > one thing is selected; return null otherwise. Bringing up the properties dialog > while multiple breakpoints are selected is confusing. Note that the previous > objectContribution action was enabled for only 1. Good point. I committed the change. > 2. Maybe I'm paranoid, but I would put an Assert.fail("not expecting this to be > called") in each of the no-op'ed ISelectionProvider methods to validate > assumptions about how PropertyDialogAction will or will not use the selection > provider...if for no other reason, to guard against an unexpected future > behavioral change. I'd rather avoid something that would create a regression... this was a cut and paste job from the old action and I'm not too familiar with the properties dialog.
(In reply to comment #5) > > 2. Maybe I'm paranoid, but I would put an Assert.fail("not expecting this to be > > called") in each of the no-op'ed ISelectionProvider methods to validate > > assumptions about how PropertyDialogAction will or will not use the selection > > provider...if for no other reason, to guard against an unexpected future > > behavioral change. > I'd rather avoid something that would create a regression... this was a cut > and paste job from the old action and I'm not too familiar with the properties > dialog. An assertion check wouldn't cause a regression for the end-user, as it's inactive in that environment. However, it would signal us (during development) that something unexpected and wrong is happening in the code--that we've made a bad assumption.
(In reply to comment #6) I added an assert in the setSelection() method. The other ones could reasonably still be called by the dialog.
True. Even if someone calls the provider to register a selection changed listener, since we know the selection can't change (we now assert that), we'd never have a reason to call the listeners. Thus it would be pointless for the provider to record any listeners; i.e., the no-op is a valid implementation. Thanks.
BTW, can you tell I've been buried in junit code lately by my recommendation to use Assert.fail()? :-)
With this patch, I am not seeing the "Properties..." menu action in the breakpoints view. Also, there is an exception Unable to create menu item "org.eclipse.cdt.debug.menu.command.breakpointProperties", command "org.eclipse.cdt.debug.command.breakpointProperties" not defined
(In reply to comment #10) > With this patch, I am not seeing the "Properties..." menu action in the > breakpoints view. > > Also, there is an exception > > Unable to create menu item > "org.eclipse.cdt.debug.menu.command.breakpointProperties", command > "org.eclipse.cdt.debug.command.breakpointProperties" not defined Are you able to debug this further. The patch included a org.eclipse.cdt.debug.command.breakpointProperties command. Also, are you seeing this for CDI, DSF-GDB, etc?
(In reply to comment #11) > Are you able to debug this further. The patch included a > org.eclipse.cdt.debug.command.breakpointProperties command. Also, are you > seeing this for CDI, DSF-GDB, etc? I don't see the command (org.eclipse.cdt.debug.command.breakpointProperties) being defined anywhere in the xml file for the org.eclipse.ui.commands extension point. I am using dsf-gdb with a CLine breakpoint, with the platform HEAD and CDT HEAD.
one additional note: without an active debug session, I don't see the menu action in the breakpoints view.
(In reply to comment #12) > I don't see the command (org.eclipse.cdt.debug.command.breakpointProperties) > being defined anywhere in the xml file for the org.eclipse.ui.commands > extension point. Looks like John refactored it out when removing the CDI disassembly view.
(In reply to comment #14) > (In reply to comment #12) > > I don't see the command (org.eclipse.cdt.debug.command.breakpointProperties) > > being defined anywhere in the xml file for the org.eclipse.ui.commands > > extension point. > Looks like John refactored it out when removing the CDI disassembly view. Not intentionally. I'm on it...
(In reply to comment #15) > > Looks like John refactored it out when removing the CDI disassembly view. > > Not intentionally. I'm on it... I had a moment of hesitation when placing the declaration along with the disassembly commands, but laziness prevailed. I filed a separate bug 305875 ...
(In reply to comment #16) > (In reply to comment #15) > > > Looks like John refactored it out when removing the CDI disassembly view. > > > > Not intentionally. I'm on it... > > I had a moment of hesitation when placing the declaration along with the > disassembly commands, but laziness prevailed. I filed a separate bug 305875 > ... Laziness + sloppiness = missing feature.
