Add new comments preference. Support indent block comments on line co… #13254

ficristo · 2017-04-01T16:58:06Z

…mment command

I created a new comments preference with an indent option with the idea of deprecate the indentLineComment preference. Is there a way to deprecate a preference?
I used a new object with the idea to add also a padding option in the future (like the CodeMirror addon), but maybe is better to simply create a new indentBlockComment preference without deprecate anything.
Any thought?

redmunds · 2017-04-03T14:52:14Z

@ficristo I think it's more trouble that it's worth to deprecate the indentLineComment preference. I vote for simply adding a new indentBlockComment preference.

redmunds · 2017-04-03T16:17:57Z

@ficristo Also, I think #13169 can be fixed without adding a new preference as follows:

For languages that do not have a different line comment format (such as HTML), the block comment format is used on a single line. In that case, the indentLineComment preference could be checked.

But I can also see the value of having a separate indenting preference for block comments.

ficristo · 2017-04-03T17:58:54Z

I would find indentLineComment a bit misleading if used also for block comments.
(In fact I wanted a single indent option to be used for line and block comments)
While I think the new object preference would be cleaner, I'll keep it simple and go with adding a new pref.

ficristo · 2017-04-05T18:57:23Z

I still need to test it a bit more

ficristo · 2017-04-09T12:12:06Z

It should be ready.
@redmunds do you have time to review?

…ine comment command

redmunds · 2017-04-10T19:13:10Z

Seeing weird results for block comment, indentBlockComment=true, and cursor at start of line (i.e. line not selected). Before:

  <p>lkj wlekjr wlkerj lwkejr wlke lqkwje lkqjwe lkqwje lwkqje lq</p>

After:

--<!-->  <p>lkj wlekjr wlkerj lwkejr wlke lqkwje lkqjwe lkqwje lwkqje lq</p>

redmunds · 2017-04-10T19:18:41Z

src/editor/EditorCommandHandlers.js

+                    if (indentBlockComment) {
+                        var endCh = _firstNotWs(doc, sel.end.line - 1);
+                        editGroup.push({
+                            text: _.repeat(" ", endCh) + suffix + "\n",


Can't always insert spaces -- need to use indent preference here in case it's Tabs.

Great catch! Fixed.

redmunds · 2017-04-10T19:19:48Z

src/editor/Editor.js

@@ -2709,6 +2713,27 @@ define(function (require, exports, module) {
    };

    /**
+     * Returns true if blockCommentIndent is enabled for the specified or current file


First line of this comment is true for "get", but not "set".

redmunds · 2017-04-10T19:28:22Z

@ficristo Done with review. Found a couple cases that don't work as expected, but mostly working as advertised. Tests pass for me on Mac 10.12.

redmunds · 2017-04-10T19:31:08Z

Also, all Preferences should be listed in wiki here:
https://github.com/adobe/brackets/wiki/How-to-Use-Brackets#preferences

Please add indentBlockComment and indentLineComment (which was never added). Thanks!

ficristo · 2017-04-11T19:07:19Z

I cannot reproduce the issue above about the html. Is that the full snippet?

redmunds · 2017-04-11T23:52:50Z

@ficristo To repro this bug, make sure cursor is at start of line, or in middle of indent. Doesn't seem to happen if cursor is immediately before start tag.

I also notice weird results when cursor is in middle of tag contents. Comment is added from start of tag to cursor positioin, which causes invalid HTML:

  <!--<p>lkj wlekjr wlkerj lwkejr wlke--> lqkwje lkqjwe lkqwje lwkqje lq</p>

redmunds · 2017-04-11T23:57:26Z

@ficristo Here's a simple page where I'm also seeing problem on <p> tag:

<!doctype html>
<html>
<head>
<title></title>
</head>
<body>
  <p>one two three</p>
</body>
</html>

ficristo · 2017-04-12T11:08:12Z

Odd, I still cannot reproduce. I tryed opening a new folder with only a test.html file with the snippet you posted above.
Some tests I added put the cursor at the beginning of the line. Do all the tests pass for you?
Nevermind. The feature work if you use Edit \ Toggle Line Comment but not if you use Edit \ Toggle Block Comment. I'll take a look.

ficristo · 2017-04-16T17:08:13Z

Initially I thought that Edit \ Toggle Block Comment was equal to Edit \ Toggle Line Comment but added block comments. But instead it add a block comment starting and ending at the exact position of the cursors.
I don't want to change that.
I want to support indentation for line comments also for languages with only block delimiters (like html).
I also removed the new preference and reused indentLineComment, I think the new preference would be misleading.
@redmunds could you take a look?

redmunds · 2017-04-16T19:32:18Z

@ficristo

But instead it add a block comment starting and ending at the exact position of the cursors.

Yes, that's the case when either start or end cursor is in middle of line. I agree that behavior should not change.

But the other case is when the selection only contains full lines (both start and end cursors are at start of line). In that case, a new line is added above selection with opening block delimiter, and another line is added after selection with closing block delimiter. This is the case where I was expecting the new indentBlockComment to take effect and delimiters would be indented to match code. Indenting for first line of selection might not match indenting of last line -- not sure how to handle that case.

Does that make sense? Let me know if you want to take that on or not -- I'll review code either way.

ficristo · 2017-04-17T10:55:57Z

Oh, I think we had a bit different things in mind.
(Now that I know what it does...) I prefer to leave Edit \ Toggle Block Comment as is.
While interesting, it would seem odd to match the indentatation when the cursors are exactly at the beginning but not if they aren't, or so it appears to me at the moment.
If possible I prefer to not implement this part of the feature.

redmunds · 2017-04-20T17:16:28Z

src/editor/EditorCommandHandlers.js

+    function _firstNotWs(doc, lineNum) {
+        var text = doc.getLine(lineNum);
+        if (text === null || text === undefined) {
+            return null;


The return value from this function is not validated -- it gets used directly as a char position. Seems like it would be more consistent with CodeMirror APIs to return -1 here (but my CM API knowledge is rusty...).

redmunds · 2017-04-20T17:40:13Z

@ficristo Done with review. Looks good except for maybe 1 change.

ficristo · 2017-04-20T18:15:04Z

I seems to remember I hit a case where the API could return null, but I don't remeber which...
And looking around the codebase I didn't find any check at all after the call to doc.getLine.
I changed it to return 0, I hope its fine for now.

ficristo · 2017-04-21T18:17:37Z

@redmunds could you take a final look (and eventually merge)?

redmunds · 2017-04-24T20:21:13Z

LGTM. Merging.

ficristo · 2017-04-25T05:35:26Z

Thank you.

ficristo force-pushed the comments branch from f959789 to ac88e1b Compare April 9, 2017 12:07

Add indentBlockComment preference. Support indent block comments on l…

e7ed43d

…ine comment command

ficristo force-pushed the comments branch from ac88e1b to e7ed43d Compare April 9, 2017 12:31

redmunds self-assigned this Apr 10, 2017

redmunds reviewed Apr 10, 2017

View reviewed changes

Check for useTabChar and correct a comment. Add some tests

79c2bfd

ficristo added 4 commits April 15, 2017 12:42

Use a single preference

15588c2

Add test for block comments

bd392a6

Correct a test

cd78916

Check also if it is a line comment command

dc9f3af

redmunds reviewed Apr 20, 2017

View reviewed changes

Return 0 from _firstNotWs

8a71b9f

redmunds merged commit 9b03593 into adobe:master Apr 24, 2017

ficristo deleted the comments branch April 25, 2017 05:35

ficristo added this to the Release 1.10 milestone Jun 18, 2017

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] Add new comments preference. Support indent block comments on line co… brackets-archive/bracketsIssues#11238

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new comments preference. Support indent block comments on line co… #13254

Add new comments preference. Support indent block comments on line co… #13254

ficristo commented Apr 1, 2017

redmunds commented Apr 3, 2017

redmunds commented Apr 3, 2017

ficristo commented Apr 3, 2017

ficristo commented Apr 5, 2017

ficristo commented Apr 9, 2017

redmunds commented Apr 10, 2017

redmunds Apr 10, 2017

ficristo Apr 11, 2017

redmunds Apr 10, 2017

redmunds commented Apr 10, 2017

redmunds commented Apr 10, 2017

ficristo commented Apr 11, 2017

redmunds commented Apr 11, 2017

redmunds commented Apr 11, 2017

ficristo commented Apr 12, 2017 •

edited

Loading

ficristo commented Apr 16, 2017

redmunds commented Apr 16, 2017

ficristo commented Apr 17, 2017

redmunds Apr 20, 2017

redmunds commented Apr 20, 2017

ficristo commented Apr 20, 2017 •

edited

Loading

ficristo commented Apr 21, 2017

redmunds commented Apr 24, 2017

ficristo commented Apr 25, 2017

Add new comments preference. Support indent block comments on line co… #13254

Add new comments preference. Support indent block comments on line co… #13254

Conversation

ficristo commented Apr 1, 2017

redmunds commented Apr 3, 2017

redmunds commented Apr 3, 2017

ficristo commented Apr 3, 2017

ficristo commented Apr 5, 2017

ficristo commented Apr 9, 2017

redmunds commented Apr 10, 2017

redmunds Apr 10, 2017

Choose a reason for hiding this comment

ficristo Apr 11, 2017

Choose a reason for hiding this comment

redmunds Apr 10, 2017

Choose a reason for hiding this comment

redmunds commented Apr 10, 2017

redmunds commented Apr 10, 2017

ficristo commented Apr 11, 2017

redmunds commented Apr 11, 2017

redmunds commented Apr 11, 2017

ficristo commented Apr 12, 2017 • edited Loading

ficristo commented Apr 16, 2017

redmunds commented Apr 16, 2017

ficristo commented Apr 17, 2017

redmunds Apr 20, 2017

Choose a reason for hiding this comment

redmunds commented Apr 20, 2017

ficristo commented Apr 20, 2017 • edited Loading

ficristo commented Apr 21, 2017

redmunds commented Apr 24, 2017

ficristo commented Apr 25, 2017

ficristo commented Apr 12, 2017 •

edited

Loading

ficristo commented Apr 20, 2017 •

edited

Loading