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

Add Metals Tree View Protocol support. #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kurnevsky
Copy link
Member

@kurnevsky kurnevsky commented Sep 1, 2019

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

@yyoncho
Copy link
Member

yyoncho commented Sep 1, 2019

Seems like @dsyzling was working on the same thing.

@dsyzling
Copy link
Member

dsyzling commented Sep 1, 2019

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?

@kurnevsky
Copy link
Member Author

@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.

@dsyzling
Copy link
Member

dsyzling commented Sep 1, 2019

@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
Copy link
Member

dsyzling commented Sep 1, 2019

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.

@kurnevsky
Copy link
Member Author

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 :)

@dsyzling
Copy link
Member

dsyzling commented Sep 1, 2019

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
Copy link
Member

dsyzling commented Sep 1, 2019

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
Copy link
Member

dsyzling commented Sep 1, 2019

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.

@yyoncho
Copy link
Member

yyoncho commented Sep 2, 2019

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

@kurnevsky
Copy link
Member Author

kurnevsky commented Sep 2, 2019

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.

@yyoncho
Copy link
Member

yyoncho commented Sep 2, 2019

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?

@dsyzling
Copy link
Member

dsyzling commented Sep 2, 2019

@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?

@yyoncho
Copy link
Member

yyoncho commented Sep 2, 2019

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.

@kurnevsky
Copy link
Member Author

@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? :)

(with-lsp-workspace (lsp-treemacs-workspace-at-point)
(let ((command (treemacs--prop-at-point :command)))
(pcase (ht-get command "command")
(`"metals-echo-command" (lsp-send-execute-command (elt (ht-get command "arguments") 0)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seq-first instead of elt

:icon (ht-get item "icon"))))

(treemacs-define-expandable-node tvp-root
:icon-open (treemacs-as-icon " ▾ ")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use the expanded/collapsed icons like symbols/deps list views? This will make the lsp-treemacs related views consistent and also it will allow defining themes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please ignore that comment. I will do the opposite - will stitch the rest of the controls to use these symbols.

"Get the icon for the NAME.
Return DEFAULT if there is no such icon."
(pcase name
(`"class" (treemacs-get-icon-value 'class nil 'Metals))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move the 'Metals theme icons into the dark and light theme and then use only lsp-treemacs-tvp-theme? This will allow you remove the switch and do (treemacs-get-icon-value (intern name) nil lsp-treemacs-tvp-theme) and it will make the creation of themes easier(no hardcoded themes).

@@ -33,6 +33,7 @@
(require 'treemacs-icons)

(require 'lsp-mode)
(require 'lsp-treemacs-tvp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed. Let users explicitly require it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is the only thing in the review blocking the merge.

@dsyzling
Copy link
Member

dsyzling commented Sep 2, 2019

@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.

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

Successfully merging this pull request may close these issues.

None yet

3 participants