Bug 19131 - Can't remove a breakpoint
Summary: Can't remove a breakpoint
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-05-19 19:20 PDT by Timothy Hatcher
Modified: 2009-10-19 23:05 PDT (History)
6 users (show)

See Also:


Attachments
proposed patch (1.56 KB, patch)
2009-08-12 13:01 PDT, Patrick Mueller
timothy: review-
Details | Formatted Diff | Diff
patch with "no test" changelog comment removed (1.53 KB, patch)
2009-08-13 07:29 PDT, Patrick Mueller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2008-05-19 19:20:50 PDT
You can only disable/enable a breakpoint. Need to be able to remove breakpoints. Drag and poof away?
Comment 1 Patrick Mueller 2009-03-20 06:56:20 PDT
Wouldn't it be easier to just make this a tri-state?  For a statement with no breakpoint, consecutive clicking puts the line into a - break point enabled / break point disabled / break point removed - states
Comment 2 Patrick Mueller 2009-08-12 09:44:14 PDT
The inability to delete breakpoints continues to bug me.  At one point recently, one of my source files was just splattered with blue turds as I kept moving breakpoints around, reloading the file, etc.

Here's a patch to change the enable/disable breakpoint click states to enable/disable/remove.  ie, as you click on the line number in a source view, the breakpoint gets added, then disabled, then removed, then added, ...

This seems fairly natural to me.  Another option would be to only add/remove via the click, and allow enable/disable in the new breakpoints sidebar.  Not clear whether removing breakpoints from the sidebar is required - I think I'd argue that if we want to add that, we may well want more function in the sidebar like "disable all/remove all", etc, and so should look at "remove a breakpoint from sidebar" as part of a larger design effort for that sidebar.

I'm happy to kosher this up as a patch, if folks agree the current patch is useful.

Index: WebCore/inspector/front-end/SourceFrame.js
===================================================================
--- WebCore/inspector/front-end/SourceFrame.js	(revision 47011)
+++ WebCore/inspector/front-end/SourceFrame.js	(working copy)
@@ -289,8 +289,10 @@
             return;
 
         var sourceRow = event.target.enclosingNodeOrSelfWithNodeName("tr");
-        if (sourceRow._breakpointObject)
-            sourceRow._breakpointObject.enabled = !sourceRow._breakpointObject.enabled;
+        if (sourceRow._breakpointObject && sourceRow._breakpointObject.enabled)
+            sourceRow._breakpointObject.enabled = false;
+        else if (sourceRow._breakpointObject) 
+            WebInspector.panels.scripts.removeBreakpoint(sourceRow._breakpointObject);
         else if (this.addBreakpointDelegate)
             this.addBreakpointDelegate(this.lineNumberForSourceRow(sourceRow));
     },
Comment 3 Timothy Hatcher 2009-08-12 10:33:31 PDT
Either option seems fine to me. The ideal design that hasn't been implemented yet, would be to allow dragging a breakpoint to move it and dragging out oif the gutter will "poof" it away, like Xcode.
Comment 4 Patrick Mueller 2009-08-12 13:01:47 PDT
Created attachment 34682 [details]
proposed patch

patch as proposed in comment 2
Comment 5 Timothy Hatcher 2009-08-12 14:56:29 PDT
Comment on attachment 34682 [details]
proposed patch

You will need to remove the "No new tests" line other wise the patch wont commit.
Comment 6 Patrick Mueller 2009-08-13 07:29:41 PDT
Created attachment 34739 [details]
patch with "no test" changelog comment removed

removed the no test line from the ChangeLog entry
Comment 7 Eric Seidel 2009-08-13 09:29:57 PDT
Comment on attachment 34739 [details]
patch with "no test" changelog comment removed

Clearing flags on attachment: 34739

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/SourceFrame.js
Committed r47202
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/SourceFrame.js
r47202 = cc0d301a171511657105756961620615f89def37 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/47202
Comment 8 Eric Seidel 2009-08-13 09:30:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Joseph Pecoraro 2009-10-19 23:05:53 PDT
Excellent!  The submitted patch made it so breakpoints can be removed if you click on their "blue tags" enough times.  How about being able to delete them directly from the Breakpoints Sidebar Pane?  Or would the expected user scenario be:

1. Click the entry in the Breakpoints Sidebar Pane to jump directly to the line of code with the blue tag.
2. Click on the tag to disable and then remove.

How about unifying a UI for something like this with the UI for deleting a Watched Expression?