Bugzilla – Full Text Bug Listing
| Summary: | Feature: Global enable/disable of breakpoints (and leave breakpoints list untouched) | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Tom Mutdosch <mutdosch> |
| Component: | Debug | Assignee: | Jared Burns <jared_burns> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | darin.eclipse, eclipse, jared_burns, John_Wiegand, stefan-l, turnham |
| Version: | 2.0 | ||
| Target Milestone: | 3.0 M8 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
|
Description
Tom Mutdosch
Deferred currently. However, see bug# 8937, which will help. Yes, Select-All on breakpoints would help, but in that case you would lose the information of which breakpoints that you had previously enabled/disabled at that time. Just pontificating on this some more on a Friday night... I guess perhaps a more simple notion of looking at this functionality would be an option to Ignore Selected Breakpoints. So when that option is selected, then execution will not stop at any breakpoints in the list, regardless of their state (enabled/disabled). When the option is deselected, breakpoints have the normal behavior. What this does is allow the user to have their set of constant breakpoints enabled, but an easy way to temporarily turn them all off. For example if they have many breakpoints enabled, and just want to get past the current debug stepping and get back to a certain state of their application, at which point they would want all of their breakpoints set again to start debugging from that point. *** Bug 35188 has been marked as a duplicate of this bug. *** Consider for 3.0 Looking at our existing infrastructure, this is actually problematic - from an API point of view, breakpoints have a binary state - enabled/disabled, and exist/do not exist. This adds another dimension that existing breakpoint listeners will not be looking for. It may be possible to sneak this in as a simple presentation change, and actually remove or de-register the breakpoints from the breakpoint manager. However, that makes persistence more difficult. I wrote a simple eclipse plugin to perform something along the lines of what I was looking for - it simply disables all breakpoints at the press of a button, and remembers the state of the breakpoints that were enabled. Programmatically, when a user presses a Suspend Breakpoints toolbar button, I keep a List (BreakpointsToBeEnabled) of all of the currently enabled breakpoints and then disable all breakpoints. When the button is toggled back, I then go through my list and enable all of the breakpoints in my BreakpointsToBeEnabled list. This does require some list maintenance such as listening for when breakpoints are removed, etc to make sure that the list I have is up-to-date. There's probably more elegant solutions, but it seems to get the job done without changing any existing infrastructure. It's pretty much similar to selecting multiple breakpoints, disabling them, and then later re-enabling them. Yep - that works. But from a UI point of view, I'd like to show users greyed out breakpoints with the enable/disable state intact. Disabling all breakpoints (and rendering them as such) is misleading to the user (I think). Agreed, that would be ideal. Yep, my way was hacky in that like you said, you really don't want to disable breakpoints; functionally you just want to stop the debugger from stopping at any existing breakpoints. I'd definitely like a solution such as you mentioned. Keep me updated, if you make any progress. Thanks. Will investigate for M8. After giving this some thought, I don't think it's possible for us to implement this in the Debug Platform in a way that it will just magically work for debugger providers with no additional work. Something we could easily do, though, would be to add an API to the breakpoint manager for setting and getting the global enablement and fire a new breakpointChanged event when this state is toggled. This would put the work of honoring the setting on debugger providers. I've looked at how we would do this in the JDT debugger and it looks like it would be simple for us. The biggest problem here is that you could get debuggers that don't honor the setting, in which case the user would be confused when they toggle the "disable all breakpoints" button and their breakpoints are still hit. But I'm not convinced that this is a crippling shortcoming. Hmmm... Yes, I think that adding the API and then having the debugger providers honor the setting seems like a reasonable solution. As you mention, it's probably not a severe limitation if a particular debugger does not respond to the toggle. What's wrong with this approach: Allow the breakpoint manager to be enabled/disabled. When disabled, breakpoint listeners are notified that all breakpoints are removed. When enabled, listeners are notified that all breakpoints have been added. We do not fire change notification when in the disabled state. We'd also add API to get the "offline" breakpoints - asking for breakpoints (getBreakpoints()) when disabled would return an empty collection. New methods on IBreakpointManager * #setEnabled(boolean) * #isEnabled() * #getOfflineBreakpoints() The only issue I see is firing change notification. If a user changed a breakpoint property while the breakpoint manager was diabled, listeners would not be notified - and thus may not update views to reflect the change. That idea really rubs me the wrong way. Something about "lying" to clients that makes me expect this to break debuggers. And things being what they are, I'm sure we wouldn't hear about it until after we ship. If a listener cares that a breakpoint has been added or removed, lying to it just sounds like a bad idea in principle. Who knows what people to in response to these events? Delete the breakpoint's marker? Add or remove it from their editor? Connect to a repository? Also, what happens when clients ask for all breakpoints? Do we make the BreakpointManager lie in every method? If we say there are no breakpoints, then clients can't get at breakpoints even though they might need them. If you tell the truth and return the breakpoints, then a new debug target will add all those breakpoints and they'll be hit even though the UI shows that they're turned off. In the end, that idea still wouldn't work unless clients did some extra work *plus* it has unknown potential to break people. Here's my concern - the problem with adding a new attribute to breakpoints that effects their behavior, means a breaking behavior change (although we can maintain binary compatability). It might be too late at this point in the game, and would require PMC approval. I didn't suggest adding a new attribute to breakpoints. However, I see now that the breakpointChanged(...) method only gives the listener the breakpoint and a marker delta. I was expecting it to be more like our DebugEvents, where you can pass an int flag. So instead of using the breakpointChanged listener, we might need a new listener? IBreakpointManagerListener? An added benefit of my approach that I discovered is the fact that it allows debug targets to skip *all* breakpoints, not just the ones that are in the breakpoint manager. For example, Java Debug has two breakpoints that it uses for "suspend on compilation errors" and "suspend for uncaught exceptions". By putting the work in the clients, we let them manage cases like this as they see fit. Please verify, DarinW. Changes to: IBreakpointManager - Added API to support manager enable/disable IBreakpointManagerListener - Created listener to track manager enablement BreakpointManager - Implemented IBreakpointManager API DebugPluginImages - Added icon for "Skip All Breakpoints" action IDebugHelpContextIds - Added help context id for action ActionMessages.properties - Externalized strings SkipAllBreakpointsAction - Breakpoint View toolbar action to toggle manager enablement BreakpointsView - Add the view programatically (otherwise, it wouldn't be registered as a listener until too late) IDebugUIContstants - Added action icon identifier constant JDIDebugTarget - Updated to install/uninstall breakpoints with breakpoint manager enablement Jared, I do not have the propert icon for the "skip breakpoints" action. I get a "red square". I released everything that the synch view showed as outgoing, but now that you mention it, the image wasn't in there. I'll release it when I go home today. I'd created the icon through the filesystem and hadn't done a refresh in the workspace (though the file still loaded just fine for selfhosting). The icon's released now. I like it - but I think we need to enhance the feature :-) * We need a better visual indication that the breakpoints are "offline". Perhaps by rendering the text as "greyed out". As well, it would be beneficial to render the breakpoints differently in the ruler - although they do render as "uninstalled" which may be good enough. * It's misleading now that the "suspend on uncaught exception" preference is on, but I do not suspend when it happens (the user does not know this is a breakpoint). We should really address bug 44894. * There's a bug with the Java debug target #breakpointChanged callback. When I alter a breakpoint and the manager is disabled, that breakpoint gets installed. I think we also need PMC approval for this change - it's not breaking in the sense of binary compatibility, but it means that debug models must support the new feature. Adding John as CC to get his opinion on whether we should forge ahead. * It's misleading now that the "suspend on uncaught exception" preference is on, but I do not suspend when it happens (the user does not know this is a breakpoint). We should really address bug 44894. I wrote it this way on purpose. I agree that it's a little confusing if you think about it, but I think that when the user clicks "Skip All Breakpoints" it's because they don't want their program to suspend. * There's a bug with the Java debug target #breakpointChanged callback. When I alter a breakpoint and the manager is disabled, that breakpoint gets installed. It's actually worse than this. The problem comes from actions that call the breakpoint API's directly, not through breakpointChanged which would let us fix it in one place. Instead, this requires our createRequest() methods (two in JavaBreakpoint and one each in JavaMethodBreakpoint and JavaWatchpoint) to check the breakpoint manager's state and bail out. It's not a ton of changes, but it's still more than I'd rather have to make. Check in changes to fix the problem with breakpoints being installed when they shouldn't. Changes to: JDIDebugTarget JavaBreakpoint JavaMethodBreakpoint JavaWatchpoint Please take a look at these changes, DarinW. Verified function works properly now. This might break "hidden breakpoints". Currently, clients can add unregistered breakpoints to debug targets programatically. It looks like our java debug target would ignore such breakpoints - breaking existing implementations of clients that wrapper our Java debugger. It would mean that we have to honor unregistered breakpoints, but ignore registered breakpoints. This seems more complex again. JDT Debug won't skip non-registered breakpoints now. Once again, changes to: JDIDebugTarget JavaBreakpoint JavaMethodBreakpoint JavaWatchpoint The breakpoint's view now updates the background color when it's skipping breakpoints. Changes to: SkipAllBreakpointsAction - Refactored code to breakpoint's view BreakpointsView - Update action and viewer background for BP manager enablement I think that should do it. I've also talked to JohnW to verify that the changes are fine from an API perspective. Please verify, DarinW. Suspend on uncaught exceptions and compilation errors does not work with the manager disabled. These are "hidden" breakpoints, so they should still be honored. The color change in the view is very subtle - hard to see on WinXP. I also experimented with setting the tree items as "greyed". I released a change to grey check boxes when disabled, and add "Disabled" to the view subtitle. setGrayed() doesn't mean what it sounds like. It's not a color setting, it's a state setting. On Linux-GTK, setGrayed() doesn't change the color at all, in fact. It changes the way the check is drawn. I experimented with this and decided that it's a misuse of the functionality. Adding "Disabled" to the bottom sounds good, though. I know I tested that unregistered breakpoints are still hit. I must have broken them again after fixing it? Can't reproduce the failure you reported about the Suspend on uncaught exceptions feature. Works as advertised for me. :) Sorry my fault - I was testing an NPE in a JUnit test, but of course, the JUnit framework handles the exception. Presentation: I like the fact that the check box is rendered as "grey" it shows that even though the breakpoint is enabled, it is somehow different. Verified. |