Block Comment Improvements/Fixes #7135
Conversation
…ents as prefix of bock comments and equal block comments delimiters
| if (!result || ctx.token.type !== "comment" || ctx.token.string.match(suffixExp)) { | ||
| // Is a range of text selected? (vs just an insertion pt) | ||
| var hasSelection = (sel.start.line !== sel.end.line) || (sel.start.ch !== sel.end.ch); | ||
| // Next, move forwards until we find a comment inside the selection |
There was a problem hiding this comment.
From here until the part when we actually do the block commenting, the code is completely different, which might make it easier to read the actual code and not this diff. This comment can explain better how it works.
|
@njx Merged this with master, so it now works with CM v4. All tests are passing too. |
|
Great! Thanks for integrating it with the new v4 stuff - looks like you didn't really have to touch the multiselect bit much. I'll try to review this next week. |
|
The big improvements here is the way it finds the block comment and that part didn't changed with multiselect. Explained in more detail in this comment. This also fixes the issue with line comments delimiters that are a suffix of block comment delimiters and that is what changed in the line comment section. Let me know if you when you need this to be merged with master. |
|
Hey - sorry, I got swamped with stuff and didn't get to review this yet, but I haven't forgotten about it :) It's at the top of my list for this week to at least do an initial review - depending on when we branch for release it might land in the next release after this one though. |
|
It is ok, but it got out of sync again, so let me know when you want to merge it with master. |
|
+1! |
|
i would love to get involved in this... there are things that seem to be missing for coffeescript support like auto close block comments... sintax highlightning for function definitions like () -> .... am i missing some extension? how can i help improve this? should i develop an extension? is this getting built in support? |
|
@joaoafrmartins if you're seeing syntax highlighting problems, that won't be addressed by this pull request. Can you check if the problems also occur here, and if so file a bug on CodeMirror? (If not please file a new, separate Brackets bug). Auto-closing block comments is not implemented in Brackets for any language yet. I've added a user story to our backlog for this: Auto insert close of block comment. Please upvote it to help raise its priority. Edit: in the meantime you could write a simple extension to add the auto-close functionality just for CoffeeScript. See the reasonable comments extension which does similar things for |
|
@TomMalbran I took the liberty of merging it with master so I could play with it - the changes were pretty small (the main thing was in EditorCommandHandlers where we changed calls to |
|
|
||
| for (i = lineSyntax.length; i < blockSyntax.length; i++) { | ||
| character = blockSyntax.charAt(i); | ||
| subExps.push(prevChars + "[^" + character + "]"); |
There was a problem hiding this comment.
Do we need to StringUtils.regexEscape() the character?
There was a problem hiding this comment.
I think we should, just in case. Fixed it
|
@TomMalbran - started reviewing this but won't get a chance to finish today. Will keep picking away at it when I have time. |
|
Found a broken case: in CoffeeScript mode, enter this: ### x ###
# fooand put the cursor on the blank line, then do Toggle Block Comment. It deletes the It seems like we should be careful not to move past the current line when doing the initial check for whether the current line has a comment or not. Maybe we could add a couple of different token steppers (e.g. |
| if (_matchExpressions(ctx.token.string, lineExp)) { | ||
| // If the token starts at ch 0 with no starting white spaces, then this might be a block comment or a line | ||
| // comment over the whole line, and if we found this comment at the start of the selection, we need to search | ||
| // backwards until we get can tell if we are in a block or a line comment |
There was a problem hiding this comment.
I don't understand this case. If I'm understanding correctly, the issue here might be that we're immediately before a line comment, but might be inside an outer block comment. I'm not sure why that case can only occur if the line comment starts at the very beginning of a line - wouldn't the same issue occur if the line comment started later on the line after whitespace?
There was a problem hiding this comment.
(Looks like you didn't respond to this comment - I wonder if it's related to the case I mention below, where the line comments start after some indentation.)
There was a problem hiding this comment.
I missed this comment. That is because if we are after the first line of a block comment that spans multiple lines, the string returned from the context actually starts from the char 0, so every whitespace before the comment is included in the string and get the correct type of comment. So if there are whitespaces at the start of the string and has a type of comment we know that it is a block comment. The only case where we don't know what happens is when the string starts with a line comment prefix.
If we have:
/* comment
| // line comment
*/
The string in the context at the cursor is actually // line comment and the position starts on the column 0. If there wasn't a block comment surrounding the line comment, the string would be // line comment and the position wouldn't start on the column 0.
Now if we have:
/* comment
// |line comment
*/
You can't tell, at the cursor context, if is a line comment or a block comment.
|
Moving the Token on line will not help in this case, since the line is empty or is only whitespaces, and the token is empty. So we need to move to the previous token to see what is happening. We could either be inside a comment or not. The fix should first be able to find the block comment suffix and after that we should check if the cursor from the initial position is between the prefix and suffix. I think I am doing something similar for another case. |
|
|
||
| // If we are in a line that only has a prefix or a suffix and the prefix and suffix are the same string, | ||
| // lets find first if this is a prefix or suffix and move the token to the inside of the block comment. | ||
| // This is required so that later we can find the prefix by moving backwards and the suffix by moving forwards. |
There was a problem hiding this comment.
This comment is slightly confusing since it implies that the ctx will always end up after the prefix of the block comment, but in at least one case (where the comment is empty and the initial pos was inside the comment) it will actually end up before the prefix. That case seems to be okay since below we start at the current token to see if it matches the prefix, but it's worth clarifying here that that edge case is correct.
There was a problem hiding this comment.
When I refer to the inside of the block comment, it means that the ctx will be after the prefix and before the suffix, or at least the string of the ctx will be at the prefix or suffix.
| invalidComment = result && !!ctx.token.string.match(prefixExp); | ||
|
|
||
| // We moved the token at the start when it was in a whitespace, but maybe we shouldn't have done it | ||
| if (prefixPos && editor.indexFromPos(sel.end) < editor.indexFromPos(prefixPos)) { |
There was a problem hiding this comment.
More efficient to check CodeMirror.cmpPos(sel.end, prefixPos) < 0)
|
Initial review done. Overall the new algorithm seems pretty straightforward. I never really looked at the old code, though, and don't have all the use cases in my head, so can't really evaluate the code from that perspective, but luckily the existing unit tests seem to quite extensively cover all the desired cases for existing languages, so it seems like we're good there. A couple of general thoughts:
|
|
Thanks for the review. I probably won't be able to work on the comments this week, but i'll try to do it next week. You are right. That comment is really outdated. I don't think that the new code might affect the multiple selections, since I haven't touched the code that deals with the selections, and I am still not sure how it actually works. |
|
Yeah - I guess what I meant with the last one is whether there would be some user-visible case where having multiple selections should do something different than you would expect by just iterating over each selection independently (and that would be specific to the new cases). But I just discovered that there were already cases (in current master) where the multiple-selection stuff isn't doing the right thing anyway - for example, if you have multiple cursors inside a single block comment, hitting toggle block comment does nothing (because it ends up generating overlapping edits, which get rejected by the doMultipleEdits stuff). I don't think there's anything specific to multiple selections with the new CoffeeScript-like cases, and anyway we should probably fix the other issues more generally (though they're also probably not super important). I'll file them separately. |
|
Filed the latter as #7510 |
| // Save the initial position to start searching for the suffix from here | ||
| initialPos = $.extend({}, ctx.pos); | ||
|
|
||
| // Find the position of the start of the prefix |
There was a problem hiding this comment.
grammar nit: should be "find the starting position of the prefix"
There was a problem hiding this comment.
I think it's also grammatically correct as-is though
|
@njx I fixed all the comments and the case you mentioned, and added a few more tests, including one for the case you mentioned. |
|
Thanks, will try to take a look later this week. |
|
Hey - finally got around to looking at this again. It's too bad it took me so long, because I've forgotten the context of most of my original comments :(, but the changes seem to make sense. I've found a few more issues, though: (but maybe we don't need to fix all of them)
// a comment
// another commentand select across them, then toggle line comment, it will remove the line comments. With this PR, it adds another level of line comments. This is a bit of an edge case (it seems to work if there are no blank lines), but it is a style some people use. (Though if they are using it, they won't be happy with the way we add line comments, since we always add them at the beginning of the line.)
### a comment ###
alert "foo!"
### another comment ###and then I select all the lines and toggle line comment multiple times, it keeps adding line comments instead of toggling them on and off. I almost think I don't care about this case :), because at that point it seems hard to detect what the heck is going on anyway. But I imagine it could annoy some people. I could live with not fixing this and seeing if anyone complains about it. |
|
@njx I fixed the comments and the first 2 cases you mentioned. I don't know how to fix the last one since is an issue with the RegExp created for the line comment delimiters.
Fixed this and added a new test for it.
I was trying to fix the other issue you mentioned on the first review and had the problem of having
I am not sure how to fix this. We would need to change the RegExps created for the line comment delimiters somehow to include this cases. The code in Ready for another review. |
|
Looks good. Merging (finally!) |
|
Thanks for persevering with this @TomMalbran :) |
|
Woot!!! Thanks |
This is a remake of PR #3060 which greatly simplifies the search for Block Comments and fixes the issues of having line comments delimiters be a prefix of block comment delimiters and the issue when the block comment delimiters use the same string.