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

Newline and indent on appropriate pairs #80

Closed
rdallasgray opened this Issue Apr 21, 2013 · 20 comments

Comments

Projects
None yet
4 participants
@rdallasgray

rdallasgray commented Apr 21, 2013

Hi Fuco -- this isn't an issue so much as a suggestion/request for comment.

I'm using this code to get autopair-like functionality on braces and brackets, where a newline should enter the created sexp and indent according to mode. I wonder if some sort of adaptation of this would be a useful addition to the default config, in order to achieve parity of behaviour with autopair?

@Fuco1

This comment has been minimized.

Show comment
Hide comment
@Fuco1

Fuco1 Apr 21, 2013

Owner

That's awesome! So it only adds the "double newline" if user uses newline, otherwise it stays on the same line, right?

This is really good to maybe even include in the trunk, and add a simple switch that would enable it for any pair (for example another argument for sp-local-pair or adding it as a type of action "reindent").

Owner

Fuco1 commented Apr 21, 2013

That's awesome! So it only adds the "double newline" if user uses newline, otherwise it stays on the same line, right?

This is really good to maybe even include in the trunk, and add a simple switch that would enable it for any pair (for example another argument for sp-local-pair or adding it as a type of action "reindent").

@rdallasgray

This comment has been minimized.

Show comment
Hide comment
@rdallasgray

rdallasgray Apr 21, 2013

On 21 Apr 2013, at 21:33, Matus Goljer notifications@github.com wrote:

That's awesome! So it only adds the "double newline" if user uses newline, otherwise it stays on the same line, right?

Yeah -- if the command after 'insert isn't newline or newline-and-indent, the hook is removed. It's the same as Textmate behaviour.

This is really good to maybe even include in the trunk, and add a simple switch that would enable it for any pair (for example another argument for sp-local-pair or adding it as a type of action "reindent").

Absolutely, if that's something you'd be up for doing. I think it's a common enough behaviour to be included somehow in sp, if you want it. Let me know if I can help (I'd be happy to write tests).

RDG

rdallasgray commented Apr 21, 2013

On 21 Apr 2013, at 21:33, Matus Goljer notifications@github.com wrote:

That's awesome! So it only adds the "double newline" if user uses newline, otherwise it stays on the same line, right?

Yeah -- if the command after 'insert isn't newline or newline-and-indent, the hook is removed. It's the same as Textmate behaviour.

This is really good to maybe even include in the trunk, and add a simple switch that would enable it for any pair (for example another argument for sp-local-pair or adding it as a type of action "reindent").

Absolutely, if that's something you'd be up for doing. I think it's a common enough behaviour to be included somehow in sp, if you want it. Let me know if I can help (I'd be happy to write tests).

RDG

@Fuco1

This comment has been minimized.

Show comment
Hide comment
@Fuco1

Fuco1 Apr 21, 2013

Owner

All right. I'll try to book some time for this, hopefully sometime next week.

Owner

Fuco1 commented Apr 21, 2013

All right. I'll try to book some time for this, hopefully sometime next week.

@rdallasgray

This comment has been minimized.

Show comment
Hide comment
@rdallasgray

rdallasgray Apr 21, 2013

On 21 Apr 2013, at 21:49, Matus Goljer notifications@github.com wrote:

All right. I'll try to book some time for this, hopefully sometime next week.

Sure thing. Apologies if I'm not so fast, I have a full-time job and a toddler :)

rdallasgray commented Apr 21, 2013

On 21 Apr 2013, at 21:49, Matus Goljer notifications@github.com wrote:

All right. I'll try to book some time for this, hopefully sometime next week.

Sure thing. Apologies if I'm not so fast, I have a full-time job and a toddler :)

@Fuco1

This comment has been minimized.

Show comment
Hide comment
@Fuco1

Fuco1 Apr 21, 2013

Owner

Hah, no worries. I'm currently going crazy about my master's thesis, so I don't have all that much time for emacs either. Emacs isn't leaving anywhere anytime soon :P

Owner

Fuco1 commented Apr 21, 2013

Hah, no worries. I'm currently going crazy about my master's thesis, so I don't have all that much time for emacs either. Emacs isn't leaving anywhere anytime soon :P

@Fuco1

This comment has been minimized.

Show comment
Hide comment
@Fuco1

Fuco1 Apr 23, 2013

Owner

Just thinking out loud here, if anyone has comments feel free to express them :P

I'm trying to come up with how to do this in the most generic way possible. But I'm also not sure if it will be worth it to over-engineer it much. Basically, would there ever be any need for other scheme than transforming

blabla {|}

into

blabla {
  |
}

Maybe users could provide their own definition of gp/sp/create-newline-and-enter-sexp. I would quite like to have this simply as an action you would add, but that has to be just dumb symbol... Or post-handler, but then those are even more general (since you've implemented this using that mechanism).

Well... I think I'll go for :reindent t or :reindent 'your-function. Maybe the keyword can be different? Dunno, reindent seems logical to me but I'm not the most logical person.

Owner

Fuco1 commented Apr 23, 2013

Just thinking out loud here, if anyone has comments feel free to express them :P

I'm trying to come up with how to do this in the most generic way possible. But I'm also not sure if it will be worth it to over-engineer it much. Basically, would there ever be any need for other scheme than transforming

blabla {|}

into

blabla {
  |
}

Maybe users could provide their own definition of gp/sp/create-newline-and-enter-sexp. I would quite like to have this simply as an action you would add, but that has to be just dumb symbol... Or post-handler, but then those are even more general (since you've implemented this using that mechanism).

Well... I think I'll go for :reindent t or :reindent 'your-function. Maybe the keyword can be different? Dunno, reindent seems logical to me but I'm not the most logical person.

@rdallasgray

This comment has been minimized.

Show comment
Hide comment
@rdallasgray

rdallasgray Apr 23, 2013

I think the crux of it might be

a) only calling the post-handler on a given command (or set of commands, as in this case) and
b) only calling it once

... that would make it simple to attach very general kinds of events like this. Those aspects were the harder things to implement in the code, so a built-in way to do them in smartparens would make the behaviour much easier.

That said, I doubt there really is much call for behaviour other than what you've outlined above, and :reindent 'your-function would allow plenty of flexibility.

rdallasgray commented Apr 23, 2013

I think the crux of it might be

a) only calling the post-handler on a given command (or set of commands, as in this case) and
b) only calling it once

... that would make it simple to attach very general kinds of events like this. Those aspects were the harder things to implement in the code, so a built-in way to do them in smartparens would make the behaviour much easier.

That said, I doubt there really is much call for behaviour other than what you've outlined above, and :reindent 'your-function would allow plenty of flexibility.

@Fuco1

This comment has been minimized.

Show comment
Hide comment
@Fuco1

Fuco1 Apr 23, 2013

Owner

Right, that would make for pretty flexible solution indeed! From the implementation point of view it's really equally complex to implement than the simple :reindent list, since I already have lots of infrastructure covered (adding arguments to the pairs/post-command handler, etc...) I like it :P

It's just adding another list argument where you can give it a cons of (function to run . run after these commands) and it will arrange that it will get executed if the next command is from the list. Sort of like a conditional post-hook.

Owner

Fuco1 commented Apr 23, 2013

Right, that would make for pretty flexible solution indeed! From the implementation point of view it's really equally complex to implement than the simple :reindent list, since I already have lots of infrastructure covered (adding arguments to the pairs/post-command handler, etc...) I like it :P

It's just adding another list argument where you can give it a cons of (function to run . run after these commands) and it will arrange that it will get executed if the next command is from the list. Sort of like a conditional post-hook.

@ilohmar

This comment has been minimized.

Show comment
Hide comment
@ilohmar

ilohmar Jun 1, 2013

I also use this code heavily starting a few weeks ago --- would make an awesome addition to smartparens!

I have it hooked into sp's :post-handlers argument like this:

;; original from https://github.com/rdallasgray/graphene

(defvar gp/sp/post-command-count 0
  "Number of commands called after a pair has been opened.")

(defun gp/sp/create-newline-and-enter-sexp ()
  "Open a new brace or bracket expression, with relevant newlines and indent. "
  (newline)
  (indent-according-to-mode)
  (forward-line -1)
  (indent-according-to-mode))

(defun gp/sp/release-newline-post-command ()
  "Remove the hook and reset the post-command count."
  (remove-hook 'post-command-hook 'gp/sp/await-newline-post-command)
  (setq gp/sp/post-command-count 0))

(defun gp/sp/await-newline-post-command ()
  "If command is newline, indent and enter sexp."
  (if (> gp/sp/post-command-count 1)
      (gp/sp/release-newline-post-command)
    (progn
      (setq gp/sp/post-command-count (+ gp/sp/post-command-count 1))
      (when (memq this-command
                  '(newline newline-and-indent reindent-then-newline-and-indent))
        (gp/sp/release-newline-post-command)
        (gp/sp/create-newline-and-enter-sexp)))))

(defun gp/sp/await-newline (id action context)
  (when (eq action 'insert)
    (add-hook 'post-command-hook 'gp/sp/await-newline-post-command)))

(sp-with-modes '(csharp-mode js-mode awk-mode)
  (sp-local-pair "(" nil :post-handlers '(:add gp/sp/await-newline))
  (sp-local-pair "{" nil :post-handlers '(:add gp/sp/await-newline))
  (sp-local-pair "[" nil :post-handlers '(:add gp/sp/await-newline)))

Of course, anything but gp/sp/create-newline-and-enter-sexp and the list to check (newline newline-and-indent reindent-then-newline-and-indent) could be nicely abstracted into smartparens. How about a keyword :post-handlers-if, and a pair of a list (of triggering commands) and what to do then? That's a very generic way, which I generally love.

ilohmar commented Jun 1, 2013

I also use this code heavily starting a few weeks ago --- would make an awesome addition to smartparens!

I have it hooked into sp's :post-handlers argument like this:

;; original from https://github.com/rdallasgray/graphene

(defvar gp/sp/post-command-count 0
  "Number of commands called after a pair has been opened.")

(defun gp/sp/create-newline-and-enter-sexp ()
  "Open a new brace or bracket expression, with relevant newlines and indent. "
  (newline)
  (indent-according-to-mode)
  (forward-line -1)
  (indent-according-to-mode))

(defun gp/sp/release-newline-post-command ()
  "Remove the hook and reset the post-command count."
  (remove-hook 'post-command-hook 'gp/sp/await-newline-post-command)
  (setq gp/sp/post-command-count 0))

(defun gp/sp/await-newline-post-command ()
  "If command is newline, indent and enter sexp."
  (if (> gp/sp/post-command-count 1)
      (gp/sp/release-newline-post-command)
    (progn
      (setq gp/sp/post-command-count (+ gp/sp/post-command-count 1))
      (when (memq this-command
                  '(newline newline-and-indent reindent-then-newline-and-indent))
        (gp/sp/release-newline-post-command)
        (gp/sp/create-newline-and-enter-sexp)))))

(defun gp/sp/await-newline (id action context)
  (when (eq action 'insert)
    (add-hook 'post-command-hook 'gp/sp/await-newline-post-command)))

(sp-with-modes '(csharp-mode js-mode awk-mode)
  (sp-local-pair "(" nil :post-handlers '(:add gp/sp/await-newline))
  (sp-local-pair "{" nil :post-handlers '(:add gp/sp/await-newline))
  (sp-local-pair "[" nil :post-handlers '(:add gp/sp/await-newline)))

Of course, anything but gp/sp/create-newline-and-enter-sexp and the list to check (newline newline-and-indent reindent-then-newline-and-indent) could be nicely abstracted into smartparens. How about a keyword :post-handlers-if, and a pair of a list (of triggering commands) and what to do then? That's a very generic way, which I generally love.

@Fuco1

This comment has been minimized.

Show comment
Hide comment
@Fuco1

Fuco1 Jun 1, 2013

Owner

So :post-handlers-if would mean "if the next command is X, run this function", right? So basically the same as #80 (comment)

Maybe it can be just added to :post-handlers as usuall, but if the form there is a list (and not lambda) interpret it in this "conditional" way). Though that will make the interface less intuitive, probably.

Would it ever be useful to wait more than for the next command?

I have a bit of time today so I guess I can give it a shot.

Owner

Fuco1 commented Jun 1, 2013

So :post-handlers-if would mean "if the next command is X, run this function", right? So basically the same as #80 (comment)

Maybe it can be just added to :post-handlers as usuall, but if the form there is a list (and not lambda) interpret it in this "conditional" way). Though that will make the interface less intuitive, probably.

Would it ever be useful to wait more than for the next command?

I have a bit of time today so I guess I can give it a shot.

@Fuco1

This comment has been minimized.

Show comment
Hide comment
@Fuco1

Fuco1 Jun 4, 2013

Owner

Ok, I think it's fairly solid now ;P Read the built-in docs and tell me if it's understandable. Here's an example usage:

(sp-local-pair 'c++-mode "{" nil :post-handlers '((my-create-newline-and-enter-sexp "RET")))

(defun my-create-newline-and-enter-sexp (&rest _ignored)
  "Open a new brace or bracket expression, with relevant newlines and indent. "
  (newline)
  (indent-according-to-mode)
  (forward-line -1)
  (indent-according-to-mode))
Owner

Fuco1 commented Jun 4, 2013

Ok, I think it's fairly solid now ;P Read the built-in docs and tell me if it's understandable. Here's an example usage:

(sp-local-pair 'c++-mode "{" nil :post-handlers '((my-create-newline-and-enter-sexp "RET")))

(defun my-create-newline-and-enter-sexp (&rest _ignored)
  "Open a new brace or bracket expression, with relevant newlines and indent. "
  (newline)
  (indent-according-to-mode)
  (forward-line -1)
  (indent-according-to-mode))
@ilohmar

This comment has been minimized.

Show comment
Hide comment
@ilohmar

ilohmar Jun 7, 2013

Works very nicely and is highly flexible --- I like it! Thanks so much again for this awesome package.

ilohmar commented Jun 7, 2013

Works very nicely and is highly flexible --- I like it! Thanks so much again for this awesome package.

@Fuco1

This comment has been minimized.

Show comment
Hide comment
@Fuco1

Fuco1 Jun 7, 2013

Owner

Great. I'm gonna close this one up then. For some issues introduced by this, start a new thread.

If you have some general ideas/comments we can continue here with an "off topic" discussion.

Owner

Fuco1 commented Jun 7, 2013

Great. I'm gonna close this one up then. For some issues introduced by this, start a new thread.

If you have some general ideas/comments we can continue here with an "off topic" discussion.

@rdallasgray

This comment has been minimized.

Show comment
Hide comment
@rdallasgray

rdallasgray Jun 13, 2013

Sorry to be slow off the mark Matus -- this is an awesome solution and works really well. I've incorporated it in Graphene.

rdallasgray commented Jun 13, 2013

Sorry to be slow off the mark Matus -- this is an awesome solution and works really well. I've incorporated it in Graphene.

@raxod502

This comment has been minimized.

Show comment
Hide comment
@raxod502

raxod502 Dec 10, 2017

Contributor

@Fuco1 This functionality is great! Are there any plans to enable it by default, or add a user option to make it more obvious how to get the behavior (without reading this thread)?

Contributor

raxod502 commented Dec 10, 2017

@Fuco1 This functionality is great! Are there any plans to enable it by default, or add a user option to make it more obvious how to get the behavior (without reading this thread)?

@Fuco1

This comment has been minimized.

Show comment
Hide comment
@Fuco1

Fuco1 Dec 11, 2017

Owner

We can set it up for some of the languages in their default configurations; and some of them already have a setup like this. I would not go with the global flag but leave it on a per-langauge basis at least. We can have defcustoms for each language probably.

I like it when software works out of the box but I'm very afraid to implement those defaults myself, especially in Emacs, where people always expect something else (no two people seem to share a workflow :D). I'll be happy to merge patches if some sort of minimal consensus is reached somehow.

Owner

Fuco1 commented Dec 11, 2017

We can set it up for some of the languages in their default configurations; and some of them already have a setup like this. I would not go with the global flag but leave it on a per-langauge basis at least. We can have defcustoms for each language probably.

I like it when software works out of the box but I'm very afraid to implement those defaults myself, especially in Emacs, where people always expect something else (no two people seem to share a workflow :D). I'll be happy to merge patches if some sort of minimal consensus is reached somehow.

@raxod502

This comment has been minimized.

Show comment
Hide comment
@raxod502

raxod502 May 30, 2018

Contributor

By now, I have code covering a lot of major modes:

(defun radian-enter-and-indent-sexp (&rest _ignored)
  "Insert an extra newline after point, and reindent."
  (newline)
  (indent-according-to-mode)
  (forward-line -1)
  (indent-according-to-mode))

(dolist (mode '(c-mode c++-mode css-mode objc-mode java-mode
                       js2-mode json-mode
                       python-mode sh-mode web-mode))
  (sp-local-pair mode "{" nil :post-handlers
                 '((radian-enter-and-indent-sexp "RET")
                   (radian-enter-and-indent-sexp "<return>"))))

(dolist (mode '(js2-mode json-mode python-mode web-mode))
  (sp-local-pair mode "[" nil :post-handlers
                 '((radian-enter-and-indent-sexp "RET")
                   (radian-enter-and-indent-sexp "<return>"))))

(dolist (mode '(python-mode))
  (sp-local-pair mode "(" nil :post-handlers
                 '((radian-enter-and-indent-sexp "RET")
                   (radian-enter-and-indent-sexp "<return>")))
  (sp-local-pair mode "\"\"\"" "\"\"\"" :post-handlers
                 '((radian-enter-and-indent-sexp "RET")
                   (radian-enter-and-indent-sexp "<return>"))))

It seems kind of unwieldy, though, and I have to update it every time I use a new mode. Is there a way I can enable this for every pair?

Contributor

raxod502 commented May 30, 2018

By now, I have code covering a lot of major modes:

(defun radian-enter-and-indent-sexp (&rest _ignored)
  "Insert an extra newline after point, and reindent."
  (newline)
  (indent-according-to-mode)
  (forward-line -1)
  (indent-according-to-mode))

(dolist (mode '(c-mode c++-mode css-mode objc-mode java-mode
                       js2-mode json-mode
                       python-mode sh-mode web-mode))
  (sp-local-pair mode "{" nil :post-handlers
                 '((radian-enter-and-indent-sexp "RET")
                   (radian-enter-and-indent-sexp "<return>"))))

(dolist (mode '(js2-mode json-mode python-mode web-mode))
  (sp-local-pair mode "[" nil :post-handlers
                 '((radian-enter-and-indent-sexp "RET")
                   (radian-enter-and-indent-sexp "<return>"))))

(dolist (mode '(python-mode))
  (sp-local-pair mode "(" nil :post-handlers
                 '((radian-enter-and-indent-sexp "RET")
                   (radian-enter-and-indent-sexp "<return>")))
  (sp-local-pair mode "\"\"\"" "\"\"\"" :post-handlers
                 '((radian-enter-and-indent-sexp "RET")
                   (radian-enter-and-indent-sexp "<return>"))))

It seems kind of unwieldy, though, and I have to update it every time I use a new mode. Is there a way I can enable this for every pair?

@Fuco1

This comment has been minimized.

Show comment
Hide comment
@Fuco1

Fuco1 May 30, 2018

Owner

@raxod502 Unfortunately not, you can probably hack an advice together that would do this for you, but there is nothing built-in for a "global" configuration of these hooks.

Owner

Fuco1 commented May 30, 2018

@raxod502 Unfortunately not, you can probably hack an advice together that would do this for you, but there is nothing built-in for a "global" configuration of these hooks.

@raxod502

This comment has been minimized.

Show comment
Hide comment
@raxod502

raxod502 May 31, 2018

Contributor

Would you be open to the idea of configuring pairs globally? This seems like it would be a very useful feature, and you suggested the same in your comment above.

Contributor

raxod502 commented May 31, 2018

Would you be open to the idea of configuring pairs globally? This seems like it would be a very useful feature, and you suggested the same in your comment above.

@Fuco1

This comment has been minimized.

Show comment
Hide comment
@Fuco1

Fuco1 May 31, 2018

Owner

@raxod502 There's a proposition for a new configuration API https://github.com/Fuco1/smartparens/blob/master/dev/sp-defpair.org which has been sitting around for quite some time. I think there is also some support for global config there.

The very early versions of SP had global configuration but we decided to remove it because it very soon became apparent that it's not going to really work universally... and we opted for configuring everything for every mode/pair explicitly.

We might want to add some API to do this in a less annoying way.

Owner

Fuco1 commented May 31, 2018

@raxod502 There's a proposition for a new configuration API https://github.com/Fuco1/smartparens/blob/master/dev/sp-defpair.org which has been sitting around for quite some time. I think there is also some support for global config there.

The very early versions of SP had global configuration but we decided to remove it because it very soon became apparent that it's not going to really work universally... and we opted for configuring everything for every mode/pair explicitly.

We might want to add some API to do this in a less annoying way.

xenodium added a commit to xenodium/dotsies that referenced this issue Aug 16, 2018

A tiny editing smartparens treat
blabla {|}

=>

blabla {
  |
}

source: Fuco1/smartparens#80
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment