lsp-treemacs icon indicating copy to clipboard operation
lsp-treemacs copied to clipboard

Add Metals Tree View Protocol support.

Open kurnevsky opened this issue 4 years ago • 16 comments

It's the easiest possible solution - just using treemacs project extension feature. Later it can be expanded to a separate buffer solution if necessary.

The only thing that left until it can be usable - adding treeViewProvider to experimental capabilities.

screenshot-2019-09-01-22:23:02

kurnevsky avatar Sep 01 '19 19:09 kurnevsky

Seems like @dsyzling was working on the same thing.

yyoncho avatar Sep 01 '19 19:09 yyoncho

Yes my code is currently in two branches - one for treemacs and one on the my lsp-mode fork to add custom client capabilities to enable the treeview provider for Metals:

https://github.com/dsyzling/lsp-mode/tree/custom-client-capabilities https://github.com/dsyzling/lsp-treemacs/tree/metals-treeview

This is currently with the autoload solution which dynamically adds handlers to the metals client and registers custom capabilities to enable the treeview provider.

Not sure how you want to handle this?

dsyzling avatar Sep 01 '19 20:09 dsyzling

@dsyzling I was thinking about adding treeViewProvider from lsp-metals.el by default and having a hook that lsp-treemacs will be using.

Do I understand it right - your code can handle only one project per buffer?

Also it looks like your icons aren't converted right from svg. I had to apply some fixes to them before conversion.

kurnevsky avatar Sep 01 '19 20:09 kurnevsky

@kurnevsky At the moment the idea would be to switch the treeview when you switch buffers between workspaces and metals instances - it would hide/show, the code is implemented but not activated - it needs to run on an idle timer when the user switches buffers. Yes the icons I initially took from vscode didn't have any letters within them - I was going to replace them later - they are pretty basic anyway - I think I might prefer straight vscode ones :-)

Yes not sure about the most convenient solution for registering capabilities - in the end I went with the option to choose to enable the treeview and do this via autoloads - after discussion with @yyoncho. But autoloads do complicate packaging.

dsyzling avatar Sep 01 '19 20:09 dsyzling

I also chose to split 2 windows - for compile and build - and had't included the other help items as yet. The remainder of the treeview protocol is implemented there - sends messages to indicate treeview is displayed/hidden along with items and updates showing compilation status.

Haven't implemented treeviewReveal as yet - there's a placeholder.

No themes or font customisation.

dsyzling avatar Sep 01 '19 20:09 dsyzling

Yes not sure about the most convenient solution for registering capabilities - in the end I went with the option to choose to enable the treeview and do this via autoloads - after discussion with @yyoncho. But autoloads do complicate packaging.

Activating treeViewProvider only means that we have to handle metals/treeViewDidChange and nothing more. So we can enable it from lsp-metals as an additional property to make-lsp-client and have a simple hook that will be called when we receive metals/treeViewDidChange.

At the moment the idea would be to switch the treeview when you switch buffers between workspaces and metals instances - it would hide/show, the code is implemented but not activated - it needs to run on an idle timer when the user switches buffers.

I'd prefer having all active lsp servers in one buffer like treemacs does it with projects. This way we don't have to redraw trees every time user switches the buffer. Also this allows to navigate all of them at once (might be convenient if you have a lot of microservices).

I also chose to split 2 windows - for compile and build - and had't included the other help items as yet.

Why not a single buffer for all of them? Having 3 buffers open at the same time might be overwhelming :)

kurnevsky avatar Sep 01 '19 21:09 kurnevsky

I'm confused are you suggesting that you would show one large tree with all projects for all workspaces you have open? So if you open Project A and Project B - entirely unrelated you would show these in the same tree? I'm not talking about projects within the same source tree - two possibly completely different projects? If that were the case I'd prefer to see the source and symbols related to the project I'm focus on - not navigate a tree with symbols unrelated to my current project/focus. Or am I missing something?

You would only need to show/hide the tree if you change to a buffer in another workspace - otherwise the current tree is related to the current workspace and is not changed or redrawn.

Why not a single buffer for all of them? Having 3 buffers open at the same time might be overwhelming :)

Purely a stylistic choice - I wasn't sure how many views metals would send in the future and whether the user would want to hide the build and/or compilation view separately (along with others). You can tell Metals which views have been displayed/hidden and Metals should not send updates to those views - optimising the client/server interaction slightly. Personally I might find the compilation status messages useful, but might not want to see the build tree - although I haven't added any code to configure this as yet. Personally I wouldn't want to see the help options - but if they were part of a tree they could be collapsed of course.

dsyzling avatar Sep 01 '19 21:09 dsyzling

The way I chose to implement treeViewDidChange was to decouple metals-treeview from lsp-mode - lsp-metals doesn't know about treeview at all or any of the treeview messages. If metals extends the treeview notifications - entirely possible - lsp-metals-treeview (within lsp-treemacs) would change not lsp-mode.

However there is a slight conflict here anyway - we now have a split implementation with lsp-metals inside lsp-mode and the treeview elsewhere - in my case the dependency is on lsp-mode providing certain features that are used to register the client caps and adding notification handlers dynamically to a known lsp client called 'metals. If that were to change the lsp treeview would break of course. Tests could be put in place to check the interface assurances and that a client called metals still exist.

I don't have really strong thoughts either way - just the way I tried to decouple the code given the constraints of implementing the tree within lsp-treemacs.

dsyzling avatar Sep 01 '19 21:09 dsyzling

Just to qualify my point on projects - I should be more precise because that term is overloaded - and clearly given the tree - which has projects I'm confusing matters :-)

If I have fs2 and zio locally - I open a file in fs2 - the fs2 projects and treeview will be displayed. If I then open another file under fs2 the treeview doesn't switch or get redrawn. However if I open a file in zio then the treeview will be switched to show the zio treeview with its projects and structure.

dsyzling avatar Sep 01 '19 21:09 dsyzling

I think that we could support both ways of displaying the treeview and the users could pick the one that fits his/her workflow.

yyoncho avatar Sep 02 '19 03:09 yyoncho

I'm confused are you suggesting that you would show one large tree with all projects for all workspaces you have open?

Yes, you got it right :) It's the same way treemacs works with projects, see: screenshot-2019-09-02-08:48:31

With this approach we don't need any loops with timers timers at all (though we can have an optional one to constantly go to the active file, like treemacs-follow-mode). It should be much clearer and easy to implement, don't you think so?

If I have fs2 and zio locally - I open a file in fs2 - the fs2 projects and treeview will be displayed. If I then open another file under fs2 the treeview doesn't switch or get redrawn. However if I open a file in zio then the treeview will be switched to show the zio treeview with its projects and structure.

This way you will loose tree state for closed projects while treemacs itself never looses it, even when a project is folded automatically. Also this will be the case when you open not a scala file in your current project - the tree will disappear. And as for me I hate when such things happen :)

That was one of the reasons I went with treemacs project extension - it's easy to implement and I can navigate all my projects where I launched metals.

kurnevsky avatar Sep 02 '19 05:09 kurnevsky

This way you will loose tree state for closed projects while treemacs itself never looses it, even when a project is folded automatically. Also this will be the case when you open not a scala file in your current project - the tree will disappear. And as for me I hate when such things happen :)

This usability issue is present for symbols view as well - I have prototype code which persists and restores the state.

I think that both solutions have their pros and cons. E. g. some users dont want to have a project explorer but they would like to do a quick peek from time to time - others prefer having sidebar view visible all the time. My proposal is to have both options available and focus on what will be the best plan to make that happen. @kurnevsky @dsyzling what do you think?

yyoncho avatar Sep 02 '19 06:09 yyoncho

@yyoncho both options might be nice as long as it doesn't over complicate the code or lead to issues interacting with Metals. I certainly haven't thought through the all of the implications.

This way you will loose tree state for closed projects while treemacs itself never looses it, even when >a project is folded automatically. Also this will be the case when you open not a scala file in your >current project - the tree will disappear. And as for me I hate when such things happen :)

I definitely agree with your pet usability hates there - I don't think that happens with my current implementation - I switch buffers and previous state of the tree is displayed for the new workspace - state is retained because the tree isn't destroyed. But may be I don't have a suitable test case, or there are other scenarios I've not considered. You would lose state if you shutdown the lsp workspace because the tree/sidebar is destroyed then.

@kurnevsky @yyoncho how do we move forward from here - I certainly don't want to force any decisions on anyone or this feature. How should we combine these code bases?

dsyzling avatar Sep 02 '19 08:09 dsyzling

How should we combine these code bases?

I leave it up to you both to decide what will be the best way to do that.

My personal opinion is that we could first unify the icons(i. e. how both look like), commit both as they are and then gradually refactor and remove duplication.

yyoncho avatar Sep 02 '19 08:09 yyoncho

@kurnevsky @yyoncho how do we move forward from here - I certainly don't want to force any decisions on anyone or this feature. How should we combine these code bases?

Could you create PR so I can review your code? :)

kurnevsky avatar Sep 02 '19 18:09 kurnevsky

@kurnevsky @yyoncho I've added a PR with the current code to #13. Have a look through and think about how we can potentially merge code bases. I can definitely easily remove the icons and then we would use your ones. Currently this implementation relies on a custom-capabilities feature implemented within lsp-mode which I can add a PR for - however we could move forward with a hook equivalent and then refactor this as necessary.

dsyzling avatar Sep 02 '19 20:09 dsyzling