-
Notifications
You must be signed in to change notification settings - Fork 49
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
base: master
Are you sure you want to change the base?
Conversation
Seems like @dsyzling was working on the same thing. |
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 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 I was thinking about adding 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 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 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. |
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. |
Activating
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).
Why not a single buffer for all of them? Having 3 buffers open at the same time might be overwhelming :) |
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 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.
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. |
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. |
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. |
I think that we could support both ways of displaying the treeview and the users could pick the one that fits his/her workflow. |
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 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.
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? |
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. |
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))) |
There was a problem hiding this comment.
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 " ▾ ") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@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. |
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.