Last Comment Bug 739192 - Source Editor should toggle comment on line or selection with one key binding
: Source Editor should toggle comment on line or selection with one key binding
Status: RESOLVED FIXED
[sourceeditor][fixed-in-fx-team]
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 14
Assigned To: Girish Sharma [:Optimizer]
:
Mentors:
Depends on:
Blocks: 729960
  Show dependency treegraph
 
Reported: 2012-03-26 05:41 PDT by Cedric Vivier [:cedricv]
Modified: 2012-05-04 12:50 PDT (History)
4 users (show)
khuey: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0 (3.64 KB, patch)
2012-03-28 03:24 PDT, Girish Sharma [:Optimizer]
no flags Details | Diff | Splinter Review
Patch v2.0 (4.01 KB, patch)
2012-03-30 06:33 PDT, Girish Sharma [:Optimizer]
mihai.sucan: feedback+
Details | Diff | Splinter Review
Patch v3.0 with test fix (11.18 KB, patch)
2012-04-02 07:35 PDT, Girish Sharma [:Optimizer]
no flags Details | Diff | Splinter Review
[in-fx-team] Patch v3.5 (11.17 KB, patch)
2012-04-04 10:07 PDT, Girish Sharma [:Optimizer]
mihai.sucan: review+
Details | Diff | Splinter Review

Description Cedric Vivier [:cedricv] 2012-03-26 05:41:12 PDT
Having both Ctrl-/ (to comment) and Ctrl-Shift-/ (to uncomment) seems unnecessary and has issues with keyboard layouts that requires Shift to type "/".

See https://bugzilla.mozilla.org/show_bug.cgi?id=725430#c43
Comment 1 Girish Sharma [:Optimizer] 2012-03-27 15:08:51 PDT
I can do this, please assign this to me.
Comment 2 Pranav Ravichandran [:pranavrc] 2012-03-27 19:11:00 PDT
I'm sorry I missed the mail on this. I'd like to do the additional work, but since Girish claimed it, so I guess he can take it :)

Just a question though:

Currently, selecting a bunch of lines in which one of them is a single-line comment and pressing ctrl+shift+/ uncomments that single line. Like in this:

foo
//bar
baz

When one key-binding is used, is this case going to be used for commenting the selected block, or uncommenting the single-line comment?
Comment 3 Girish Sharma [:Optimizer] 2012-03-27 22:25:55 PDT
(In reply to Pranav Ravichandran [:pranavrc] from comment #2)
> I'm sorry I missed the mail on this. I'd like to do the additional work, but
> since Girish claimed it, so I guess he can take it :)
> 
> Just a question though:
> 
> Currently, selecting a bunch of lines in which one of them is a single-line
> comment and pressing ctrl+shift+/ uncomments that single line. Like in this:
> 
> foo
> //bar
> baz
> 
> When one key-binding is used, is this case going to be used for commenting
> the selected block, or uncommenting the single-line comment?

If the user selected the whole bunch, and is aware that there is only one common shortcut, then he must have done that to comment the whole block only. Same is the behavior of everybody's favorite, Notepad++.
Comment 4 Mihai Sucan [:msucan] 2012-03-28 01:09:39 PDT
Thank you Girish!

Looking forward for your patch!
Comment 5 Mihai Sucan [:msucan] 2012-03-28 01:36:34 PDT
(In reply to Pranav Ravichandran [:pranavrc] from comment #2)
> I'm sorry I missed the mail on this. I'd like to do the additional work, but
> since Girish claimed it, so I guess he can take it :)
> 
> Just a question though:
> 
> Currently, selecting a bunch of lines in which one of them is a single-line
> comment and pressing ctrl+shift+/ uncomments that single line. Like in this:
> 
> foo
> //bar
> baz
> 
> When one key-binding is used, is this case going to be used for commenting
> the selected block, or uncommenting the single-line comment?

Good question! I think I agree with Girish on this: the whole block would be commented.
Comment 6 Girish Sharma [:Optimizer] 2012-03-28 03:24:50 PDT
Created attachment 610059 [details] [diff] [review]
Patch v1.0

Added the patch with the desired functionality.
Mihai, please tell me whether I need to change the test for the original bug after having a look at the patch
Comment 7 Mihai Sucan [:msucan] 2012-03-30 05:27:22 PDT
Comment on attachment 610059 [details] [diff] [review]
Patch v1.0

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

This looks good! Thanks for your patch Girish!

