search

syl20bnr / spacemacs

A community-driven Emacs distribution - The best editor is neither Emacs nor Vim, it's Emacs *and* Vim!
http://spacemacs.org
GNU General Public License v3.0
23.69k stars 4.89k forks source link

LSP over-riding long established Clojure major mode key bindings #14400

Closed practicalli-johnny closed 1 year ago

practicalli-johnny commented 3 years ago

Description :octocat:

LSP over-riding long established key bindings for the Clojure major mode

Reproduction guide 🐞

Observed behaviour: :eyes: :broken_heart: , T s should toggle evil-cleverparens and has been over-ridden by lsp , h h should open cider-docs but is over-ridden by lsp docs (and links to src do not work in lsp)

Further investigation of keybindings is underway.

Expected behaviour: :heart: :smile: LSP should not over-ride established CIDER key bindings even when using LSP, especially where the functionality of the replacing functions is different or not as comprehensive.

System Info :computer:

Backtrace :paw_prints:

<<BACKTRACE IF RELEVANT>>
smile13241324 commented 3 years ago

I am not sure how this should be done, the whole idea behind LSP is to have common functionality behind the same bindings which work like interfaces. The respective implementation of this is entirely done by the respective LSP server implementation which the user can freely choose.

In otherwords if functionality for the help buffer is not as comprehensive as what cider provides this may only be true for the respective server. An alternative one may deliver this function. The same goes for the bindings as they are the same for every mode. This binding , T s is simply already taken so we cannot have this overwritten by specific layers without loosing general functionality.

Instead the toggle must be moved to a new binding where it does not conflict with the existing LSP toggles. I would suggest , T E for structural editing in case LSP is used.

About cider-doc what about moving it to , h H when LSP is installed? This would still allow to have the old functionality available when LSP is used.

For the missing LSP server feature for the help buffer I would suggest to open a PR upstream i.e. the server impl.

smile13241324 commented 3 years ago

Funny I can't see where , T s is bound, does this come from the package itself?

practicalli-johnny commented 3 years ago

As LSP has to be the only way for tooling now, the only choice is to add new binding for commands that LSP is replacing

I have tried to avoid upper case bindings but don't have any suggestions at present. I will have a think about what could be used instead.

practicalli-johnny commented 3 years ago

For evil-cleverparens, , T c is available and matches the package name. I suggest this should just be changed regardless of if LSP is running. This will avoid confusion of having two different key bindings for the same function depending on if LSP is enabled.

I dont think adding a , h H key binding when using lsp is useful, its just going to confuse things if there are two different key bindings for the same command.

When someone uses LSP they will have to get used to the changes or not use it (or bind their own shortcuts).

I will raise an issue on lsp, probably lsp ui package, to see if they can add the missing functionality for docs - in that it doesnt follow the file name and open the source.

I guess these things are to be expected in the rush to add LSP when the tooling around it is still quite new. The only choice for Clojure is to use LSP or not use it, as their is only one implementation. At least that implementation is getting some much needed work thanks to the Calva team and others.

practicalli-johnny commented 3 years ago

It seems the safe structural editing affects numerous languages in Spacemacs, which all use the same key binding as that was always a principle of Spacemacs.

So I assume the maintainers of all these languages need to agree what to do for the leader T s key binding across all these languages.

You will need to decide if the existing key binding or the new LSP key binding should be kept. Its a shame the toggles were not put under a leader T l sub menu to avoid clashes

lebensterben commented 3 years ago

, T in LSP needs to be reworked. It's not mnemonic even without the issues you mentioned.

smile13241324 commented 3 years ago

If we have so much layers affected maybe we should move the LSP toggle bindings to a prefix then, moving the LSP toggles to the mentioned prefix of , T l for toggles -> lsp may be a good idea.

@lebensterben I am not sure about the non mnemonic thing, conventions also say T for toggles for major mode specific ones. Whats your suggestion for an alternative binding?

lebensterben commented 3 years ago

I mean bindings under , T.

smile13241324 commented 3 years ago

@jr0cket I have moved the LSP toggle bindings from SPC m T to SPC m T l for toggle -> lsp now.

If we have that much affected layers I think the damage we do with insisting on all these layers change their known bindings is higher than the damage we do if we move some rarely used LSP specific toggles to a new prefix. Also T l is definitely clearer as this prefix will only hold LSP specific settings and SPC m T may contain random mode specific toggles.

lebensterben commented 3 years ago

The real issue of LSP comes from lsp-ui in particular (and also some UI features in main lsp-mode package).

There are too many "toggles" for various UI features, and we haven't added all of them yet.

github-actions[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Please let us know if this issue is still valid!