725430 - Add feature to sourceeditor/scratchpad: comment/uncomment line/block of code.

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Johan C]

Menuitem(s) with keyboard shortcut(s) for commenting/uncommenting line(s)/block of code.

Use case 1:
E.G.
1) Select line(s) of code
2) Use the menuitem/keyboard shortcut for "//" single-line comments
3) The selected line(s) are commented/uncommented

Use case 2:
E.G.
1) Select some code.
2) use the menuitem/keyboard shortcut for "/**/" (block) comments
3) The selected code is commented/(uncommented? (if possible))

Suggestions for keyboard shortcuts (windows):
Single-line "//": ctrl-Q
Block "/**/": ctrl-shift-Q

Comment 1

[Cedric Vivier [:cedricv]]

How about rather Ctrl-/ or Ctrl-M - then seem more popular shortcuts for this feature (and are both easier to relate to comments than "Q")

Also, it should work when there is no selection:
1) Press key binding or menu item
2) When there is no selection, the whole current line where the caret is on is commented

Comment 2

[Pranav Ravichandran [:pranavrc]]

I'm willing to work on this bug. Can it be assigned?

Comment 3

[Pranav Ravichandran [:pranavrc]]

I kind of have a question here. I wrote a patch for commenting a selected block of code that replaces the selection string with a new string which is the concatenation of the comment symbols and the selected string. Like '/*'+selection+'*/'. Does this sound hackish? Or is it fine? Just wondering if I should continue with the patch or startover with a different strategy.

Comment 4

[Pranav Ravichandran [:pranavrc]]

Another method here would be to use two setText() methods, each having the same start and end offsets as the start and end of the selection respectively.

Comment 5

[Mihai Sucan [:msucan]]

Just a quick note: we have already discussed these concerns on IRC and Pranav will submit a patch based on that.

Pranav, thank you for taking the bug! I am looking forward to review your patch!

(I apologize I didn't see your messages earlier. I was not CCed to the bug. :( )

Comment 6

[Pranav Ravichandran [:pranavrc]]

Attachment: Proposed patch

Apologies for the delay.

Comment 7

[Pranav Ravichandran [:pranavrc]]

Ouch, I just noticed I might have missed an underscore in the function names.

Comment 8

[Mihai Sucan [:msucan]]

Comment on attachment 599082 [details] [diff] [review]
Proposed patch

Review of attachment 599082 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for your patch Pranav! This works as we discussed and I am glad to see this piece of awesomeness.

General comments:

1. In doComment()/doUncomment() please check the current editor mode. For text mode both functions should do nothing, return false. Please make sure that when they execute you return true, in both functions.

2. Also, based on the current mode please do not add // style of comments to CSS (you can test this in the style editor). This type of comments are not allowed in CSS. When there's no selection you can wrap the whole line in /* */

3. Similarly, in doUncomment() please do not uncomment /// style of comments in CSS mode because they don't exist.

4. In HTML mode please use <!-- and -->.

5. Please do follow the coding style of the file, do put curly braces on the same line:
   if (foo) {
     bar();
   } else {
     baz();
   }

... but note that methods are styled like you did, so you don't have to change this, only the if/whiles.

6. When you select a bunch of lines that each have // at the start and you press Ctrl-Shift-M you would expect that it detects those lines are individually commented out, and that the shortcut will uncomment them. So, please add the following ability: if the selection does not have any open and close comment symbols, then loop through each line and apply the same logic as for "uncomment a single line" (find the comment symbol, check if there's only whitespace between line start and the symbol, if yes, remove the comment symbol).

More comments below.

Thanks for your awesome work! I am looking forward for the updated patch!


(please pull again from mozilla-central and rebase your patch - it no longer applies cleanly because I just landed a number of source editor changes :) )

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +901,5 @@
> +   * Inserts a Block Comment around selection.
> +   * Inserts a Single Line Comment in the line of the current Caret Offset, when
> +   * there is no selection.
> +   */
> +  _doComment: function SE_doComment()

Please mark the method as @private in the comment. Also please rename to SE__doComment() (notice the two _ chars).

(same for _doUncomment())

@@ +913,5 @@
> +    {
> +      let commentStart = "//";
> +      let commentEnd = "";
> +      this.setText(commentStart, commentStartOffset, commentStartOffset);
> +      this.setText(commentEnd, commentEndOffset + commentStart.length, commentEndOffset + commentStart.length);

Is this line of code needed? As I understand, it does nothing. Please explain, maybe I am missing something.

@@ +920,5 @@
> +    {
> +      let commentStart = "/*";
> +      let commentEnd = "*/";
> +      this.setText(commentStart, selection.start, selection.start);
> +      this.setText(commentEnd, selection.end + commentStart.length, selection.end + commentStart.length);

This is great!

Please call this.startCompoundChange() before the first setText() then endCompoundChange() after the second setText() so you get one single undo/redo step, not two.

@@ +952,5 @@
> +          this.setText("", caret, caret + commentSymbol.length);
> +          break;
> +        }
> +        caret = caret + 1;
> +      }

You can make something more efficient instead of this while loop. You can do model.getLine(selectionStartLine) then indexOf(commentSymbol) and remove that. Also, use a regex to make sure there's only whitespace from the line start until the comment symbol.

@@ +985,5 @@
> +          commentClosed = true;
> +          break;
> +        }
> +        lastLineCaret = lastLineCaret + 1;
> +      }

Similarly to the above approach: you can replace the while loops with model.getLine(selectionStartLine) then find openCommentSymbol, but here you do not need to check if whitespace. Then do the same for the last line.

I like that you check if you found the open and close comment symbols, then you remove them.

@@ +986,5 @@
> +          break;
> +        }
> +        lastLineCaret = lastLineCaret + 1;
> +      }
> +      if ((commentOpened == true) && (commentClosed == true))

if (commentOpened && commentClosed) {

Comment 9

[Pranav Ravichandran [:pranavrc]]

Thanks a lot for the review Mihai! I'll start making the necessary changes then.

Also, https://bugzilla.mozilla.org/show_bug.cgi?id=719453 - if I'm not wrong, I should be writing a wrapper function for each of getLineStart/getLineEnd methods in source-editor-orion.jsm (like in SE__getText() and SE__setText()) and then update the patch to use these wrapper functions, right?

Comment 10

[Mihai Sucan [:msucan]]

(In reply to Pranav Ravichandran [:pranavrc] from comment #9)
> Thanks a lot for the review Mihai! I'll start making the necessary changes
> then.
> 
> Also, https://bugzilla.mozilla.org/show_bug.cgi?id=719453 - if I'm not
> wrong, I should be writing a wrapper function for each of
> getLineStart/getLineEnd methods in source-editor-orion.jsm (like in
> SE__getText() and SE__setText()) and then update the patch to use these
> wrapper functions, right?

That's fine either way. You can base this patch on top of that if you want, but it's not needed - to keep things simpler for you.

Comment 11

[Pranav Ravichandran [:pranavrc]]

Attachment: Proposed patch v2

Looking forward to the review :)

Comment 12

[Mihai Sucan [:msucan]]

Comment on attachment 600390 [details] [diff] [review]
Proposed patch v2

Review of attachment 600390 [details] [diff] [review]:
-----------------------------------------------------------------

This is a quite comprehensive patch Pranav! Great work! Thanks!

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +853,5 @@
> +    let blockCommentStart = "";
> +    let blockCommentEnd = "";
> +    let currentMode = this.getMode();
> +
> +    switch (currentMode) {

Maybe you can put this in a separate helper function which you can call from _doComment and _doUncomment. What do you think?

@@ +880,5 @@
> +
> +    if (selection.start == selection.end) {
> +      this.startCompoundChange();
> +      this.setText(singleLineCommentStart, commentStartOffset, commentStartOffset);
> +      this.setText(singleLineCommentEnd, commentEndOffset + singleLineCommentStart.length, commentEndOffset + singleLineCommentStart.length);

Don't do the second setText() call if singleLineCommentEnd is empty. Don't even call start/endCompoundChange().

@@ +944,5 @@
> +      let selectionFirstLineStart = this.getLineStart(selectionStartLine);
> +      let selectionLastLineStart = this.getLineStart(selectionEndLine);
> +      let firstLineCaret = selectionFirstLineStart + openOffset;
> +      let lastLineCaret = selectionLastLineStart + closeOffset;
> +      if ((openOffset != -1) && (closeOffset != -1)) {

nit: you don't need to wrap the two conditions in parentheses.

@@ +964,5 @@
> +      let openOffset = currentLine.indexOf(singleLineCommentStart);
> +      let openCaret = selectionLineStart + openOffset;
> +      let closeOffset = 0;
> +      let closeCaret = 0;
> +      if (singleLineCommentEnd != "") {

This is the "remove single line comments" loop, so I think you shouldn't support removing a singleLineCommentEnd. In CSS and HTML /* and */ are for both multiple lines and single lines. I doubt we'll end up with /* foo1 */ \n /* foo2 */ ... each on a line.

@@ +970,5 @@
> +        closeCaret = selectionLineStart + closeOffset;
> +      } else if (selection.start != selection.end) {
> +        let textUntilComment = this.getText(selectionLineStart, openCaret + singleLineCommentStart.length);
> +        let commentPattern = new RegExp("^(\\s*)(\\/)(\\/)$");
> +        if (!commentPattern.test(textUntilComment)) {

You do the regex test only if selection.start != selection.end. Why not do it always?

You don't want to remove single line comments from the end of the line - only those at the start of the line (allowing, of course, only whitespace).

Also, you don't need to include the comment in the regex, nor in the getText() call.

Lastly, you don't need to do new RegExp(). You can directly do:
if (/^[^\s]+/.test(textUntilComment)) {
  selectionStartIne++;
  continue;
}

@@ +977,5 @@
> +        }
> +      }
> +
> +      if ((openOffset != -1) && (closeOffset != -1)) {
> +        this.startCompoundChange();   

Move the call to startCompoundChange() before the loop and endCompoundChange() after the loop.

@@ +984,5 @@
> +          this.setText("", closeCaret - singleLineCommentStart.length, closeCaret + singleLineCommentEnd.length - singleLineCommentStart.length);
> +        }
> +        this.endCompoundChange();
> +      } 
> +      selectionStartLine = selectionStartLine + 1;

selectionStartLine++;

@@ +1169,5 @@
> +    return this._model.getLineStart(aLineIndex)
> +  },
> +
> +  /**
> +   * Get the end character offset of the line with index aLineIndex, excluding the end offset. When the line delimiter is present, the offset is the start offset of the next line or the char count. Otherwise, it is the offset of the line delimiter.

Please do word wrapping here.

Comment 13

[Pranav Ravichandran [:pranavrc]]

Thanks for the feedback, Mihai!
 
> Maybe you can put this in a separate helper function which you can call from
> _doComment and _doUncomment. What do you think?

Yeah, I thought about it when I had to use the same switch statement twice, but for some reason, I wasn't sure if it was allowed here. Now that you say it, I think it's the sensible thing to do.

> You do the regex test only if selection.start != selection.end. Why not do
> it always?
> 
> You don't want to remove single line comments from the end of the line -
> only those at the start of the line (allowing, of course, only whitespace).

I was considering single-line comments like 'foo //bar' and I thought removing them with the shortcut must be allowed. But I guess that's inconsistent with the commenting behavior, so I'll fix that.

Also, should I rebase the updated patch?

Comment 14

[Pranav Ravichandran [:pranavrc]]

> This is the "remove single line comments" loop, so I think you shouldn't
> support removing a singleLineCommentEnd. In CSS and HTML /* and */ are for
> both multiple lines and single lines. I doubt we'll end up with /* foo1 */
> \n /* foo2 */ ... each on a line.

I don't quite understand this. What about in HTML or CSS mode where I need to remove a single-line comment like:

<!-- foo --> 
/* foo */

when there's no selection?

Comment 15

[Mihai Sucan [:msucan]]

(In reply to Pranav Ravichandran [:pranavrc] from comment #13)
> Thanks for the feedback, Mihai!

You're welcome!

> Also, should I rebase the updated patch?

Yes, please do!

(In reply to Pranav Ravichandran [:pranavrc] from comment #14)
> > This is the "remove single line comments" loop, so I think you shouldn't
> > support removing a singleLineCommentEnd. In CSS and HTML /* and */ are for
> > both multiple lines and single lines. I doubt we'll end up with /* foo1 */
> > \n /* foo2 */ ... each on a line.
> 
> I don't quite understand this. What about in HTML or CSS mode where I need
> to remove a single-line comment like:
> 
> <!-- foo --> 
> /* foo */
> 
> when there's no selection?

You need to support removing comments of /* foo */ style when there's no selection, or when there's a selection only on one line, but not when it spans through many lines of each /* line 1 */\n/* line 2 */\n/* line 3 */ etc. That's what I meant. Sorry I was confusing.

Reasoning: when you select multiple lines and comment them you get /* all lines */ not a round of comments /* for each line */. So, supporting that case for uncomment is kinda weird. Does it make sense?

Comment 16

[Pranav Ravichandran [:pranavrc]]

> You need to support removing comments of /* foo */ style when there's no
> selection, or when there's a selection only on one line, but not when it
> spans through many lines of each /* line 1 */\n/* line 2 */\n/* line 3 */
> etc. That's what I meant. Sorry I was confusing.
> 
> Reasoning: when you select multiple lines and comment them you get /* all
> lines */ not a round of comments /* for each line */. So, supporting that
> case for uncomment is kinda weird. Does it make sense?

Thanks for the clarification! 

/* foo */
/* line1 */
/* line2 */
/* bar */

So, if I've understood right, when a selection is made from the first to the last line in the above case, the first /* and the last */ must not be removed since there are other comment blocks in the middle, and it also leaves the closing and opening comment symbols in the first and last line hanging. Is that it?

Comment 17

[Mihai Sucan [:msucan]]

Hm, this is becoming confusing. I was merely suggesting you only do searches for singleLineCommentStart symbols on every line and remove those, for the JS mode. Done. Nothing else. No singleLineComment removals in CSS and HTML modes, because those always use comment blocks.

Comment 18

[Pranav Ravichandran [:pranavrc]]

Attachment: Proposed patch v3

Comment 19

[Mihai Sucan [:msucan]]

Comment on attachment 600684 [details] [diff] [review]
Proposed patch v3

Review of attachment 600684 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for your patch update Pranav! Really good work.

Since I am new to your code I find some things confusing, so I have some comments to, hopefully, make it less confusing and easier to read.

Also, please remove any trailing whitespaces in the patch. It currently has trailing whitespaces that we need to remove before landing.

Looking forward for the updated patch. Thank you!

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +834,5 @@
>      return this._iframe;
>    },
>  
>    /**
> +   * Helper function to return comment symbols based on mode.

Helper function to retrieve the strings used for comments in the current editor mode.

@@ +837,5 @@
>    /**
> +   * Helper function to return comment symbols based on mode.
> +   *
> +   * @private 
> +   * @return Array

array, lower case

@@ +840,5 @@
> +   * @private 
> +   * @return Array
> +   *         Array of Comment Symbols.
> +   */
> +  _doAssignCommentSymbols: function SE__doAssignCommentSymbols()   

Would _getCommentStrings() be better?

@@ +871,5 @@
> +        aBlockCommentStart = "<!--";
> +        aBlockCommentEnd = "-->";
> +        break;
> +    }
> +    return [aSingleLineCommentStart, aSingleLineCommentEnd, aBlockCommentStart, aBlockCommentEnd];

I'd like this simplified and made clearer, I'm kind of confused.

Can you please do the following? Return an object with three properties: line, blockStart, blockEnd. You don't need to provide the string for commenting lines for CSS and for HTML, because they don't have the concept of line comments, let it empty.

Also, please update the function description to explain the return value in mode detail (tell which properties are returned and what they represent).

@@ +887,5 @@
> +    let selection = this.getSelection();
> +    let selectionStartLine = this._model.getLineAtOffset(selection.start);
> +    let selectionEndLine = this._model.getLineAtOffset(selection.end);
> +    let commentStartOffset = this.getLineStart(selectionStartLine);
> +    let commentEndOffset = this.getLineEnd(selectionEndLine)  

commentStartOffset and commentEndOffset are used only when selection.start == selection.end, so please put them there.

Also, selectionStartLine and selectionEndLine are then equal, which means you can use only one of them.

Lastly, that means the two variables could be renamed to lineStartOffset and lineEndOffset. The current commentStart/EndOffset names are confusing for me.

@@ +889,5 @@
> +    let selectionEndLine = this._model.getLineAtOffset(selection.end);
> +    let commentStartOffset = this.getLineStart(selectionStartLine);
> +    let commentEndOffset = this.getLineEnd(selectionEndLine)  
> +    let singleLineCommentStart, singleLineCommentEnd, blockCommentStart, blockCommentEnd;
> +    [singleLineCommentStart, singleLineCommentEnd, blockCommentStart, blockCommentEnd] = this._doAssignCommentSymbols();

You don't need to do let foo1, foo2,...;

You can directly do:
let [foo1, foo2] = foo();

But you'll switch using an object, so you'll no longer need this.

@@ +895,5 @@
> +    if (selection.start == selection.end) {
> +      if (singleLineCommentEnd == "") {
> +        this.setText(singleLineCommentStart, commentStartOffset, commentStartOffset);
> +      } else {
> +      this.startCompoundChange();

This block of code needs to be correctly indented.

@@ +928,5 @@
> +    [singleLineCommentStart, singleLineCommentEnd, blockCommentStart, blockCommentEnd] = this._doAssignCommentSymbols();
> +
> +    if (selection.start != selection.end) { 
> +      let firstLine = this._model.getLine(selectionStartLine);
> +      let lastLine = this._model.getLine(selectionEndLine);

firstLineText and lastLineText.

@@ +930,5 @@
> +    if (selection.start != selection.end) { 
> +      let firstLine = this._model.getLine(selectionStartLine);
> +      let lastLine = this._model.getLine(selectionEndLine);
> +      let openOffset = firstLine.indexOf(blockCommentStart);
> +      let closeOffset = lastLine.indexOf(blockCommentEnd);

Here you want lastLineText.lastIndexOf(blockCommentEnd).

Also rename openOffset to openIndex and closeOffset to closeIndex.

@@ +932,5 @@
> +      let lastLine = this._model.getLine(selectionEndLine);
> +      let openOffset = firstLine.indexOf(blockCommentStart);
> +      let closeOffset = lastLine.indexOf(blockCommentEnd);
> +      let selectionFirstLineStart = this.getLineStart(selectionStartLine);
> +      let selectionLastLineStart = this.getLineStart(selectionEndLine);

This code is hard to follow for me. Can you please rename variable the two variables to: firstLineStartOffset and lastLineStartOffset.

Similarly, please rename selectionStartLine to firstLine and lastLine.

@@ +934,5 @@
> +      let closeOffset = lastLine.indexOf(blockCommentEnd);
> +      let selectionFirstLineStart = this.getLineStart(selectionStartLine);
> +      let selectionLastLineStart = this.getLineStart(selectionEndLine);
> +      let firstLineCaret = selectionFirstLineStart + openOffset;
> +      let lastLineCaret = selectionLastLineStart + closeOffset;

To make things less confusing go for a rename: openOffset and closeOffset.

Why: your current openOffset/closeOffset variables do not hold actual orion offsets, but they hold line-relative indexes. That's confusing. firstLineCaret and lastLineCaret hold actual offset for comment open and close.

@@ +940,5 @@
> +        this.startCompoundChange();
> +        this.setText("", firstLineCaret, firstLineCaret + blockCommentStart.length);
> +        this.setText("", lastLineCaret - blockCommentStart.length, lastLineCaret + blockCommentEnd.length - blockCommentStart.length);
> +        this.endCompoundChange();
> +        blockCommentRemoved = true;

You can just return true; here.

Comment 20

[Pranav Ravichandran [:pranavrc]]

Attachment: Proposed patch v4

Thanks for the feedback, Mihai!

And sorry for the delay in the patch submission. Some RL issues. 

I think I've made the feedback changes in the patch, but here and there, I've added stuff to wipe out hackishness. Just saying because the patch might not be exactly the same as the feedback required :)

And thanks again for your patience!

Comment 21

[Mihai Sucan [:msucan]]

Comment on attachment 601663 [details] [diff] [review]
Proposed patch v4

Review of attachment 601663 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the update! This is better but you still have trailing white space and there's some confusion. When I recommended that _getCommentStrings() returns an object with blockStart,blockEnd and line i meant that "line" holds the comment string for single-line comments (for example in JS that's "//").

If you do that, you don't need to return .mode in the comment object (please don't).

Some lines of code are wrongly indented - please do not use tabs for indentation, only use spaces.

Lastly, the while loop in _doUncomment() still tries to uncomment block comments for every line. I asked this in comment 12 and I tried to better explain in subsequent comments.

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +1003,5 @@
> +      if (commentObject.mode == SourceEditor.MODES.JAVASCRIPT) {
> +      	openIndex = currentLine.indexOf(singleLineCommentStart);
> +	      openOffset = selectionLineStart + openIndex;
> +        let textUntilComment = this.getText(selectionLineStart, openOffset);
> +       	if (!(RegExp("^(\\s*)$").test(textUntilComment))) {

I asked in comment 12 that you change this to: if (/^[^\s]+/.test(textUntilComment)) { ... }.

Comment 22

[Pranav Ravichandran [:pranavrc]]

Attachment: Proposed patch v5

Comment 23

[Mihai Sucan [:msucan]]

Comment on attachment 601997 [details] [diff] [review]
Proposed patch v5

Review of attachment 601997 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks a lot for your updated patch Pranav! This is great!

More comments below...

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +897,5 @@
> +   * @private
> +   * @return object
> +   *         An object that holds four properties:
> +   *         - mode: the current editor mode.
> +   *         - line: the line number, counting from 0.

Please update the comment to reflect the latest changes (mode has been removed and "line" holds the string for line-only comments).

Also, mention the function returns null for unknown/unsupported editor modes.

@@ +912,5 @@
> +    let currentMode = this.getMode();
> +
> +    switch (currentMode) {
> +      case SourceEditor.MODES.TEXT:
> +        return false;

instead of specifically listing modes.TEXT please include default: return null. So for unknown/unsupported modes we return null.

@@ +947,5 @@
> +
> +    if (selection.start == selection.end) {
> +      let lineStartOffset = this.getLineStart(selectionLine);
> +      let lineEndOffset = this.getLineEnd(selectionLine);
> +      if (commentObject.line == "//") {

Just do if (commentObject.line) { ... } else { ... }

@@ +1031,5 @@
> +          }
> +        } else if (commentObject.line == "//") {
> +          this.setText("", openOffset, openOffset + commentObject.line.length);
> +        }
> +      }

You can simplify the while loop code a lot:

... before startCompoundChange, before the while loop
if (!commentObject.line) {
  return true;
}
(this is because the loop applies only when the current mode has one-line style of comments)
...
while(...) {
...
directly put the code that checks for the presence of commentObject.line in currentLine and the code that removes it from each line.
remove all the code that deals with blockStart/blockEnd, etc.
...
}

Comment 24

[Pranav Ravichandran [:pranavrc]]

Attachment: Proposed patch v6

Comment 25

[Mihai Sucan [:msucan]]

Comment on attachment 603283 [details] [diff] [review]
Proposed patch v6

Review of attachment 603283 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Pranav for your patch update! Great work!

This is now ready for a test. Please write a test. Please read the following link to learn how to write and run tests:

https://developer.mozilla.org/en/Browser_chrome_tests

The source editor tests are in browser/devtools/sourceeditor/test. Copy one of the existing tests into a new test file. Edit Makefile.in and add your new test file in the list you see there. Then to hg add path/to/the/new/test/file. Edit the test so it tests the new code you added. Then run make to update the build, then run the source editor tests (see the above link on how to do this). When you have a working test, please update this patch - do hg diff to submit a patch that includes the source editor changes bundled together with the new test file.

Thank you again! Cool stuff!

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +943,5 @@
> +   */
> +  _doComment: function SE__doComment()
> +  {
> +    let selection = this.getSelection();
> +    let commentObject = this._getCommentStrings();

Please return false when commentObject is null.

Also, please return false when the editor is readonly (check with this.readOnly). This code needs to prevent making changes in read only mode.

(please do the same for doComment and doUncomment)

Comment 26

[Rob Campbell [:rc] (:robcee)]

I was just talking to Mihai about this and Cmd-M on the Mac is not going to work for Commenting. That's a global window command to Minimize the current Window.

I don't have a great suggestion for an alternate that doesn't include punctuation. I will ask my standard question about adding features to the editor: Is it so hard to go to the beginning of the section and adding a /* followed by a */ at the end of the section you want to comment?

Do we really need a dedicated keyboard shortcut?

Comment 27

[Mihai Sucan [:msucan]]

Hey Rob!


(In reply to Rob Campbell [:rc] (robcee) from comment #26)
> I was just talking to Mihai about this and Cmd-M on the Mac is not going to
> work for Commenting. That's a global window command to Minimize the current
> Window.
> 
> I don't have a great suggestion for an alternate that doesn't include
> punctuation. I will ask my standard question about adding features to the
> editor: Is it so hard to go to the beginning of the section and adding a /*
> followed by a */ at the end of the section you want to comment?
> 
> Do we really need a dedicated keyboard shortcut?

This is a fairly common feature in source editors that I believe we should include. It is fairly common to have to comment/uncomment lines of code for testing purposes and other use-cases.

Comment 28

[Johan C]

I return to my initial suggestion of using ctrl-Q as well as ctrl-shift-Q.

Comment 29

[Mihai Sucan [:msucan]]

(In reply to Johan Charlez from comment #28)
> I return to my initial suggestion of using ctrl-Q as well as ctrl-shift-Q.

Ctrl-Q is common for Quit, we can't use it.

Comment 30

[Panos Astithas (he/him) [:past] (please ni?)]

Ctrl-/ is my personal favorite, which is used in eclipse and others.

Comment 31

[Johan C]

Ctrl-/ is a problem due to l10n, on my keyboard I have to press shift-7 to print '/'. [modifier]-/ does not work.

Comment 32

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Johan Charlez from comment #31)
> Ctrl-/ is a problem due to l10n, on my keyboard I have to press shift-7 to
> print '/'. [modifier]-/ does not work.

That's unfortunate, but isn't this the same as with the Ctrl-+ shortcut for zoom? In my keyboard Ctrl-plus is actually Ctrl-Shift-+, because that's how you get to the plus character. And that's in a standard English keyboard, i.e. the default locale.

Comment 33

[Pranav Ravichandran [:pranavrc]]

Attachment: Proposed patch v7 with test

Comment 34

[Mihai Sucan [:msucan]]

Comment on attachment 605712 [details] [diff] [review]
Proposed patch v7 with test

Review of attachment 605712 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for your patch update Pranav! I see you had quite some rebasing to do.

The test is awesome! I only have some comments on how to improve it.

Please also test in readonly mode: set editor.readOnly = true then try Ctrl-M and Ctrl-Shift-M to see if any changes are made.

(you can ask for a review from me, now that you have a test!)

::: browser/devtools/sourceeditor/test/browser_bug725430_comment_uncomment.js
@@ +11,5 @@
> +  let SourceEditor = temp.SourceEditor;
> +
> +  let component = Services.prefs.getCharPref(SourceEditor.PREFS.COMPONENT);
> +  if (component == "textarea") {
> +    ok(true, "skip test for bug 712982: only applicable for non-textarea components");

This check is no longer needed. The textarea fallback component has been removed.

@@ +21,5 @@
> +  let editor;
> +
> +  const windowUrl = "data:text/xml,<?xml version='1.0'?>" +
> +    "<window xmlns='http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul'" +
> +    " title='test for bug 712982' width='600' height='500'><hbox flex='1'/></window>";

Please update the bug number.

@@ +47,5 @@
> +    editor.setText(text)
> +
> +    editor.startCompoundChange();
> +    editor.setCaretPosition(0);
> +    EventUtils.synthesizeKey("VK_M", {ctrlKey: true}, testWin);

This is going to fail on Macs. Please use accelKey, as you did in source-editor-orion.jsm.

@@ +56,5 @@
> +    editor.undo();
> +
> +    editor.startCompoundChange();
> +    EventUtils.synthesizeKey("VK_A", {ctrlKey: true}, testWin);
> +    editor.getSelectedText();

Is this needed? You have this in multiple places and I am not sure if I understand why.

@@ +60,5 @@
> +    editor.getSelectedText();
> +    EventUtils.synthesizeKey("VK_M", {ctrlKey: true}, testWin);
> +    is(editor.getText(), "/*" + text + "*/", "Block Commenting in Javascript Mode works");
> +    EventUtils.synthesizeKey("VK_A", {ctrlKey: true}, testWin);
> +    EventUtils.synthesizeKey("VK_M", {ctrlKey: true, shiftKey: true}, testWin);

After you comment the selection do an undo() and check if the content is correct. Then redo() to check if the content is correct again. This is to check if the changes did by the doComment/doUncomment methods were grouped into one.

Do the same after you test Ctrl-Shift-M.

Do these undo/redo checks for multi-line comment/uncomment as well. You don't need to do these for each mode, it's sufficient to only do the undo()/redo() checks for the first mode (JS).

@@ +87,5 @@
> +    is(editor.getText(), text, "Uncommenting without a selection in CSS Mode works");
> +    editor.endCompoundChange();
> +    editor.undo();
> +
> +    editor.setText(text);

You group all changes into compound changes so you can return to the initial text, but then you do setText(). In such cases you don't really need to start compound changes, nor undo at the end. You can just overwrite everything with setText(), like you do.

Comment 35

[Pranav Ravichandran [:pranavrc]]

Attachment: Proposed patch v8 with test

Comment 36

[Mihai Sucan [:msucan]]

Comment on attachment 606276 [details] [diff] [review]
Proposed patch v8 with test

Review of attachment 606276 [details] [diff] [review]:
-----------------------------------------------------------------

Really good work Pranav! Thanks!

One general nit: the test has lines that go a lot beyond the 80 chars limit. Please adjust the code to only have 80 chars per line.

R+ for the patch, with these comments addressed.

::: browser/devtools/sourceeditor/test/browser_bug725430_comment_uncomment.js
@@ +9,5 @@
> +  let temp = {};
> +  Cu.import("resource:///modules/source-editor.jsm", temp);
> +  let SourceEditor = temp.SourceEditor;
> +
> +  let component = Services.prefs.getCharPref(SourceEditor.PREFS.COMPONENT);

Do you use this?

@@ +134,5 @@
> +
> +    editor.setCaretPosition(0);
> +    EventUtils.synthesizeKey("VK_M", {accelKey: true}, testWin);
> +    is(editor.getText(), text, "Commenting disabled in ReadOnly mode");
> +    let altText = "//" + text;

Did you mean to do setText("//" + text) here? Or?

Comment 37

[Pranav Ravichandran [:pranavrc]]

Attachment: Proposed patch v9 with test

A couple of lines slightly exceed the 80 char limit (for consistency). I hope that's not a problem.

Comment 38

[Mihai Sucan [:msucan]]

Comment on attachment 606431 [details] [diff] [review]
Proposed patch v9 with test

Review of attachment 606431 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for your patch and patience through the review! Good work! Much appreciated.

I am going to push the patch to the try server, to see if all tests pass there. Then I am going to land it (soon!).

Comment 39

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] updated patch

Updated the patch with changed keyboard shortcut. Ctrl+/ for Windows and Linux, Cmd+/ for Macs. Rationale: while it is true that slash may not always be available on every keyboard layout, in every language, it is also safe to assume that developers use keyboard layouts that do have this key - writing file paths and doing many other dev-related work require having slash in the layout. Developer tools are already treated "special" in terms of localization: localization is optional. Another reason for picking this keyboard shortcut: even in the cases where this key is not available on the user's keyboard, this feature is not essential in any way to users, it's only a convenience shortcut. Beyond that, the benefit of not taking existing letters exceeds the problems with picking a punctuation-based shortcut.

I'm going to land this patch tomorrow if no objections are made by then. Try results are green.

https://tbpl.mozilla.org/?tree=Try&rev=3c816daa3c5a

Comment 40

[Mihai Sucan [:msucan]]

Comment on attachment 606689 [details] [diff] [review]
[in-fx-team] updated patch

Landed:
https://hg.mozilla.org/integration/fx-team/rev/9b911534deca

Comment 41

[Pranav Ravichandran [:pranavrc]]

Thanks Mihai!

Comment 42

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/9b911534deca

Comment 43

[Cedric Vivier [:cedricv]]

(In reply to Mihai Sucan [:msucan] from comment #39)
> Created attachment 606689 [details] [diff] [review]
> [in-fx-team] updated patch
> 
> Updated the patch with changed keyboard shortcut. Ctrl+/ for Windows and
> Linux, Cmd+/ for Macs. Rationale: while it is true that slash may not always
> be available on every keyboard layout, in every language, it is also safe to
> assume that developers use keyboard layouts that do have this key

I agree with this assumption, however on some keyboard layouts (eg. azerty), slash might be available when pressing Shift (on ":" key - like "?" is Shift "/" key on qwerty).

Wouldn't this be badly conflict with Un-comment key binding?
Similarly, do we really need two key bindings to comment AND uncomment?
We could perhaps use only one that toggles (comment line/selection is not already commented, uncomment otherwise [checking for // or /* */ at selection or line boundaries)

(we can open a follow up depending outcome of this discussion)

Comment 44

[Mihai Sucan [:msucan]]

(In reply to Cedric Vivier [:cedricv] from comment #43)
> (In reply to Mihai Sucan [:msucan] from comment #39)
> > Created attachment 606689 [details] [diff] [review]
> > [in-fx-team] updated patch
> > 
> > Updated the patch with changed keyboard shortcut. Ctrl+/ for Windows and
> > Linux, Cmd+/ for Macs. Rationale: while it is true that slash may not always
> > be available on every keyboard layout, in every language, it is also safe to
> > assume that developers use keyboard layouts that do have this key
> 
> I agree with this assumption, however on some keyboard layouts (eg. azerty),
> slash might be available when pressing Shift (on ":" key - like "?" is Shift
> "/" key on qwerty).
> 
> Wouldn't this be badly conflict with Un-comment key binding?
> Similarly, do we really need two key bindings to comment AND uncomment?
> We could perhaps use only one that toggles (comment line/selection is not
> already commented, uncomment otherwise [checking for // or /* */ at
> selection or line boundaries)
> 
> (we can open a follow up depending outcome of this discussion)

Sure! Sounds like a good suggestion. We can put this under a single keyboard shortcut. Please open a follow up!

Comment 45

[Mihai Sucan [:msucan]]

The new Source Editor API should be documented: getLineStart() and getLineEnd(). Also, the new shortcuts for comment/uncomment should be listed in the docs (ctrl-/ and ctrl-shift-/). Thank you!

Comment 46

[Eric Shepherd [:sheppy]]

These are documented:

https://developer.mozilla.org/en/Tools/Using_the_Source_Editor#Keyboard_commands

https://developer.mozilla.org/en/JavaScript_code_modules/source-editor.jsm#getLineEnd()
https://developer.mozilla.org/en/JavaScript_code_modules/source-editor.jsm#getLineStart

And listed on Firefox 14 for developers

Comment 47

[Markijan Firak]

comment-toggle is not i18n conform

As  Johan Charlez, Comment 31, already explained, comment-toggle does not work for him.

Many users in the world have a keyboard, where the '/'-key is only reachable with the 'shift'-modifier-key: canadian-french, french, german, italian, latin america and others.
Ref: http://en.wikipedia.org/wiki/Keyboard_layout

In IDEs like eclipse or jetbrains (and others) exist the same problem, but these l10n-IDEs can configure the applications-shortcut-keys.

Therefore the answer from Mihai Sucan, Comment 39, missed the point:
>> while it is true that slash may not always be available on every keyboard layout, in every language, it is also safe to assume that developers use keyboard layouts that do have this key - writing file paths and doing many other dev-related work require having slash in the layout.<<

Nearly all keyboard layouts have the the '/'-char, but they print the '/'-char with different key-strokes ('/' or  'shift'+'/'). This implies a different handling in the event-handler for key events (event.ctrlKey, event.shiftKey ..)

My suggestions from a developer pov:
- offer an alternate key-shortcut: Ctrl + NumpadDiv (/) (similar as firefox for zoom-in/out with Ctrl + NumpadAdd (+),  Ctrl + NumpadSub (-))
- offer an configuration via about:config (eg extensions.firebug.key.shortcut.toggleInspecting;accel shift c, extensions.firebug.key.shortcut.reenterCommand;control shift e, ..)

Comment 48

[Mihai Sucan [:msucan]]

Thank you Markijan! Can you please open a bug report?