Scala Metals Treeview #13

dsyzling · 2019-09-02T19:59:33Z

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.

yyoncho

Looks good. It seems like we cannot share a lot with the embedded implementation.

Here it is some nitpicking, I haven't tested it live yet.

yyoncho · 2019-09-03T05:05:14Z

lsp-metals-treeview.el

+the WORKSPACE."
+  (-when-let* ((state (lsp--metals-treeview-get-data workspace))
+               (buffers (append (lsp--metals-treeview-data-buffers state) (list buffer))))
+    (setf (lsp--metals-treeview-data-buffers state) buffers)))


you may use push to push the buffer directly in the slot (e. g. (push buffer (lsp--metals-treeview-data-buffers state)))

yyoncho · 2019-09-03T05:07:28Z

lsp-metals-treeview.el

+treeview buffers and if so return the buffers."
+  ;; retrieve any treeview buffers that are visible
+  (->> (window-list (selected-frame))
+       (-map (lambda (window)


there is function -keep which does what you want.

yyoncho · 2019-09-03T05:09:25Z

lsp-metals-treeview.el

+  "Return visibility status of metals treeview associated
+with WORKSPACE. Return 'visible, 'hidden, 'none depending on state of
+treeview."
+  (if (lsp--metals-treeview-visible? workspace)


using cond will yield better code here. e. g.

(cond ((lsp--metals-treeview-visible? workspace) 'visible) ...

yyoncho · 2019-09-03T05:12:04Z

lsp-metals-treeview.el

+            (setq-local lsp--metals-treeview-current-workspace workspace)
+            (setq-local lsp--metals-view-id view-id)
+            (treemacs-METALS-ROOT-extension)
+            (setq-local mode-line-format (lsp--metals-view-name view-id))


Please refer to this PR #10 @kurnevsky has improved the way we handle the modeline.

After this you shouldn't need minor mode lsp-metals-treeview-mode.

yyoncho · 2019-09-03T05:13:34Z

lsp-metals-treeview.el

+
+(defun lsp-metals-treeview-reveal ()
+  (interactive)
+  (let (workspace (car (lsp-workspaces)))


seems like it is not used?

yyoncho · 2019-09-03T05:15:00Z

lsp-metals-treeview.el

+WORKSPACE is not specified obtain the current workspace for the file in
+the current buffer."
+  (interactive)
+  (-if-let* ((workspace (or workspace (car (lsp-workspaces)))))


If this should work only the context of metals workspace you should use lsp-find-workspace with the corresponding server-id.

dsyzling · 2019-09-05T19:30:32Z

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 · 2019-09-05T19:46:00Z

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.

yyoncho · 2019-09-05T19:48:23Z

lsp-metals-treeview.el

@@ -166,7 +171,7 @@ the WORKSPACE."
  "Add the BUFFER to the list of treeview buffers associated with
 the WORKSPACE."
  (-when-let* ((state (lsp--metals-treeview-get-data workspace))
-               (buffers (append (lsp--metals-treeview-data-buffers state) (list buffer))))
+               (buffers (push buffer (lsp--metals-treeview-data-buffers state))))
    (setf (lsp--metals-treeview-data-buffers state) buffers)))


setf is now not needed because push updates the buffers directly in that slot.

Ah yes of course it mutates in place.

yyoncho

No more nit-picking from my side. Merging after @kurnevsky test it. The same comment applies about #11 .

kurnevsky · 2019-09-05T20:01:59Z

I'll look into it at the weekend.

dsyzling · 2019-09-05T21:01:06Z

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 · 2019-09-05T21:29:48Z

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 · 2019-09-06T17:05:12Z

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

kurnevsky · 2019-09-07T05:31:00Z

lsp-metals-treeview.el

+;; replace lsp--metals-treeview-state to return treemacs-metals-node-closed-state
+;;
+(treemacs-define-leaf-node metals-leaf
+  (treemacs-get-icon-value 'root nil "Metals")


You can use 'dynamic-icon symbol as icon to indicate that the icon is defined by the parent.

kurnevsky · 2019-09-07T05:34:39Z

lsp-metals-treeview.el

+  (treemacs-get-icon-value 'root nil "Metals")
+
+  :ret-action #'lsp--metals-treeview-exec-node-action
+  :tab-action #'lsp--metals-treeview-exec-node-action


I think we shouldn't define tab-action here. Usually with tab user want to expand a node but not to run a action. And with icons it's not always clear if a node is expandable or not.
Also that's the reason I wouldn't remove treemacs-define-leaf-node in favor of treemacs-define-expandable-node - treemacs indicates with red color that node can't be expanded when tab is pressed.

kurnevsky · 2019-09-07T05:57:41Z

lsp-metals-treeview.el

+    (if (ht-get metals-node "collapseState")
+        (if open-form?
+            (treemacs-as-icon "- " 'face 'font-lock-string-face)
+          (treemacs-as-icon "+ " 'face 'font-lock-string-face))


How about using • ▸ ▾ icons? Also it seems they should have spaces before and after (3 symbols in total) to have proper alignment.

kurnevsky · 2019-09-07T05:58:51Z

lsp-metals-treeview.el

+
+(treemacs-define-expandable-node metals-node
+  ;; :icon-open (treemacs-as-icon "- " 'face 'font-lock-string-face)
+  ;; :icon-closed (treemacs-as-icon "+ " 'face 'font-lock-string-face)


kurnevsky · 2019-09-07T05:59:07Z

lsp-metals-treeview.el

+
+(treemacs-define-expandable-node metals-root
+  ;; :icon-open (treemacs-as-icon "- " 'face 'font-lock-string-face)
+  ;; :icon-closed (treemacs-as-icon "+ " 'face 'font-lock-string-face)


kurnevsky · 2019-09-07T07:45:38Z

lsp-metals-treeview.el

+  (when-let ((view (car views)))
+    (lsp--metals-show-view workspace
+                           (alist-get :view-id view)
+                           `((side . left) (slot . ,slot)))


Use treemacs variables as defaults, see #9

kurnevsky · 2019-09-07T07:56:52Z

lsp-metals-treeview.el

+        (add-hook 'lsp-after-uninitialized-hook #'lsp--metals-treeview-on-workspace-shutdown))
+
+    ;; No views are available - show temp message.
+    (lsp--metals-show-waiting-message workspace `((side . left) (slot . ,slot)))))


Use treemacs variables as defaults, see #9.

kurnevsky · 2019-09-07T08:05:35Z

lsp-metals-treeview.el

+is currently being displayed and whether we need to show
+an alternative workspace's treeview."
+  (with-current-buffer (current-buffer)
+    (-if-let (workspaces (lsp-workspaces))


Check for the workspace type, otherwise it switches to a state Waiting for Metals Treeview information or closes the treeview on other languages.

kurnevsky · 2019-09-07T08:22:12Z

lsp-metals-treeview.el

+  (when (lsp--metals-treeview-exists? workspace)
+    (lsp--metals-treeview-delete-window workspace))
+
+  (let ((state (make-lsp--metals-treeview-data


Not sure about this design decision - it looks like we should know all root nodes and not wait for them from metals. Metals sends treeViewDidChange to indicate that we can get children for root nodes. It doesn't include metalsHelp there because it doesn't change and can be expanded at any moment. So maybe we can just enumerate root nodes in the code and get rid of lsp--metals-treeview-set-data?

Yes I'm not sure about this either. Given the spec didn't define the views and they were sent within an initial message after initialisation, my view was the Metals team were allowing for extension and could possibly send further views in future - may be a bsp view or this may be folded into the compile view who knows? So I took the decision to not rely on the views and build a side view that would dynamically add them as they were sent.

I agree the help items are just odd - they are fixed and handled differently from the other views - so I've no idea why the spec didn't simply define a set of static views up front.

If it made it easy for us to accommodate the two modes of operation (single treemacs tree and treeview panel) then I would be open to change. I haven't thought that far ahead, I also don't know what the Metals team are considering when introducing the bsp extensions - given IntelliJ has a new bsp view showing compilation and unit test status.

kurnevsky · 2019-09-07T08:27:58Z

lsp-metals-treeview.el

+            (setq-local lsp--metals-treeview-current-workspace workspace)
+            (setq-local lsp--metals-view-id view-id)
+            (treemacs-METALS-ROOT-extension)
+            (setq-local mode-line-format (lsp--metals-view-name view-id))


After this you shouldn't need minor mode lsp-metals-treeview-mode.

dsyzling · 2019-09-08T20:34:12Z

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

dsyzling · 2019-09-19T21:26:31Z

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 · 2019-10-10T06:03:27Z

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

kurnevsky · 2019-10-10T07:49:45Z

How about taking icons from my PR #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 · 2019-10-10T09:00:36Z

@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 · 2019-10-14T18:12:44Z

Added updated icons from @kurnevsky PR #11.

dsyzling · 2019-10-14T18:19:03Z

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 · 2019-10-15T17:23:29Z

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

Metals icons moved to existing icons directory. Register notification handlers for Metals and custom client capabilities after lsp-metals during lsp-metals-treeview-enable (autoload function).

Use the buffer-list-update hook to detect when the user changes to a file within another workspace and if the treeview is visible switch to the new workspace's treeview window.

Use push rather than the more complicated append with list. Use -keep rather than (remove nil) Add cond to make lsp--metals-treeview-visible more readable. Remove placeholder function lsp-metals-treeview-reveal - will add at a later date. Use lsp-find-workspace with metals server id rather than relying on lsp-workspaces.

Remove unnecessary setf, push will mutate the buffer list in place.

Simple example to show user how to enable and configure Metals treeview using use-package. Note: this relies on autoload - when developing it may be easier to explicitly require the package from a local dev area.

Use 'dynamic-icon in leaf node to delegate icon rendering functionality to parent node. Remove tab action from leaf node. Use arrow icons from lsp-treemacs theme and three spaces for leaf nodes to align parent children correctly. Remove commented code from expandable node and metals root definition. Remove overriding return action for root, this does have the effect of providing a treemacs error message and flash the element red - but some people may prefer this. Remove key definition for invoking treemacs node debug helper - not required. Fix file header with correct summary for treeview. Use treemacs variables to define side window position and width. When switching buffers ensure that the new buffer is associated with a Metals server so that we don't attempt to switch treeviews on another language server. In order to achieve this reliably we ensure that the buffer being switched to has a major mode of scala-mode.

Rename functions and symbols and make package consistent: lsp--metals -> lsp-metals-treeview-- lsp--metals-treeview -> lsp-metals-treeview--

Fix a bug with multiple workspaces, when closing one workspace the treeview will be closed. When a file associated with the closed workspace was open and invoking lsp-metals-treeview the other workspace's treeview would be displayed. lsp-metals-treeview now uses lsp-find-workspace with metals server-id and filename of current buffer to locate the associated workspace.

dsyzling · 2019-10-25T13:39:05Z

@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 · 2019-10-25T17:58:30Z

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.

Add configuration and usage instructions for Metals lsp-treemacs treeview along with animated screenshot.

dsyzling · 2019-10-26T15:44:01Z

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 · 2019-10-26T16:19:07Z

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 · 2019-10-26T16:20:49Z

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 · 2019-10-26T16:28:22Z

yes my account is '@dsyzling'

yyoncho · 2019-10-26T16:37:02Z

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

dsyzling mentioned this pull request Sep 2, 2019

Add Metals Tree View Protocol support. #11

Open

yyoncho reviewed Sep 3, 2019

View reviewed changes

yyoncho reviewed Sep 5, 2019

View reviewed changes

yyoncho approved these changes Sep 5, 2019

View reviewed changes

kurnevsky suggested changes Sep 7, 2019

View reviewed changes

dsyzling added 10 commits October 25, 2019 14:35

Move lsp-metals-treeview to lsp-treemacs project.

3680114

Metals icons moved to existing icons directory. Register notification handlers for Metals and custom client capabilities after lsp-metals during lsp-metals-treeview-enable (autoload function).

Initial implementation of treeview/workspace switching.

1bc0c31

Use the buffer-list-update hook to detect when the user changes to a file within another workspace and if the treeview is visible switch to the new workspace's treeview window.

Support link-hint package with treeview using default visit action.

1de546e

Do not need to use setf when using push to update buffers.

f5e6924

Remove unnecessary setf, push will mutate the buffer list in place.

Add use-package configuration example in comment header.

1eacb8b

Simple example to show user how to enable and configure Metals treeview using use-package. Note: this relies on autoload - when developing it may be easier to explicitly require the package from a local dev area.

Rename functions/symbols for package naming consistency.

5a5c42d

Rename functions and symbols and make package consistent: lsp--metals -> lsp-metals-treeview-- lsp--metals-treeview -> lsp-metals-treeview--

Updated Metals treeview icons from kurnevsky.

6d61972

dsyzling force-pushed the metals-treeview branch from 95e0acb to 9bf5442 Compare October 25, 2019 13:37

dsyzling added 2 commits October 26, 2019 16:31

README.org updates for Metals lsp-treemacs treeview.

2ad93dc

Add configuration and usage instructions for Metals lsp-treemacs treeview along with animated screenshot.

Update internal link syntax for github support.

e6f7ad1

yyoncho merged commit a06a5be into emacs-lsp:master Oct 26, 2019

Scala Metals Treeview #13

Scala Metals Treeview #13

Uh oh!

Conversation

dsyzling commented Sep 2, 2019

Uh oh!

yyoncho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsyzling commented Sep 5, 2019

Uh oh!

yyoncho commented Sep 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yyoncho left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kurnevsky commented Sep 5, 2019

Uh oh!

dsyzling commented Sep 5, 2019

Uh oh!

yyoncho commented Sep 5, 2019

Uh oh!

dsyzling commented Sep 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsyzling commented Sep 8, 2019

Uh oh!

dsyzling commented Sep 19, 2019

Uh oh!

yyoncho commented Oct 10, 2019

Uh oh!

kurnevsky commented Oct 10, 2019

Uh oh!

dsyzling commented Oct 10, 2019

Uh oh!

dsyzling commented Oct 14, 2019

Uh oh!

dsyzling commented Oct 14, 2019

Uh oh!

kurnevsky commented Oct 15, 2019

yyoncho left a comment •

edited

Loading