Comments follow...

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +1051,5 @@
> +  _doCommentUncomment: function SE__doCommentUncomment()
> +  {
> +    if (!this._doUncomment()) {
> +      this._doComment();
> +    }

This is a bit weird. Even if it duplicates a bit of code, I suggest you put the checks here: check if the selected text is a block comment or has per-line comments, similar to what doUncomment does. Then you don't even need any changes in doUncomment.

@@ +1148,5 @@
>  
> +    // If the selected text is not a block of comment, then uncomment each line
> +    // only if first and the last lines are commented.
> +    let firstLastCommented = true;
> +    let model = this._model;

Please add this at the top of the function and make use of it through out the entire function, or don't add it at all. Having it here just for the forEach() call is weird.

@@ +1149,5 @@
> +    // If the selected text is not a block of comment, then uncomment each line
> +    // only if first and the last lines are commented.
> +    let firstLastCommented = true;
> +    let model = this._model;
> +    [firstLine, lastLine].forEach(function(lineCaret) {

s/lineCaret/aLineIndex/

@@ +1153,5 @@
> +    [firstLine, lastLine].forEach(function(lineCaret) {
> +      let currentLine = model.getLine(lineCaret);
> +      let openIndex = currentLine.indexOf(commentObject.line);
> +      if (openIndex == -1)
> +        firstLastCommented = false;

We require the curly braces even for one liners like these.

Please also make sure that only whitespace is allowed before the commentObject.line string.

@@ +1154,5 @@
> +      let currentLine = model.getLine(lineCaret);
> +      let openIndex = currentLine.indexOf(commentObject.line);
> +      if (openIndex == -1)
> +        firstLastCommented = false;
> +    });

You should use .every() instead of forEach().

https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Array/every
Comment 8 Girish Sharma [:Optimizer] 2012-03-30 05:48:28 PDT
(In reply to Mihai Sucan [:msucan] from comment #7)
> ::: browser/devtools/sourceeditor/source-editor-orion.jsm
> @@ +1051,5 @@
> > +  _doCommentUncomment: function SE__doCommentUncomment()
> > +  {
> > +    if (!this._doUncomment()) {
> > +      this._doComment();
> > +    }
> 
> This is a bit weird. Even if it duplicates a bit of code, I suggest you put
> the checks here: check if the selected text is a block comment or has
> per-line comments, similar to what doUncomment does. Then you don't even
> need any changes in doUncomment.

So I should add the code from both doComment and doUncomment and modify it, and never use the _doUncomment or/and _doUncomment itself ?

> @@ +1148,5 @@
> >  
> > +    // If the selected text is not a block of comment, then uncomment each line
> > +    // only if first and the last lines are commented.
> > +    let firstLastCommented = true;
> > +    let model = this._model;
> 
> Please add this at the top of the function and make use of it through out
> the entire function, or don't add it at all. Having it here just for the
> forEach() call is weird.

Hmm. Will do.

> @@ +1149,5 @@
> > +    // If the selected text is not a block of comment, then uncomment each line
> > +    // only if first and the last lines are commented.
> > +    let firstLastCommented = true;
> > +    let model = this._model;
> > +    [firstLine, lastLine].forEach(function(lineCaret) {
> 
> s/lineCaret/aLineIndex/

Okay.

> @@ +1153,5 @@
> > +    [firstLine, lastLine].forEach(function(lineCaret) {
> > +      let currentLine = model.getLine(lineCaret);
> > +      let openIndex = currentLine.indexOf(commentObject.line);
> > +      if (openIndex == -1)
> > +        firstLastCommented = false;
> 
> We require the curly braces even for one liners like these.

Ahh, forgot.

> Please also make sure that only whitespace is allowed before the
> commentObject.line string.

okay.

> @@ +1154,5 @@
> > +      let currentLine = model.getLine(lineCaret);
> > +      let openIndex = currentLine.indexOf(commentObject.line);
> > +      if (openIndex == -1)
> > +        firstLastCommented = false;
> > +    });
> 
> You should use .every() instead of forEach().
> 
> https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Array/
> every

Hmm.
Comment 9 Mihai Sucan [:msucan] 2012-03-30 05:53:08 PDT
(In reply to Girish Sharma [:Optimizer] from comment #8)
> (In reply to Mihai Sucan [:msucan] from comment #7)
> > ::: browser/devtools/sourceeditor/source-editor-orion.jsm
> > @@ +1051,5 @@
> > > +  _doCommentUncomment: function SE__doCommentUncomment()
> > > +  {
> > > +    if (!this._doUncomment()) {
> > > +      this._doComment();
> > > +    }
> > 
> > This is a bit weird. Even if it duplicates a bit of code, I suggest you put
> > the checks here: check if the selected text is a block comment or has
> > per-line comments, similar to what doUncomment does. Then you don't even
> > need any changes in doUncomment.
> 
> So I should add the code from both doComment and doUncomment and modify it,
> and never use the _doUncomment or/and _doUncomment itself ?

I mean you copy only the code that checks if the selection has a block comment or line comments. If yes, then you call doUncomment(), otherwise you call doComment().
Comment 10 Girish Sharma [:Optimizer] 2012-03-30 05:56:52 PDT
(In reply to Mihai Sucan [:msucan] from comment #9)
> (In reply to Girish Sharma [:Optimizer] from comment #8)
> > (In reply to Mihai Sucan [:msucan] from comment #7)
> > > ::: browser/devtools/sourceeditor/source-editor-orion.jsm
> > > @@ +1051,5 @@
> > > > +  _doCommentUncomment: function SE__doCommentUncomment()
> > > > +  {
> > > > +    if (!this._doUncomment()) {
> > > > +      this._doComment();
> > > > +    }
> > > 
> > > This is a bit weird. Even if it duplicates a bit of code, I suggest you put
> > > the checks here: check if the selected text is a block comment or has
> > > per-line comments, similar to what doUncomment does. Then you don't even
> > > need any changes in doUncomment.
> > 
> > So I should add the code from both doComment and doUncomment and modify it,
> > and never use the _doUncomment or/and _doUncomment itself ?
> 
> I mean you copy only the code that checks if the selection has a block
> comment or line comments. If yes, then you call doUncomment(), otherwise you
> call doComment().

Okay .
Comment 11 Girish Sharma [:Optimizer] 2012-03-30 06:33:36 PDT
Created attachment 610866 [details] [diff] [review]
Patch v2.0

Previous review comments addressed.
Comment 12 Mihai Sucan [:msucan] 2012-04-02 05:48:31 PDT
Comment on attachment 610866 [details] [diff] [review]
Patch v2.0

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

This looks much better. Thank you! The direction is good - just please cleanup the code as suggested and you can go ahead and fix the browser_bug725430_comment_uncomment.js test file.

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +1070,5 @@
> +    let openIndex = firstLineText.indexOf(commentObject.blockStart);
> +    let closeIndex = lastLineText.lastIndexOf(commentObject.blockEnd);
> +    if (openIndex != -1 && closeIndex != -1) {
> +      this._doUncomment();
> +      return true;

Please do return this._doUncomment();

(the same for the other similar returns)

@@ +1084,5 @@
> +      let currentLine = model.getLine(aLineCaret);
> +      let lineStart = this.getLineStart(aLineCaret);
> +      let openIndex = currentLine.indexOf(commentObject.line);
> +      let openOffset = lineStart + openIndex;
> +      let textUntilComment = this.getText(lineStart, openOffset);

You already have the first and last line text from the checks you do above (for block comments). Do you really need getText()? You can read the text from the firstLineText and lastLineText directly.
Comment 13 Girish Sharma [:Optimizer] 2012-04-02 05:50:52 PDT
Fixing these 2 issues, is everything else okay ?
Should I make some changes in the tests ?
Comment 14 Mihai Sucan [:msucan] 2012-04-02 05:55:36 PDT
(In reply to Girish Sharma [:Optimizer] from comment #13)
> Fixing these 2 issues, is everything else okay ?

Should be, unless I find other issues at my next pass.

> Should I make some changes in the tests ?

Run the source editor tests and you will see failures. Please fix them.

(tip: always run the tests before you submit patches ;) )
Comment 15 Girish Sharma [:Optimizer] 2012-04-02 07:35:18 PDT
Created attachment 611453 [details] [diff] [review]
Patch v3.0 with test fix

I have addressed all the comments, also, I have made changes to the test so that it does not fails.

I also fixed a bug in _doUncomment() function . For the follosing case :

/*
line1
line2
//*/

and caret on last line, pressing the shortcut resulted in removal of '*/' with an error in error console .
Basically the check for both blockStart and blockEnd was true as in the string '/*/' both the indefOf() returned some non -1 number. Hence one more check was made to see if openIndex and closeIndex are separated by blockStart.length
Comment 16 Mihai Sucan [:msucan] 2012-04-04 09:02:31 PDT
Comment on attachment 611453 [details] [diff] [review]
Patch v3.0 with test fix

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

Thanks for the updated patch! This is cool stuff!

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +1068,5 @@
> +    let firstLineText = model.getLine(firstLine);
> +    let lastLineText = model.getLine(lastLine);
> +    let openIndex = firstLineText.indexOf(commentObject.blockStart);
> +    let closeIndex = lastLineText.lastIndexOf(commentObject.blockEnd);
> +    if (openIndex != -1 && closeIndex != -1) {

You need the same check as in doUncomment, right?

@@ +1088,5 @@
> +          return true;
> +        }
> +      }
> +      return false;
> +    }.bind(this));

Is bind(this) needed now?

@@ +1171,5 @@
>      let lastLineText = this._model.getLine(lastLine);
>      let openIndex = firstLineText.indexOf(commentObject.blockStart);
>      let closeIndex = lastLineText.lastIndexOf(commentObject.blockEnd);
> +    if (openIndex != -1 && closeIndex != -1
> +        && closeIndex - openIndex >= commentObject.blockStart.length) {

This change introduces a regression: if I select, say, 3 whole lines and I press Ctrl+/ it works, but when I try to uncomment the lines it doesn't work, because the close and open indexes are both at 0, but on different lines.

Code style nit: please put the && on the previous line and put the difference in parentheses.

::: browser/devtools/sourceeditor/test/browser_bug725430_comment_uncomment.js
@@ +73,5 @@
>      editor.setText(regText);
>      EventUtils.synthesizeKey("VK_A", {accelKey: true}, testWin);
> +    EventUtils.synthesizeKey("/", {accelKey: true}, testWin);
> +    is(editor.getText(), expText, "JS Multiple Line Uncommenting works"
> +                                  + "if first and last lines commented");

you are missing a space and this change is not really needed, actually.
Comment 17 Girish Sharma [:Optimizer] 2012-04-04 09:51:05 PDT
(In reply to Mihai Sucan [:msucan] from comment #16)
> Comment on attachment 611453 [details] [diff] [review]
> Patch v3.0 with test fix
> 
> Review of attachment 611453 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the updated patch! This is cool stuff!
> 
> ::: browser/devtools/sourceeditor/source-editor-orion.jsm
> @@ +1068,5 @@
> > +    let firstLineText = model.getLine(firstLine);
> > +    let lastLineText = model.getLine(lastLine);
> > +    let openIndex = firstLineText.indexOf(commentObject.blockStart);
> > +    let closeIndex = lastLineText.lastIndexOf(commentObject.blockEnd);
> > +    if (openIndex != -1 && closeIndex != -1) {
> 
> You need the same check as in doUncomment, right?

Yes. Will do.

> @@ +1088,5 @@
> > +          return true;
> > +        }
> > +      }
> > +      return false;
> > +    }.bind(this));
> 
> Is bind(this) needed now?

Ah, not anymore.
 
> @@ +1171,5 @@
> >      let lastLineText = this._model.getLine(lastLine);
> >      let openIndex = firstLineText.indexOf(commentObject.blockStart);
> >      let closeIndex = lastLineText.lastIndexOf(commentObject.blockEnd);
> > +    if (openIndex != -1 && closeIndex != -1
> > +        && closeIndex - openIndex >= commentObject.blockStart.length) {
> 
> This change introduces a regression: if I select, say, 3 whole lines and I
> press Ctrl+/ it works, but when I try to uncomment the lines it doesn't
> work, because the close and open indexes are both at 0, but on different
> lines.

Okay, I should change it to 
if (openIndex != -1 && closeIndex != -1 &&
   (firstLineOffset != lastLineOffset ||
   (closeIndex - openIndex) >= commentObject.blockStart.length)) {

> Code style nit: please put the && on the previous line and put the
> difference in parentheses.

Hmm.

> ::: browser/devtools/sourceeditor/test/browser_bug725430_comment_uncomment.js
> @@ +73,5 @@
> >      editor.setText(regText);
> >      EventUtils.synthesizeKey("VK_A", {accelKey: true}, testWin);
> > +    EventUtils.synthesizeKey("/", {accelKey: true}, testWin);
> > +    is(editor.getText(), expText, "JS Multiple Line Uncommenting works"
> > +                                  + "if first and last lines commented");
> 
> you are missing a space and this change is not really needed, actually.

Okay, I will revert it back.
Comment 18 Girish Sharma [:Optimizer] 2012-04-04 10:07:41 PDT
Created attachment 612242 [details] [diff] [review]
[in-fx-team] Patch v3.5

Comments addressed, maybe it should be pushed to try now.
Comment 19 Mihai Sucan [:msucan] 2012-04-05 04:56:09 PDT
Comment on attachment 612242 [details] [diff] [review]
[in-fx-team] Patch v3.5

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

Thank you! Really good work!
Comment 20 Mihai Sucan [:msucan] 2012-04-09 03:40:57 PDT
Comment on attachment 612242 [details] [diff] [review]
[in-fx-team] Patch v3.5

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

Thank you Girish!
Comment 21 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-09 14:56:03 PDT
https://hg.mozilla.org/mozilla-central/rev/cafe030aca8a
Comment 22 Eric Shepherd [:sheppy] 2012-05-04 12:50:07 PDT
Documented:

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

And mentioned on Firefox 14 for developers.

Note You need to log in before you can comment on or make changes to this bug.