Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Customize matching pairs - add to or remove from the package defaults #249

Merged
merged 17 commits into from Dec 6, 2016

Conversation

potto007
Copy link
Contributor

@potto007 potto007 commented Sep 24, 2016

This allows you to add or remove matching pairs from Bracket Matcher at any time. You can do so either Globally via the Settings view (cmd-,) or at the Scope level via config.cson (cmd-shift-p + "config"). Changes take effect immediately.

I got tired of fighting this while developing in Rust. Upon searching about it, I found numerous discussions about the need, and pinpointed two existing issues pertaining to this enhancement. Issue #214 requests the ability to choose which matching pairs to use. Issue #236 requests the ability to turn off the matching functionality for certain characters.

screen shot 2016-09-24 at 7 47 37 am

@50Wliu
Copy link
Contributor

50Wliu commented Sep 25, 2016

Instead of adding an "exclude pairs", I would rename "Add Pairs" to "Autocompleted Characters" (or similar), then make the default of that setting include the current list of default pairs. That would satisfy both requirements. In addition, the "Autocomplete Brackets/Smart Quotes" options can be removed since "Autocompleted Characters" would handle that as well.

There should also be some more documentation regarding : as the pair separator.

/cc @atom/feedback

@50Wliu
Copy link
Contributor

50Wliu commented Sep 25, 2016

Also, with the current setup it's impossible to add : as a character to pair with.

expect(buffer.lineForRow(0)).toBe "<>"
expect(editor.getCursorBufferPosition()).toEqual([0, 1])

# Scope tests inexplicably fail
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests will need to be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started down a scary road on this one - ended up time-boxing and disabling the tests at this point. The problem seems to be with the Spec library itself in the mocking of the testing editor. I added a bunch of console.log lines all over the code - all of them confirmed that I was grabbing from the correct scope. For some reason, the way the test codebase is implementing the atom.config.set at scope level, it isn't triggering a callback.

I gave myself a TODO to create some tests of the tests (heh) to try to consistently recreate what I see as a potential bug. Then I can go about fixing.

@@ -29,6 +29,25 @@ class BracketMatcher
else
@pairedCharacters = @defaultPairs

excludePairs: (excludePairs) ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disclaimer: I am unfamiliar with the structure of bracket-matcher.

It looks like this logic is being duplicated in bracket-matcher-view...would it be possible to unify the two?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I noticed that as well. It is now even more similar after the refactoring I did for performance. bracket-matcher.coffee was previously parsing and modifying the config options every time an edit operation was being performed. Ouch! I've changed it so that an update to config routine is called by the constructor; I then added callbacks to each of the config options so that the update config routine is called anytime the options change.

I ended up going back to bracket-matcher-view.coffee and putting in very similar behaviors. So yes, it should be refactored to have shared code, and only separate concerns where necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've refactored the package quite a bit now in an effort to remove duplicated code. This was / is harder than I thought, as it isn't always code that is duplicated, but behavior. I found logic written two different ways between those two files. I believe my rewrite cleans it up a decent amount, and provides for a better separation of concerns.

@@ -544,6 +544,44 @@ describe "bracket matching", ->
expect(buffer.lineForRow(0)).toBe "{}"
expect(editor.getCursorBufferPosition()).toEqual([0, 1])

describe "when addPairs configuration is set globally", ->
it "inserts a matching carat", ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

carat -> caret

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh! Good catch.

@50Wliu
Copy link
Contributor

50Wliu commented Sep 25, 2016

This should also fix #121.

@potto007
Copy link
Contributor Author

Instead of adding an "exclude pairs", I would rename "Add Pairs" to "Autocompleted Characters" (or similar), then make the default of that setting include the current list of default pairs. That would satisfy both requirements. In addition, the "Autocomplete Brackets/Smart Quotes" options can be removed since "Autocompleted Characters" would handle that as well.

@50Wliu - Good point. This occurred to me when I was refactoring some of the codebase - I ended up submitting so I could get some feedback before proceeding. It seems that the design pattern for Atom follows what you suggest, having a default behavior, which is shown in light gray within text boxes, which then becomes an all-or-nothing setting. I'll make that change, it would definitely make more sense.

@50Wliu
Copy link
Contributor

50Wliu commented Sep 25, 2016

Also fixes #171.

@potto007
Copy link
Contributor Author

There should also be some more documentation regarding : as the pair separator.
Also, with the current setup it's impossible to add : as a character to pair with.

Agreed. I used the : because it represents the KV separator in the codebase. Didn't really consider whether it was a good one or not - also, didn't test whether one could use escaping for this. I'll give this some thought. Thanks.

@potto007
Copy link
Contributor Author

Ok, I've changed it to not even have a separator; looks a lot cleaner, and removes the need to worry about escaping for any reason. So a line in the config would look like this: <>,%%,::

Cool?

@50Wliu
Copy link
Contributor

50Wliu commented Sep 25, 2016

Yep! Except, of course, for , 😉. I seriously doubt any language uses , pairing though.

@potto007
Copy link
Contributor Author

Yep! Except, of course, for , 😉. I seriously doubt any language uses , pairing though.

Ironically, comma's now work, but only if added via the config.cson file. ;)

@potto007
Copy link
Contributor Author

This is what the Settings screen now looks like:

screen shot 2016-09-27 at 2 56 39 am

@@ -149,7 +91,7 @@ class BracketMatcher
previousCharacter = previousCharacters.slice(-1)

hasEscapeSequenceBeforeCursor = previousCharacters.match(/\\/g)?.length >= 1 # To guard against the "\\" sequence
if (@pairedCharacters[previousCharacter] is nextCharacter) and not hasEscapeSequenceBeforeCursor and @getScopedSetting('bracket-matcher.autocompleteBrackets')
if (@matchManager.pairedCharacters[previousCharacter] is nextCharacter) and not hasEscapeSequenceBeforeCursor and @getScopedSetting('bracket-matcher.autocompleteBrackets')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you also use autocompletePairs for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

autocompletePairs is a local variable only in scope in the processAutoPairs method in MatchManager (line 17 - 21).


module.exports =
class MatchManager
smartQuotePairs:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this setting is currently enabled by default, would it make sense to 🔥 all of these and add them to the new autocompletePairs default as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that's easy enough to chop out. I wasn't sure if there was a specific purpose or desire to treat smartquotes distinctly. I personally never use them...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@50Wliu
Copy link
Contributor

50Wliu commented Oct 1, 2016

Just realized I forgot to hit the "comment" button after submitting my review comments... 🙈

@lierdakil
Copy link

My two cents on the issue.

Being able to explicitly remove some characters from auto-matching while implicitly leaving the other defaults in is something I'd prefer to see. I.e. I'd like to have both 'Autocomplete Characters' and 'Excluded Characters'. My reasoning being that if I want to disable some characters (e.g. ') on scope-level, I would have to drag a whole list of defaults with that, which seems suboptimal.

For example, I would prefer something like this:

".some.scope":
  "bracket-matcher":
    excludeCharacters: ["'"]

to

".some.scope":
  "bracket-matcher":
    autocompleteCharacters: ['()', '[]', '""', '{}' , "``"]

Also second option implies that I will update my config by hand in case defaults change, although that's arguably is not that big of an issue.

@50Wliu
Copy link
Contributor

50Wliu commented Oct 2, 2016

I.e. I'd like to have both 'Autocomplete Characters' and 'Excluded Characters'. My reasoning being that if I want to disable some characters (e.g. ') on scope-level, I would have to drag a whole list of defaults with that, which seems suboptimal.

Yes, that's something that I was considering bringing up with the nonWordCharacters core setting as well. Good point. Perhaps the scoped setting should be addPair and excludePair (don't implement this yet though).

/cc @atom/feedback

@ungb
Copy link
Contributor

ungb commented Nov 29, 2016

@maxbrunsfeld @nathansobo @iolsen I was wondering if you guys had any feedback on the Pairs to Indent.

I definitely feel this PR is a improvement over the current behavior, but I feel the wording for Pairs to Indent might be misleading as it actually doesn't control the indention, but more so it adds two new lines and moves the cursor up by one to be in the middle.

Also, was wondering about the comment @50Wliu made :

Yeah. I don't like this pairs to indent thing. Sounds like it should be moved to the individual languages' settings files.

Here's a screenshot of the test cases I ran:
testcase

@nathansobo
Copy link
Contributor

but I feel the wording for Pairs to Indent might be misleading as it actually doesn't control the indention

After staring at this long enough to understand what's going on, I agree. I do think it makes sense to allow this to be customized beyond the source of a specific language bundle, but that name is misleading...

It's really about placing the closing character of the pair on a new line. Maybe Autonewline Characters with an explanatory string that explains the closing character of these pairs will be placed on its own line below the cursor?

@ungb
Copy link
Contributor

ungb commented Dec 1, 2016

Hey @nathansobo @potto007 @50Wliu @Ben3eeE
What do you guys think:

Instead of Pairs to Indent

Pairs to Format

Automatically format the pair by adding a newline when user clicks enter between the pair defined.

Might be good to add a example to the README as well:

Pairs to Format Example:
format

I think it would be good to have this functionality in this package, but the indention behavior is specific to the language.

@50Wliu
Copy link
Contributor

50Wliu commented Dec 1, 2016

I don't know. I still think this should be handled by the individual languages.

If not, Pairs to Format still sounds ambiguous. Maybe Pairs With Extra Newline? Automatically add a newline between the pair when enter is pressed.

Naming things is hard 😖.

@nathansobo
Copy link
Contributor

nathansobo commented Dec 1, 2016

@50Wliu Can you articulate your position a bit... What if a user wants to have this behavior and a given package doesn't support it? Should the only route be submit a PR on the package?

@50Wliu @ungb I guess I also like Pairs With Extra Newline better even though I told you to just make the decision.

@50Wliu
Copy link
Contributor

50Wliu commented Dec 1, 2016

Never mind. As I was writing out my explanation I've decided that since it's going to be a scoped setting regardless, it'll be fine and language packages will be available to override it/add their own pairs, which was my main concern.

Which brings me to another point...
the current format of settings like pairsToIndent, autocompleteCharacters, and nonWordCharacters from Atom core makes it very hard for languages to override if they want to add/remove something. There's no way to respect the user's setting when doing so since you have to specify everything, rather than being able to add or remove pairs/characters as necessary.

For example, $ is a valid identifier for many languages. However the default nonWordCharacters includes $, which means attempting to double-click to select a variable with $ in it will only select part of it. The best way to fix this currently is to override nonWordCharacters in the language settings by copying/pasting the default value and removing the $. However, this totally disregards any customizations the user may have made.

@nathansobo
Copy link
Contributor

nathansobo commented Dec 1, 2016

@50Wliu Great points. Do you have any ideas for how we could improve our APIs to fix this issue? Maybe opening up an issue to get a discussion started would be helpful if you have the inclination.

@Ben3eeE
Copy link
Contributor

Ben3eeE commented Dec 1, 2016

I also think Pairs With Extra Newline sounds better than Pairs To Format. Lets decide on using that name so we can get this merged.

@ungb
Copy link
Contributor

ungb commented Dec 5, 2016

@Ben3eeE @potto007 I've made the change for Pairs To Indent -> Pairs With Extra Newline as well as the description.

Let me know if we are all in agreement and we can get this merged!

@potto007
Copy link
Contributor Author

potto007 commented Dec 5, 2016

@ungb Thanks for making those changes. I'm good with them. I'm excited to watch this get merged!

@potto007
Copy link
Contributor Author

potto007 commented Dec 5, 2016

Actually - one more thing - I'm updating the README.md to reflect the name change.

@ungb
Copy link
Contributor

ungb commented Dec 5, 2016

Ah I missed that, Thanks @potto007, Will merge today once build passes.

@potto007
Copy link
Contributor Author

potto007 commented Dec 5, 2016

As I'm watching Travis take its typical 15 - 30 minutes to allocate an OS X environment, I thought I'd ask: do any of you know if there's a good reason for the language: objective-c line at the top of the .travis.yml file? I've seen this in some of the other modules, but it doesn't seem like it's necessary, given that this is all CoffeeScript.

I see it in this early Atom documentation: http://blog.atom.io/2014/04/25/ci-for-your-packages.html

@potto007
Copy link
Contributor Author

potto007 commented Dec 5, 2016

@50Wliu @Ben3eeE @ungb
Ok, I understand now why the Atom team went the route of forcing an OS X machine - all those errors are overwhelming during the build of node-gyp. However, those are all due to the fact that it is expecting a C++11 compiler. Travis isn't using a high enough version of gcc by default. So: add the necessary packages and set the environment, and voilà!

Replace language: objective-c with:

env:
  - CXX=g++-4.8
addons:
  apt:
    sources:
      - ubuntu-toolchain-r-test
    packages:
      - g++-4.8

Proof in the pudding: https://travis-ci.org/potto007/bracket-matcher
Notice that mine built and tested in 1 min 39 sec and atom/bracket-matcher is still waiting on a build initiated an hour ago.

@50Wliu
Copy link
Contributor

50Wliu commented Dec 6, 2016

@potto007 would love a PR to change the build environment :).

Copy link
Contributor

@50Wliu 50Wliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly just documentation suggestions.


You can customize matching pairs in Bracket Matcher at any time. You can do so either Globally via the Settings view (<kbd>cmd-,</kbd>) or at the Scope level via `config.cson` (<kbd>cmd-shift-p</kbd> + "config"). Changes take effect immediately.

* <b>Autocomplete Characters</b> - Comma-separated pairs that the editor will treat as brackets / quotes. Entries in this field override the package defaults.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no package defaults anymore, right?
Also, use ** notation for bold.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* <b>Autocomplete Characters</b> - Comma-separated pairs that the editor will treat as brackets / quotes. Entries in this field override the package defaults.
* ie: `<>, (), []`

* <b>Pairs With Extra Newline</b> - Comma-separated pairs that enhance the editor's Auto Indent feature. When used, a newline is automatically added between the pair when enter is pressed between them. Note: This feature is meant to be used in combination with brackets defined for indentation by the active language mode (increaseIndentPattern / decreaseIndentPattern).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

increaseIndentPattern and decreaseIndentPattern should be in backticks.

```

###### config.cson
In addition to Global configs, you are able to add scope-specific modifications to Atom in `config.cson`. This is especially useful for editor rule changes specific to each language. Scope specific configs override package defaults <i>and</i> global configs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about: In addition to the global settings, you are also able.... Same thing at the end: global configs -> global settings. Also, hyphenate Scope-specific and change configs to settings.
* or _ notation for italics.

Again, there really aren't any defaults anymore. Everything's configurable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll implement those changes. WRT defaults, I'm referring to the package.json defaults.

###### config.cson
In addition to Global configs, you are able to add scope-specific modifications to Atom in `config.cson`. This is especially useful for editor rule changes specific to each language. Scope specific configs override package defaults <i>and</i> global configs.
Example:
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add cson to the backticks so that it gets syntax-highlighted

Example:
```
".rust.source":
"bracket-matcher":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing indentation

@updateConfig()

# Subscribe to config changes
@subscriptions.add atom.config.observe 'bracket-matcher.autocompleteBrackets', {scope: @editor.getRootScopeDescriptor()}, (newConfig) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There doesn't seem to be a reason to update the config when bracket autocompletion is turned on or off, is there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True - I think I left this in after a troubleshooting session.

# Subscribe to config changes
@subscriptions.add atom.config.observe 'bracket-matcher.autocompleteBrackets', {scope: @editor.getRootScopeDescriptor()}, (newConfig) =>
@updateConfig()
@subscriptions.add atom.config.observe 'bracket-matcher.wrapSelectionsInBrackets', {scope: @editor.getRootScopeDescriptor()}, (newConfig) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True - I think I left this in after a troubleshooting session.

"items": {
"type": "string"
}
},
"autocompleteBrackets": {
"type": "boolean",
"default": true,
"description": "Autocomplete bracket and quote characters, such as `(` and `)`, and `\"`."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Autocomplete the characters defined in the autocompleteBrackets setting

This setting name is a bit misleading now, but we can't change it without breaking existing configs. Oh well.

"type": "boolean",
"default": true,
"description": "Autocomplete smart quote characters, such as `“` and `”`, and `«` and `»`."
},
"wrapSelectionsInBrackets": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description for this one can also be updated.

@@ -27,10 +27,10 @@ Settings view (<kbd>cmd-,</kbd>).

You can customize matching pairs in Bracket Matcher at any time. You can do so either Globally via the Settings view (<kbd>cmd-,</kbd>) or at the Scope level via `config.cson` (<kbd>cmd-shift-p</kbd> + "config"). Changes take effect immediately.

* <b>Autocomplete Characters</b> - Comma-separated pairs that the editor will treat as brackets / quotes. Entries in this field override the package defaults.
* **Autocomplete Characters** - Comma-separated pairs that the editor will treat as brackets / quotes. Entries in this field override the package defaults.
* ie: `<>, (), []`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to avoid latin abbreviations: use "for example" instead.
(See https://github.com/atom/flight-manual.atom.io/blob/master/CONTRIBUTING.md#language)

@@ -27,10 +27,10 @@ Settings view (<kbd>cmd-,</kbd>).

You can customize matching pairs in Bracket Matcher at any time. You can do so either Globally via the Settings view (<kbd>cmd-,</kbd>) or at the Scope level via `config.cson` (<kbd>cmd-shift-p</kbd> + "config"). Changes take effect immediately.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Globally and Scope shouldn't be capitalized. Settings View should be though.

Also,

...or at the scope level via your config.cson.

You don't need to include the shortcuts. I think it's safe to assume that if they're reading this they should know how to get to the Settings View and config.cson.

* ie: `<>, (), []`

* <b>Pairs With Extra Newline</b> - Comma-separated pairs that enhance the editor's Auto Indent feature. When used, a newline is automatically added between the pair when enter is pressed between them. Note: This feature is meant to be used in combination with brackets defined for indentation by the active language mode (increaseIndentPattern / decreaseIndentPattern).
* **Pairs With Extra Newline** - Comma-separated pairs that enhance the editor's Auto Indent feature. When used, a newline is automatically added between the pair when enter is pressed between them. Note: This feature is meant to be used in combination with brackets defined for indentation by the active language mode (`increaseIndentPattern` / `decreaseIndentPattern`).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uncapitalize auto indent.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants