search

emacs-lsp / lsp-treemacs

lsp-mode :heart: treemacs
GNU General Public License v3.0
400 stars 47 forks source link

Scala Metals Treeview #13

Closed dsyzling closed 4 years ago

dsyzling commented 5 years ago

Currently this implementation requires a custom-capabilities feature added to lsp-mode which I can add as a separate pull request or we could just move ahead with a hook implementation.

dsyzling commented 5 years ago

Thanks @yyoncho I've incorporated your suggestions along with workspace switching.

One question on the modeline, the current implementation uses the Metals view name to set the text on the mode line. I've tried to keep this decoupled - i.e. I don't know what the view names are or how many I may be sent. When using define-derived-mode can this be accommodated? So not until I have created the buffer and treemacs treeview will I know the name of the view. It appears the text for the modeline is defined within the macro arguments passed to define-derived-mode? Or would I use define-derived-mode along with a var to define the keymap and continue to use setq-local for mode-line-format?

yyoncho commented 5 years ago

Sorry, I don't know... I just pointed out the @kurnevsky's PR because I thought that you have used the old code as an example. If it is not trivial how to achieve that let's keep it as it is for now.

kurnevsky commented 5 years ago

I'll look into it at the weekend.

dsyzling commented 5 years ago

Thanks - before it can be tested in full - I need to address how to modify the client capabilities sent to Metals. Now my initial solution was to modify lsp-mode with a new lsp--client field containing client capabilities which are modified and blended before being sent. I can raise a PR for lsp-mode unless you wish to move down the hook solution rather than add a further field to the lsp--client struct?

yyoncho commented 5 years ago

Adding the field works for me. I am not sure I understand the other option but since you have the code go ahead and open a pr.

dsyzling commented 5 years ago

Added the lsp-mode PR here: emacs-lsp/lsp-mode#1013

dsyzling commented 5 years ago

Thanks @kurnevsky I'll work through these hopefully the early part of this week.

dsyzling commented 4 years ago

I've updated the code to resolve most of these issues. I currently still need the minor mode because I need to supply my own customised keymap to override q to quit the treeview and close all of the buffers associated with the views (build/compile). The default treemacs behaviour closes the current buffer only.

yyoncho commented 4 years ago

@kurnevsky do you have more comments or we are ready to merge it?

kurnevsky commented 4 years ago

How about taking icons from my PR https://github.com/emacs-lsp/lsp-treemacs/pull/11 ?

Besides that I'm still not sure we should rely on metals first send treeViewDidChange to initialize our state and show trees to user (as help isn't included there and therefore isn't shown). But it can be reconsidered later.

dsyzling commented 4 years ago

@kurnevsky Yes I can definitely take the icons from the PR you suggested.

I think we should definitely consider the change - like I said the original reason for the design was Metals sending the views with their ids and names in the first place without them being documented in the 'spec' - i.e. it appeared that Metals should be in control of telling the editor what views were available. I agree the help is not included and maintained separately the two cases don't gel in a cohesive design decision.

So I agree let's think about this in future and whether we can then provide an optional feature to include within a single treemacs tree if the user wishes to view projects this way.

dsyzling commented 4 years ago

Added updated icons from @kurnevsky PR #11.

dsyzling commented 4 years ago

I would note the current directory for the icon locations is in a different place between PR #11 and this one - icons here are in icons/metals whilst PR #11 uses icons/vscode/metals. So we should decide on a location so we could merge both PRs.

kurnevsky commented 4 years ago

So we should decide on a location so we could merge both PRs.

I'm fine with both locations but I would suppose not to merge my PR - this one empliments much more complete. The only thing I'd add - ability to use treemacs projects extensions but it can be done later with another PR :)

dsyzling commented 4 years ago

@yyoncho Fixed a minor bug and pushed the changes. Any preference on location of metals icons, currently they are in icons/metals, would you prefer icons/vscode/metals ?

Can we then merge this PR?

yyoncho commented 4 years ago

I do not have a preference for the icons location ATM. I could merge the PR, but still it needs readme section. It could be added afterwards.

dsyzling commented 4 years ago

I've added some changes to the lsp-treemacs README - if you could take a quick look through and see if you're happy with the additions.

yyoncho commented 4 years ago

Looks good. One thing that I noticed on the gif - the fact that it splits the window the second time you open a file from the view - you may avoid that by doing the same thing as lsp-treemacs-open-file.

I am proceeding with the merge. Next, we will merge the @kurnevsky implementation.

Thank you for the great work!

yyoncho commented 4 years ago

One more thing: do you have a tweeter account? I usually tweet for the new lsp-mode features and I want to link your account.

dsyzling commented 4 years ago

yes my account is '@dsyzling'

yyoncho commented 4 years ago

Ah, ok - if you want you may tweet I will retweet.