Skip to content
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

Terrible ESC key behaviour with company popup #1372

Closed
trishume opened this issue Apr 25, 2015 · 104 comments
Closed

Terrible ESC key behaviour with company popup #1372

trishume opened this issue Apr 25, 2015 · 104 comments

Comments

@trishume
Copy link
Contributor

Currently in develop since cdd1edd pressing escape while a popup is open will abort the company popup but not exit insert mode. This is terrible user-unfriendly behaviour that kills the ability to use muscle memory. See more discussion in the comments on the commit.

The way that #1097 should be fixed is that pressing escape both exits company and exits to normal mode.

@syl20bnr
Copy link
Owner

I agree that a sane default for auto-completion is to use TAB.

auto-complete has a better UX in my opinion and I would like to replicate it in company but I'm not sure it is possible :-( I explain. In auto-complete pressing TAB while there are several candidates for the same prefix will auto-complete the prefix. Is this possible to do it in company ? I believe I saw an issue upstream where the maintainer said it is not the company philosophy (warning, I'm not sure at all about this) which would be really sad.
I never had issue with auto-complete which as natural way to do auto-complete, with company I struggle.
We even have inconsistent behavior with esc :-(

@sooheon
Copy link

sooheon commented Apr 27, 2015

I think autocompletion is a very personal thing, and there may be less benefits to having one person's customisations being the spacemacs default. That said, what you want @syl20bnr, can be done by setting:

      (define-key company-active-map [tab] 'company-complete-common)
      (define-key company-active-map (kbd "TAB") 'company-complete-common)
      (define-key company-active-map (kbd "<tab>") 'company-complete-common)

@syl20bnr
Copy link
Owner

@sooheon ❤️
Yes this is what I want as default for spacemacs, looks natural to me because it follow the incremental nature of auto-completion.

@syl20bnr
Copy link
Owner

Ok what auto-complete does which is very good is that once TAB has completed the common prefix it will cycle between the candidates, THAT is good UX.
Can we do this with company ?

Ultimately if we can achieve consistency between auto-complete and company it would be perfect.

syl20bnr added a commit that referenced this issue Apr 27, 2015
@syl20bnr
Copy link
Owner

It is pushed in develop. I also updated evil-escape to remove the support for company.

@syl20bnr
Copy link
Owner

Ok you'll find me weird but I put RET back to match auto-complete behavior.
So here it is:

  • tab expand the prefix and cycle between candidates
  • S-tab same thing but cycle backward
  • RET actually does the auto-completion

I find this UX very good because it is symmetric and does not involve the hands to quit the home row.

soooo we still have the issue with RET but the behavior is the same in both completion engine.

I think the real issue with RET is to find why some users have #1097 (including me).

@syl20bnr
Copy link
Owner

@mijoharas I removed the variable to use tab to complete instead of RET I will put it back, sorry for this.

@syl20bnr
Copy link
Owner

Spamming this issue :-)

An alternate proposal would be:

  • tab perform completion
  • S-tab complete up to common prefix and cycle
  • S-M-tab complete up to common prefix and cycle backward

So we get rid of RET and have the quick cycle on a single touch.

@syl20bnr
Copy link
Owner

Trying it now :-)

@syl20bnr
Copy link
Owner

Ok forget this, it is awful !

@sooheon
Copy link

sooheon commented Apr 27, 2015

What I have is tab perform complete common, and that's it. RET does nothing, because it can fuck with newlines. I also enable company-show-numbers, so if I ever want to put something in right away, it's M-number. I think this is the sanest because it doesn't mess with RETurn when I don't want it to.

@mijoharas
Copy link
Contributor

@syl20bnr I also prefer not to have RET complete due to newline issues (like the stuff with do in ruby that @trishume got around with a bit of brilliant hackery 😄 ). Can I suggest Shift-Ret as default? or maybe just have an option to remove the Ret completion behaviour?

@mijoharas
Copy link
Contributor

just to clarify, my suggestion is tab is complete common, Shift-Ret (or C-Ret?) would be complete and then just use C-j and C-k to scroll up and down like normal.

@mijoharas
Copy link
Contributor

I also think we should swap C-/ and C-M-/ as I think filter is more useful than just search with the company dropdown, so should be the default option (was just thinking that the other day)

@trishume
Copy link
Contributor Author

Definitely there should be an option to disable RET for full completion. Maybe it would use Shift-TAB for complete fully.

Also note that TAB has collision issues too (yasnippet, indentation, org) so using both RET and TAB is the worst of both worlds in that regard.

@syl20bnr
Copy link
Owner

Indeed TAB + RET is the worth :-)
It is easier to suppress the issue with TAB though:

  • We can remove the collision with yasnippet by changing the key binding for it.
  • it is less common to use tab for indent inside a sentence since space is generally used and we have auto-indentation when pressing RET

I like the single key to complete common prefix+cycle, feels natural. I propose the remove RET by replacing it with C-l or something on the home row. I can also experiment with something like jl pressed quickly.

@tuhdo
Copy link
Contributor

tuhdo commented Apr 27, 2015

We can already use M-/ to expand snippets already.

@mijoharas
Copy link
Contributor

I agree with you on the complete-common, now that I use it, I think it is a good feature. I also agree that we need a better way to complete than RET.

My vote is for C-RET just because that feels somewhat natural to me. I don't have a strong opinion though. I think Shift-Tab might be confusing as people may try and do that to scroll back up the list, but it could be a solution. C-l could be ok, do we use it for anything else often?

I would probably not be too keen on jl as (since I'm on dvorak) I wouldn't find that too natural.

@syl20bnr
Copy link
Owner

I find it slow to move the finger from TAB to CTRL or even SHIFT.
The sequence jl can be customizable. This way it is really quick to TAB TAB TAB jl since it involves both hands without any combination.

@CestDiego
Copy link
Contributor

@tuhdo I dindn't know M-/ was used to expand snippets! :O :( does this go into the doc? Anyway I think that TAB is more natural for expanding snippets ans whatnot. What is really awful though is when snippets get auto-completed and you get the name of the snippet expanded and not the snippet itself.

@syl20bnr Please consider that in linux S-TAB ( that is Shift+TAB ) is interpreted as <backtab> in some linux, and also Ctrl+shift+tab is <C-iso-lefttab> and also M-S-TAB is <M-iso-lefttab> this is very weird..but...it's emacs! :D

@CestDiego
Copy link
Contributor

@syl20bnr just to for the sake of discussion. Also, what's wrong with having C-j and C-k to navigate the current candidates of completion I used to have to select with this guys. and then to TAB to expand the completion.

EDIT: Right now I love that I can C-j or C-k to any candidate and company shows me if that is a snippet for example def for the defun snippet here
defun

And so if I want to expand that snippet I just press and that's it... obviously pressing C-l would be even cooler but other than that I'm happy with the current setup.

@mijoharas
Copy link
Contributor

hmmm... still not sure on jl, my vote would still be for C-Ret or C-l (having the key-chord in addition to both of those would still be good for me). Obviously it's something I could easily rebind in my setup so not a big deal either way.

@CestDiego what you're suggesting is exactly what I had, but after playing around with complete-common and thinking about it a bit today, I quite like binding that to tab. (I assume we'd still keep the C-j and C-k binding regardless of what's decided on, right @syl20bnr ?). Obviously it's a matter of preference, but I think it works quite nicely.

@CestDiego
Copy link
Contributor

@mijoharas ye I really didn't try the complete-common, but now I see it's value. if we have the tab/shift+tab and C-j/C-k combo that would be so awesome. But also have in mind @syl20bnr that in linux<C-RET> is<C-return>

@CestDiego
Copy link
Contributor

Great! :) gonna test it now. I don't feel right about the jk thing I think it should reuse sth like key-chord or sth but I don't know how is that implemented anyway. BTW I guess there are people that have already replaced fd for jk in evil escape so maybe a warning should be made as welll (although I remember there being an issue with evil escape with hjkl keys. Anyway will report back :)

@syl20bnr
Copy link
Owner

key-chord has some very nasty edge cases with evil, I was able to reliably crash emacs with it enabled. There is also some issue with the prefix arguments.

@CestDiego
Copy link
Contributor

Oh I didn't know that...lol if this works then it's ok then :)

@sooheon
Copy link

sooheon commented Apr 30, 2015

@syl20bnr regarding your reasoning for C-j and C-k keybindings, are you aware that M-n and M-p does the same by default? When working in lisps, many people often use C-j in both normal and insert mode for splitting up lines, similarly to enter. C-k I'm sure you know is another essential smartparens/paredit/lispy convention for killing the rest of the sexp. Binding those to have different behaviour only during completion would give us the same problems as with having return complete.

Edit: I also would like to put in a word for the behaviour of complete-common only. It's good in that tab isn't too smart and only does the same thing every time (after you complete common you type another character or two and complete the rest). This works especially well with company-pseudo-tooltip-unless-just-one-frontend company-preview-if-just-one-frontend, which makes it visually clear when tab will complete the whole word and when it will complete common.

@CestDiego
Copy link
Contributor

I didn't understand the problems with RET. What are them exactly? I didn't
know about m-c and M-p just tested them and at first glance they look kinda
good.

That being said I wouldn't be mad if the completions used M-n and M-p for
navigating... I have one issue with the company pop-up though,, sometimes
when there is not enough space at the bottom the popup will appear on top
of the text, but the functions that go to the 'next' candidate go up
instead of down, and the same inverted behavior with 'prev' candidate :(
it's not very intuitive visually even though it makes sense programatically.

On Thu, Apr 30, 2015 at 1:43 AM, Sooheon Kim notifications@github.com
wrote:

@syl20bnr https://github.com/syl20bnr regarding your reasoning for C-j
and C-k keybindings, are you aware that M-n and M-p does the same by
default? When working in lisps, many people often use C-j in both normal
and insert mode for splitting up lines, similarly to enter. C-k I'm sure
you know is another essential smartparens/paredit/lispy convention for
killing the rest of the sexp. Binding those to have different behaviour
only during completion would give us the same problems as with having
return complete.


Reply to this email directly or view it on GitHub
#1372 (comment).

@sooheon
Copy link

sooheon commented Apr 30, 2015

@CestDiego RET in insert mode should give you a new line. Your muscle memory expects it. If you're typing without looking at the cursor, and press RET for a newline but company popup appeared, it would give you a completion when you don't want it. It forces you to pay attention to the state of company popup, and do things like esc and then RET.

regarding the completion order, you can set company-tooltip-flip-when-above nil, and the direction will be what you expect (but then its location changes. It's a compromise.).

@CestDiego
Copy link
Contributor

Oh ye that happened to me .... well I guess you are right with the C-j and
C-k then...I use them a lot in web-mode.... @syl20bnr maybe we should
reconsider this.

On Thu, Apr 30, 2015 at 1:53 AM, Sooheon Kim notifications@github.com
wrote:

@CestDiego https://github.com/CestDiego RET in insert mode should give
you a new line. Your muscle memory expects it. If you're typing without
looking at the cursor, and press RET for a newline but company popup
appeared, it would give you a completion when you don't want it. It forces
you to pay attention to the state of company popup, and do things like esc
and then RET.


Reply to this email directly or view it on GitHub
#1372 (comment).

@sooheon
Copy link

sooheon commented Apr 30, 2015

Continuing on the M-n vs C-j, I see why you would like to hold down C, move with j or k, and complete with l. It's a common vi idiom. However, I think you'd get nearly the same experience changing that to M-l for complete (or any other M binding) and using M-n/p for nav. Adding company-show-numbers only makes this more consistent, as numbered selections are completed with M-num.

Edit: it occurs to me that @tuhdo's suggestion for M-/ also fits here perfectly. Fewer reinventing the wheel for ux when there are package defaults and emacs defaults for them.

@cpaulik
Copy link
Contributor

cpaulik commented Apr 30, 2015

I have to say that I do not like C-l for completion because it is very inefficient as @syl20bnr already said to go from tab to ctrl with your left pinky.

Why not use shift-tab to cycle and keep tab on complete. One can easily press the right-shift while keeping the left pinky on the tab button.

These bindings worked for me on linux for both GUI and Termnal emacs:

      ;; key bindings
      (define-key evil-insert-state-map "k" 'spacemacs//company-complete-end)
      (let ((map company-active-map))
        ;; use TAB to auto-complete instead of RET
        (define-key map (kbd "j") 'spacemacs//company-complete-start)
        (define-key map [return] 'nil)
        (define-key map (kbd "RET") 'nil)
        (define-key map [tab] 'company-complete)
        (define-key map (kbd "TAB") 'company-complete-selection)
        (define-key map (kbd "<tab>") 'company-complete-selection)
        (define-key map (kbd "C-/") 'company-search-candidates)
        (define-key map (kbd "C-M-/") 'company-filter-candidates)
        (define-key map (kbd "C-d") 'company-show-doc-buffer)
        (define-key map (kbd "C-j") 'company-select-next)
        (define-key map (kbd "C-k") 'company-select-previous)
        (define-key map (kbd "C-l") 'company-complete-selection)
        (define-key map (kbd "<backtab>") 'company-complete-common-or-cycle)
        (define-key map (kbd "S-C-i") 'company-complete-common-or-cycle))

As a side note C-l does the exact opposite in helm - going back so we might want to look at that for consistency if C-l is kept to complete.

@sooheon
Copy link

sooheon commented Apr 30, 2015

@cpaulik C-l in helm is a whole other problem, I agree it should go forward and to the right (like in ido), not up a directory. I have to say that tab and shift-tab not being pure reversible actions of each other is unintuitive. Additionally, C-j and C-k have the problems described above, and I'm not sure how binding "j" and "k" to those functions would work when you just want to type those letters?

@cpaulik
Copy link
Contributor

cpaulik commented Apr 30, 2015

Also in the last develop I can no longer expand snippets with tab. Is it on purpose that this is only possible with M-/ now?

@sooheon
Copy link

sooheon commented Apr 30, 2015

If you read above syl20bnr has said that he took out yasnippet.

@cpaulik
Copy link
Contributor

cpaulik commented Apr 30, 2015

Ah sorry I missed that.

@mijoharas
Copy link
Contributor

Hmmm... I'm in favour of the C-j and C-k bindings as they are the same as in helm...

@PierreR
Copy link
Contributor

PierreR commented Apr 30, 2015

Just a little question. Does space complete the selection with an extra space ? Most of the time this is what I want (or mean) while completing the selection.

If I remember correctly this is enabled by (setq company-auto-complete t) so I guess it will still be possible to activate the feature ?

@PierreR
Copy link
Contributor

PierreR commented Apr 30, 2015

I'm also in favour of the C-j and C-k bindings for the reasons pointed by @mijoharas

I actually hope there will be an option to keep RET completing the selection for the same reason. That's what helm is doing. Personally I have yet to run in the issue described for RET so I don't see the problem. While selecting a completion it is quite clear I am in a different context so I never expect it to create a newline at that point. If I am not mistaken that's what most IDEs out there are doing ...

Apart from the remaining discussed complete selection I agree with all the other design decisions.

@tuhdo
Copy link
Contributor

tuhdo commented Apr 30, 2015

I actually hope there will be an option to keep RET completing the selection for the same reason. That's what helm is doing. Personally I have yet to run in the issue described for RET so I don't see the problem. While selecting a completion it is quite clear I am in a different context so I never expect it to create a newline at that point. If I am not mistaken that's what most IDEs out there are doing ...

I agree with you. I see no problem with RET and I think it's quite convenient and popular not only in Emacs but other IDEs out there.

@cpaulik
Copy link
Contributor

cpaulik commented Apr 30, 2015

We should have default behaviour that is what most people expect. Which is to complete either with tab or with RET

@syl20bnr
Copy link
Owner

The discussion goes in circle. :-)

I'll keep the current design with RET added for complete-selection.

Then I will add back the variable to use only TAB as complete-selection.

We have still to fix the issue where sometimes company is not disabled when
going back to normal state.

Le jeudi 30 avril 2015, Christoph Paulik notifications@github.com a
écrit :

We should have default behaviour that is what most people expect. Which is
to complete either with tab or with RET


Reply to this email directly or view it on GitHub
#1372 (comment).

-syl20bnr-

@mijoharas
Copy link
Contributor

complete-selection or complete-common? as isn't that the current situation? either way, sensible defaults, with maybe a section in the documentation for auto-completion layer to show people how to customize this to suit themselves as it seems most people want different things here 😄

@syl20bnr
Copy link
Owner

Same as it is in develop right now but with RET added for
complete-selection.

Plus a variable to use only TAB instead but with complete-selection instead
of complete-common-cycle. As said above complete-common alone does nothing
when the prefix is completed so it cannot be used in a TAB only setup.

Le jeudi 30 avril 2015, Michael Hauser-Raspe notifications@github.com a
écrit :

complete-selection or complete-common? as isn't that the current
situation? either way, sensible defaults, with maybe a section in the
documentation for auto-completion layer to show people how to customize
this to suit themselves as it seems most people want different things here [image:
😄]


Reply to this email directly or view it on GitHub
#1372 (comment).

-syl20bnr-

@trishume
Copy link
Contributor Author

I like this new design complete-common-or-cycle is awesome! I also like the idea of the jk chord.

There should definitely be a variable for use of RET because people that primarily use languages with semicolons mostly won't have problems with it but people who frequently use things like Ruby and Haskell will constantly have trouble with it.

This makes it sound like it should be language specific but that would kill muscle memory. So better to have a global preference (and maybe a toggle) and then keep around some strategies (like my old hack with keywords) that make it so that RET users can at least sometimes use ret-unfriendly languages like Ruby.

Also as a dvorak user, +1 for having a variable for the jk binding.

@syl20bnr
Copy link
Owner

syl20bnr commented May 4, 2015

I pushed the new design for auto-completion UX. I think it captures well all the stuff we said in this discussion.

You can check the documentation here:
https://github.com/syl20bnr/spacemacs/tree/develop/contrib/auto-completion#key-bindings

@syl20bnr
Copy link
Owner

syl20bnr commented May 9, 2015

I consider this issue fixed.

@1ambda
Copy link

1ambda commented Nov 16, 2016

How I can redefine ac-next and ac-previous in my .spacemacs?

I used this code but failed :(

(defun dotspacemacs/user-config ()
  "..."

  ;; evil
  ;;; http://ergoemacs.org/emacs/keyboard_shortcuts_examples.html
  (define-key evil-insert-state-map (kbd "C-p") nil)
  (define-key evil-insert-state-map (kbd "C-n") nil)

  ...

  ;; auto-complete (should be followed by evil key maps)
  (define-key map (kbd "C-n") 'company-select-next)
  (define-key map (kbd "C-p") 'company-select-previous)

)

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

No branches or pull requests

9 participants