⌘/ (Ctrl/ on non-Mac) comments/uncomments lines in all code editors I’ve tested (Xcode, IntelliJ IDEA, Sublime Text 2, Textmate). In Web Inspector it currently pause/continue script execution. I believe, it came from Firebug that doesn’t support code editing. I suggest to change this shorthand to something else. For example, Xcode uses Ctrl + Cmd + Y.
Sounds good to me. Do you have a patch?
Not yet. I’m going to start working on it.
Given that Nikita gave no sign since July, I'll give it a try with this bug :)
Go ahead, Mihai. Few things to consider: It should work with both the current editor and CodeMirror. Sublime Text adds adds "//" at the current indent level: if (1) // foo Conversely, Xcode and IntelliJ IDEA adds "//" at the beginning of the line, e.g.: if (1) // foo I’m fine with either of them as long as it doesn’t comment a line twice: if (1) // // foo Xcode suffers from it.
> It should work with both the current editor and CodeMirror. Correction, CodeMirror is a non-goal for Web Inspector (at least not in its current state). I'll remove the experiment.
(In reply to comment #5) > > It should work with both the current editor and CodeMirror. > > Correction, CodeMirror is a non-goal for Web Inspector (at least not in its current state). I'll remove the experiment. Why so? Any details somewhere?
v2 had gutter problems we could not afford (https://github.com/marijnh/CodeMirror/issues/730). When v3 is ready, we will perform another assessment.
Created attachment 165135 [details] Proposed bug fix. Replaced Cmd + / from Pause script to Toggle code comment.
Selection moves 2 characters on the left after commenting out a line. Screencast: http://www.screenr.com/O338. Also, on 17 second I select one whole line but it comments out the next one too.
Comment on attachment 165135 [details] Proposed bug fix. Replaced Cmd + / from Pause script to Toggle code comment. View in context: https://bugs.webkit.org/attachment.cgi?id=165135&action=review > Source/WebCore/inspector/front-end/DefaultTextEditor.js:1594 > + if (line.match(commentRegEx)) { this will not work for multiple lines with comments: var a = 1; // a comment var b = 2; will become: // var a = 1; a comment // var b = 2; while should be: // var a = 1; // // a comment // var b = 2; > Source/WebCore/inspector/front-end/DefaultTextEditor.js:1606 > + }, return true;
Also how about adding "// " (with a space at the end) instead of "//"?
(In reply to comment #11) > Also how about adding "// " (with a space at the end) instead of "//"? Sounds doable :) What should happen however when removing the comments from the beginning of the line? Should it remove only "//"? Only "// "? Both? One of the reasons I opted for "//" is that it preserves the most of the original code when adding/removing (even though there are cases when after a comment/uncomment the source will not be same anymore).
Comment on attachment 165135 [details] Proposed bug fix. Replaced Cmd + / from Pause script to Toggle code comment. View in context: https://bugs.webkit.org/attachment.cgi?id=165135&action=review >> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1594 >> + if (line.match(commentRegEx)) { > > this will not work for multiple lines with comments: > > var a = 1; > // a comment > var b = 2; > > will become: > > // var a = 1; > a comment > // var b = 2; > > while should be: > > // var a = 1; > // // a comment > // var b = 2; I have already discussed this aspect on IRC with apavolov and a new patch is in works, complete with unit tests and a some refactoring of the code :)
Created attachment 166026 [details] Proposed bug fix. Replaced Cmd + / from Pause script to Toggle code comment. Missing expected results for tests. Doesn't have expected results for the test. What's the recommended way of producing the expected results?
Attachment 166026 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1 LayoutTests/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 166027 [details] Proposed bug fix. Missing expected results for tests.
Created attachment 166030 [details] Proposed bug fix. Missing expected results for tests. Minor style fixes.
Comment on attachment 166030 [details] Proposed bug fix. Missing expected results for tests. Attachment 166030 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14059004 New failing tests: inspector/editor/comment-toggling.html
Comment on attachment 166030 [details] Proposed bug fix. Missing expected results for tests. Attachment 166030 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14056048 New failing tests: inspector/editor/comment-toggling.html
(In reply to comment #14) > Created an attachment (id=166026) [details] > Proposed bug fix. Replaced Cmd + / from Pause script to Toggle code comment. Missing expected results for tests. > > Doesn't have expected results for the test. What's the recommended way of producing the expected results? When you run a test with missing expectations, DRT will write down the "*-expected.txt" file for you, so you just need to scrutinize it and make sure the results are correct. And don't forget to add it to your patch :)
Comment on attachment 166030 [details] Proposed bug fix. Missing expected results for tests. View in context: https://bugs.webkit.org/attachment.cgi?id=166030&action=review > Source/WebCore/inspector/front-end/DefaultTextEditor.js:1581 > + _commentLines: function(range, commentRegEx) So, we already have _indentLines and _unindentLines methods, which do almost the same work (just a regexp is different). I'd guess there must be much of the code that can be reused. Also, I'd move out these fancy IDE-like code transformations from the basic editor functionality into a separate class, and ideally make it work with any TextEditor implementation. Also, we should look at how Xcode comments the lines. For example this test case: //line1 // line2 //line3 //line4 // line5 I don't have an Xcode by my hand, but Sublime for example, would uncomment all lines first, and then comment in the following way: // line1 // line2 // line3 // line4 // line5 (Note, that empty lines are ignored, unlike your implementation). Also, we'll probably want Ctrl+Shift+/ to add block comments /* ... */
Comment on attachment 166030 [details] Proposed bug fix. Missing expected results for tests. r- per Andrey's comments.
Comment on attachment 166030 [details] Proposed bug fix. Missing expected results for tests. View in context: https://bugs.webkit.org/attachment.cgi?id=166030&action=review >> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1581 >> + _commentLines: function(range, commentRegEx) > > So, we already have _indentLines and _unindentLines methods, which do almost the same work (just a regexp is different). I'd guess there must be much of the code that can be reused. > > Also, I'd move out these fancy IDE-like code transformations from the basic editor functionality into a separate class, and ideally make it work with any TextEditor implementation. > > Also, we should look at how Xcode comments the lines. For example this test case: > > //line1 > // line2 > //line3 > > //line4 > // line5 > > I don't have an Xcode by my hand, but Sublime for example, would uncomment all lines first, and then comment in the following way: > > // line1 > // line2 > // line3 > > // line4 > // line5 > > (Note, that empty lines are ignored, unlike your implementation). > > Also, we'll probably want Ctrl+Shift+/ to add block comments /* ... */ First of all, is really block commenting in the scope for this bug? Because I think it needs even more specifying than line comments (e.g. should the editor look for block comments outside the selection? Should it look for comments inside the selection but smaller than it? What should happen if the beginning of the selection is on a line that is commented with //? etc.) Finally, what are the actual r- items (besides the missing test expectations) and what are nice-to-have-s (possibly extractable in a different patch)?
(In reply to comment #23) > (From update of attachment 166030 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166030&action=review > > >> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1581 > >> + _commentLines: function(range, commentRegEx) > > > > So, we already have _indentLines and _unindentLines methods, which do almost the same work (just a regexp is different). I'd guess there must be much of the code that can be reused. > > > > Also, I'd move out these fancy IDE-like code transformations from the basic editor functionality into a separate class, and ideally make it work with any TextEditor implementation. > > > > Also, we should look at how Xcode comments the lines. For example this test case: > > > > //line1 > > // line2 > > //line3 > > > > //line4 > > // line5 > > > > I don't have an Xcode by my hand, but Sublime for example, would uncomment all lines first, and then comment in the following way: > > > > // line1 > > // line2 > > // line3 > > > > // line4 > > // line5 > > > > (Note, that empty lines are ignored, unlike your implementation). > > > > Also, we'll probably want Ctrl+Shift+/ to add block comments /* ... */ > > First of all, is really block commenting in the scope for this bug? Because I think it needs even more specifying than line comments (e.g. should the editor look for block comments outside the selection? Should it look for comments inside the selection but smaller than it? What should happen if the beginning of the selection is on a line that is commented with //? etc.) > > Finally, what are the actual r- items (besides the missing test expectations) and what are nice-to-have-s (possibly extractable in a different patch)? As far as I'm concerned, r- is for code duplication.
> > As far as I'm concerned, r- is for code duplication. OK, I'll fix that :)
Sorry for the somewhat unrelated and belated comment. (In reply to comment #5) > Correction, CodeMirror is a non-goal for Web Inspector (at least not in its current state). I'll remove the experiment. (In reply to comment #7) > v2 had gutter problems we could not afford (https://github.com/marijnh/CodeMirror/issues/730). When v3 is ready, we will perform another assessment. Pavel please don't delete the experiment just yet, I have an upcoming patch to update to v3. It should be stable soon, will fix the #94118 gutter problem, and should help implementing #94056 and #94062 a great deal. Also, implementing `Cmd + /` in the experiment shouldn't be a problem.
Closing as invalid, as this bug pertains to the old inspector UI and/or its tests. Please file a new bug (https://www.webkit.org/new-inspector-bug) if the bug/feature/issue is still relevant to WebKit trunk.