hvesalai / emacs-scala-mode

The definitive scala-mode for emacs
http://ensime.org
GNU General Public License v3.0
361 stars 68 forks source link

resurrect scala-indent:indent-on-parentheses #118

Closed hvesalai closed 8 years ago

hvesalai commented 8 years ago

This commit removed our own scala-indent:indent-on-parentheses handling in favour of using smarparens but never added documentation on how to achieve the same functionality.

hvesalai commented 8 years ago

618f0fbef4d3d94156f834a66e36c79f07a54e02

hvesalai commented 8 years ago

The functionality removed was:

(defun scala-indent:indent-on-parentheses ()
  (when (and (= (char-syntax (char-before)) ?\))
             (= (save-excursion (back-to-indentation) (point)) (1- (point))))
    (scala-indent:indent-line)))

What in effect indents the closing parenthesis when inserted on an empty line. The added documentation:

  (sp-local-pair 'scala-mode "(" nil :post-handlers '(("||\n[i]" "RET")))
  (sp-local-pair 'scala-mode "{" nil :post-handlers '(("||\n[i]" "RET") ("| " "SPC")))

does something completely different.

fommil commented 8 years ago

Ok, I thought it did the same thing. Maybe I've got some other mode/setting active that does the indentation when I insert parens that I didn't document properly. I'm pretty sure smart parens can handle this.

If we need to add it back in, I'd like to avoid adding it to post-self-insert-hook as it causes performance problems and conflicts with other modes... it's easy enough for somebody to manually add it as part of their scala-mode-hooks.

hvesalai commented 8 years ago

I'm fine with just adding documentation on how to do this with smartparens. Just can't figure it out my self.

fommil commented 8 years ago

with smart parens, type { then RET and you should end up with

{
  <point>
}

all three lines are indented for me. Maybe not the first line, come to thing of it.

hvesalai commented 8 years ago

Yes, but I don't want that. I want it to be as it was before so that with

foo { RET I get

foo {
  <point>

And then if I continue with bar RET }

I get

foo {
  bar
}<point>
hvesalai commented 8 years ago

I.e. I don't want any of the automatic insertion of parens.

hvesalai commented 8 years ago

It seems to me smartparens can't do it so I'd say we have to revert to the code as it was before. I'd like to understand the performance problem though. I can't see any reason why it would affect performance since it is a very small piece of code (when compared to, for example, smartparens).

fommil commented 8 years ago

OK, let's add the function back in but please don't add it to the post-self-insert-hook... that seems like a personal preference and adding anything in there is a known performance problem as it gets called on every keystroke.

fommil commented 8 years ago

btw, isn't this a sort of general function that other modes could benefit from? if you replace scala-indent:indent-line with indent-line doesn't it work everywhere?

hvesalai commented 8 years ago

I still don't see the problem with post-self-insert-hook. If you are happy with smartparens having its self insert hook (it is really complex), then I don't see why scala-mode could have it's two-liner there. Yes, it is a general function, but I would like to see scala-mode working with "sane defaults" out of the box, without having to install tens of other modes to support basic code editing.

fommil commented 8 years ago

if that's what you want. I can always add a hook to remove it from my config. I already do this for many post-self-insert-hook functions.

fommil commented 8 years ago

incidentally, what does this functionality achieve that electric indent doesn't do? That's part of stock emacs https://www.emacswiki.org/emacs/AutoIndentation

I think most people would be surprised that they get some sort of electic indent without having enabled it.

hvesalai commented 8 years ago

The function we had only indented on } in the very specific case that it was inserted onto an empty line. As such it is much less interfering than any of various auto paren modes or electric.