New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Comment/Uncomment should ignore leading whitespace #1961

Closed
datguy opened this Issue Jul 10, 2013 · 8 comments

Comments

Projects
None yet
3 participants
@datguy
Contributor

datguy commented Jul 10, 2013

If I use Comment/Uncomment (on the Edit menu, or via hotkey) to comment a line, then AutoFormat the code, the comments are preceded with spaces. This prevents the Comment/Uncomment feature from recognizing the line as a comment, so I am unable to uncomment the line automatically.

@datguy

This comment has been minimized.

Show comment
Hide comment
@datguy

datguy Jul 12, 2013

Contributor

I'm looking at this, and I've completed the first part - it does a regex match for whitespace at the beginning of a line followed by two slashes (followed by anything). However, I noticed one bit of odd behavior in the existing code - maybe intentional, maybe not. If I select the following lines and attempt to comment/uncomment, it will see that some of the lines are not commented, and proceed to comment them all.

  //this is commented
  this; //is not

This doesn't seem like desirable behavior, but I wonder how it should behave. Should it toggle each line individually? That doesn't seem much better...

Contributor

datguy commented Jul 12, 2013

I'm looking at this, and I've completed the first part - it does a regex match for whitespace at the beginning of a line followed by two slashes (followed by anything). However, I noticed one bit of odd behavior in the existing code - maybe intentional, maybe not. If I select the following lines and attempt to comment/uncomment, it will see that some of the lines are not commented, and proceed to comment them all.

  //this is commented
  this; //is not

This doesn't seem like desirable behavior, but I wonder how it should behave. Should it toggle each line individually? That doesn't seem much better...

@dhruv13J

This comment has been minimized.

Show comment
Hide comment
@dhruv13J

dhruv13J Jul 12, 2013

Contributor

Can't the regex be a little smarter and use /../ style comments for multiple lines?

Contributor

dhruv13J commented Jul 12, 2013

Can't the regex be a little smarter and use /../ style comments for multiple lines?

@datguy

This comment has been minimized.

Show comment
Hide comment
@datguy

datguy Jul 12, 2013

Contributor

Interesting idea, but that would make the logic more complicated. Do you think that it should recognize multi-line comments only for uncommenting, or should it use the multi-line format for adding comments? I like the flexibility of commenting a block of lines at once then uncommenting individual lines, which you would not be able to do with the multi-line comment format.

Contributor

datguy commented Jul 12, 2013

Interesting idea, but that would make the logic more complicated. Do you think that it should recognize multi-line comments only for uncommenting, or should it use the multi-line format for adding comments? I like the flexibility of commenting a block of lines at once then uncommenting individual lines, which you would not be able to do with the multi-line comment format.

@dhruv13J

This comment has been minimized.

Show comment
Hide comment
@dhruv13J

dhruv13J Jul 12, 2013

Contributor

I agree, the single line comments are more flexible... But I feel that the commenting should be a lot smarter when comments are nested. For example, comments at the beginning of the line could mean code is commented, but comments after a bit of code is usually some sort of an explanation. Hence, the idea of multi-line comments.
Would it really be that difficult to make it smarter through the hot-keys? Like Ctrl + Shift + / could be a multiline toggle, while Ctrl + / would do the block comment thing.
I could help out with the coding, I know where to look ;-). However, I'm not used to Java GUI.

Contributor

dhruv13J commented Jul 12, 2013

I agree, the single line comments are more flexible... But I feel that the commenting should be a lot smarter when comments are nested. For example, comments at the beginning of the line could mean code is commented, but comments after a bit of code is usually some sort of an explanation. Hence, the idea of multi-line comments.
Would it really be that difficult to make it smarter through the hot-keys? Like Ctrl + Shift + / could be a multiline toggle, while Ctrl + / would do the block comment thing.
I could help out with the coding, I know where to look ;-). However, I'm not used to Java GUI.

@dhruv13J

This comment has been minimized.

Show comment
Hide comment
@dhruv13J

dhruv13J Jul 12, 2013

Contributor

Here's my idea for the logic (multiple lines) :
if(multiple lines selected): // look for '\n' through regex
....if( " /* " is present at the start):
........if( " / " is present at the end):
............Uncomment.
........else if ( " */ " is present anywhere else): //once again, regex magic!
............Remove the " */ " from there, and place it at the end
........else: //this means no " */ "
............Look beyond the selected text now for " */ ". If present, do nothing, else, just add " */ ".
....else: //this means no multiline comment was started
........Add /
.. */ to the beginning and the end.

Contributor

dhruv13J commented Jul 12, 2013

Here's my idea for the logic (multiple lines) :
if(multiple lines selected): // look for '\n' through regex
....if( " /* " is present at the start):
........if( " / " is present at the end):
............Uncomment.
........else if ( " */ " is present anywhere else): //once again, regex magic!
............Remove the " */ " from there, and place it at the end
........else: //this means no " */ "
............Look beyond the selected text now for " */ ". If present, do nothing, else, just add " */ ".
....else: //this means no multiline comment was started
........Add /
.. */ to the beginning and the end.

@datguy

This comment has been minimized.

Show comment
Hide comment
@datguy

datguy Jul 12, 2013

Contributor

One problem with the proposed logic is that we can't uncomment a multiline comment that had an embedded comment. Let's say I select the following lines and choose the new comment/uncomment.

 /* 
  * This is an existing multiline comment
  */
 void setup() {
  //TODO add more code
 println("done");  /* that's all folks */
 exit();
 }

Following is what the proposed logic would produce. Note that it is a valid comment, but it violates the convention that multiline comments start the middle lines of the comment with an asterisk (as shown in the previous code sample. Also, to uncomment it, you would need to somehow rebuild the embedded comments that had their trailing '*/' stripped.

 /* 
  * This is an existing multiline comment

 void setup() {
  //TODO add more code
 println("done");  /* that's all folks 
 exit();
 } */

A safer way to handle any selection that contains an embedded comment using the /* */ format is to revert to //, as follows. If the entire selection (excluding leading and trailing whitespace) starts with /* and ends with */, we could remove those characters as well as the leading asterisks on the middle lines.

 ///* 
 // * This is an existing multiline comment
 // */
 //void setup() {
 // //TODO add more code
 //println("done");  /* that's all folks */
 //exit();
 //} 

EDIT: clarified wording of the last two paragraphs.
EDIT2: fixed formatting

Contributor

datguy commented Jul 12, 2013

One problem with the proposed logic is that we can't uncomment a multiline comment that had an embedded comment. Let's say I select the following lines and choose the new comment/uncomment.

 /* 
  * This is an existing multiline comment
  */
 void setup() {
  //TODO add more code
 println("done");  /* that's all folks */
 exit();
 }

Following is what the proposed logic would produce. Note that it is a valid comment, but it violates the convention that multiline comments start the middle lines of the comment with an asterisk (as shown in the previous code sample. Also, to uncomment it, you would need to somehow rebuild the embedded comments that had their trailing '*/' stripped.

 /* 
  * This is an existing multiline comment

 void setup() {
  //TODO add more code
 println("done");  /* that's all folks 
 exit();
 } */

A safer way to handle any selection that contains an embedded comment using the /* */ format is to revert to //, as follows. If the entire selection (excluding leading and trailing whitespace) starts with /* and ends with */, we could remove those characters as well as the leading asterisks on the middle lines.

 ///* 
 // * This is an existing multiline comment
 // */
 //void setup() {
 // //TODO add more code
 //println("done");  /* that's all folks */
 //exit();
 //} 

EDIT: clarified wording of the last two paragraphs.
EDIT2: fixed formatting

@dhruv13J

This comment has been minimized.

Show comment
Hide comment
@dhruv13J

dhruv13J Jul 12, 2013

Contributor

Oops, totally missed that... sorry!

On Fri, Jul 12, 2013 at 4:47 PM, datguy notifications@github.com wrote:

One problem with the proposed logic is that we can't uncomment a multiline
comment that had an embedded comment. Let's say I select the following
lines and choose the new comment/uncomment.

/*

  • This is an existing multiline comment
    /
    void setup() {
    //TODO add more code
    println("done"); /
    that's all folks */
    exit();
    }

This is what the above logic would produce:

/*

  • This is an existing multiline comment

    void setup() {
    //TODO add more code
    println("done"); /* that's all folks
    exit();
    } */

To uncomment that, you would need to somehow rebuild the embedded comments
that had their trailing "/" stripped. A safer way to handle existing
comments that use the /
*/ format when they are embedded within the block
you are trying to comment is to revert to //, as follows:

///*
// * This is an existing multiline comment
// /
//void setup() {
// //TODO add more code
//println("done"); /
that's all folks */
//exit();
//}


Reply to this email directly or view it on GitHubhttps://github.com/processing/processing/issues/1961#issuecomment-20880918
.

Contributor

dhruv13J commented Jul 12, 2013

Oops, totally missed that... sorry!

On Fri, Jul 12, 2013 at 4:47 PM, datguy notifications@github.com wrote:

One problem with the proposed logic is that we can't uncomment a multiline
comment that had an embedded comment. Let's say I select the following
lines and choose the new comment/uncomment.

/*

  • This is an existing multiline comment
    /
    void setup() {
    //TODO add more code
    println("done"); /
    that's all folks */
    exit();
    }

This is what the above logic would produce:

/*

  • This is an existing multiline comment

    void setup() {
    //TODO add more code
    println("done"); /* that's all folks
    exit();
    } */

To uncomment that, you would need to somehow rebuild the embedded comments
that had their trailing "/" stripped. A safer way to handle existing
comments that use the /
*/ format when they are embedded within the block
you are trying to comment is to revert to //, as follows:

///*
// * This is an existing multiline comment
// /
//void setup() {
// //TODO add more code
//println("done"); /
that's all folks */
//exit();
//}


Reply to this email directly or view it on GitHubhttps://github.com/processing/processing/issues/1961#issuecomment-20880918
.

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry Aug 12, 2015

Member

This was fixed earlier and works properly (or as proper as it's gonna get) in 3.0 beta 3.

Member

benfry commented Aug 12, 2015

This was fixed earlier and works properly (or as proper as it's gonna get) in 3.0 beta 3.

@benfry benfry closed this Aug 12, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment