Line comment bug fix

[heysupratim]

Better line comment toggling , initiates the line comment at the start of the code and maintains the position of the cursor throughout (like sublime )

Better for lint to comment indents

Affected me so hence changed it

Will fail some editorCommandHandler tests which expect the line comment at the start of the line

fix for #11953 (comment)

[abose]

@borax12 Thanks for contributing to brackets.
There is a slight issue here

When you include a selection with uneven cursor position, it tries to comment out in the wrong place, and toggle comment breaks.
This could be fixed if you analyze the left most line position in the selection and then use that position for inserting the comments.

[heysupratim]

Ah , i see, checking it out

[heysupratim]

@abose i had to push extra commits due to some code formatting issues . Hope it works now

[abose]

unit tests for editor command handlers fails after this change. The unit tests needs to be modified with the new commenting structure.

[heysupratim]

@abose - Rewrote the test cases , The editor command handler Unit Test Cases Pass now

[redmunds]

It looks like you're changing it so that inline comments are always indented. Is that correct?

This needs to be a preference because some languages (VBScript, PHP, bash?) require that inline comment delimiter is in the first column. Then you'll need tests for both cases.

[heysupratim]

@redmunds yeah it is always indented . I checked the behaviour in ST2 for php and bash scripts - they are also always indented .

Does brackets have per language based preferences ?

[abose]

🆒 Verified, all unit tests passing now.
You could have a preference like
"dontIntentComments" {"languages":['bash']} ..
With reasonable defaultpreferences set for languages.

[redmunds]

You could have a preference like "dontIntentComments" {"languages":['bash']}

Brackets already has a mechanism for specifying language specific preferences, so no need to invent a new one. Take a look at this example in the How to Use Brackets doc, especially the "language" section:

    "language": {
        "javascript": {
            ...
        }
    },

As far as what to call the new preference, it depends on what the default is. Since the current default is not to indent, then a name like indentInlineComments would be consistent with existing behavior since prefs that are not defined, evaluate to "falsy". If we want to change the default to be indented, then maybe dontIndentInlineComments.

Don't forget to add new preference to Preferences section of the How to Use Brackets doc and also to the defaultPreferences.json file.

[heysupratim]

@redmunds @abose - Ohk , will take a bit of time as i need to see how preference loading and initiating works in brackets . but i will have to research which languages take line comment indentation by default

[TomMalbran]

If some languages require that the comments start on the first column, it should be a property of the language definition and not a preference. There can be a preference if an user decides to overwrite such behaviour or wants to go back to have comments at the start of a line.

[abose]

Never knew that language based pref existed 😅 Thanks @redmunds @TomMalbran

[redmunds]

@borax12 There is already PR #11574 that looks like it fixes the same issue.

@marcelgerber What's the status of your PR?

[abose]

@borax12 Could you also look at #11574 and see if Your implementation has any functionality difference with it.
#11574 also doesn't seem to implement any unit tests.

[heysupratim]

i will do that @Arun . I have been trying to get the per language
based setting correctly . I will also look at #11574

On 12/1/15, Arun Bose notifications@github.com wrote:

@borax12 Could you also look at #11574 and see if Your implementation has
any functionality difference with it.
#11574 also doesn't seem to implement any unit tests.

---

Reply to this email directly or view it on GitHub:
#11954 (comment)

[image: Supratim Chakraborty on about.me]

Supratim Chakraborty
about.me/borax12
http://about.me/borax12

[abose]

@borax12 Did you get any chance to look into the isues? This is a very good usability improvement that we could ship as part of 1.6 coming late december or early jan.

[heysupratim]

@petetnt - i separated the test cases with one set making it run with preference set to true and other making it run on preference set to false

[heysupratim]

@petetnt - is there anything else needed on this , just needed an update :)

[petetnt]

Sorry @borax12, I have been swamped with work so I haven't had the time to review the latest commit. I'll do it tomorrow morning if that is okay with you 👍

[heysupratim]

no worries there @petetnt - appreciate the update .And regarding the work . I hope it turns out the best for you :)

[heysupratim]

@petetnt - updates? :)

[petetnt]

Style nit: Indentation with the beforeEach and afterEach

[heysupratim]

@petetnt -what exactly needs to be indented here ?

[petetnt]

Sorry about being vague: I mean that there's extra indentation within this function block:

        beforeEach(function () {
                setupFullEditor();
                PreferencesManager.set("indentLineComment",true);
            });

Should be:

        beforeEach(function () {
            setupFullEditor();
            PreferencesManager.set("indentLineComment",true);
        });

[petetnt]

@borax12 Sorry for taking so long 🐌

I made some more comments, mostly just style nitpicks but there was a single test that wasn't changed to split format later in the file too.

[heysupratim]

@petetnt - i made the final changes , split the two tests and tried to address the top mentioned style nits , hopefully this should do it :)

[heysupratim]

@petetnt - hey man , i must be bugging you , but i did split the changes, i guess that should do it

[petetnt]

LGTM /cc @abose

[abose]

Thanks @borax12 @petetnt .Will check this out over the weekend .

[abose]

LGTM. Verified all the test cases hilighted.
Thanks very much for this change @borax12
Merging. 🎆

[heysupratim]

cool @abose - looking forward to contributing more :)

[abose]

@borax12 If you are interested in contributing to new features in brackets, please take a look at our trello board to see features requested by the community.
If you are interested in fixing something more immediate, Many unit tests are failing, which needs to be fixed 😅