Support multi-level imenu indexes #179

tigersoldier · 2018-09-12T18:35:52Z

LSP has added support for hierarchical document symbols. lsp-ui-imenu will throw
error if the imenu index has more than 2 nested alists.

This change aims at keeping the rendering for 2-level indexes unchanged, while
supporting more level indexes. For multi-level indexes, left title placement
doesn't make much sense for all levels, so I only keep the left title placement
for the toplevel title.

tigersoldier · 2018-09-12T18:37:34Z

Here are the screenshots for the multiple level imenu indexes

Top placement

Left placement

yyoncho · 2018-09-12T18:40:38Z

Great!

MaskRay · 2018-09-12T18:44:25Z

great! will read after hours

…

On Wed, Sep 12, 2018, 11:40 yyoncho ***@***.***> wrote: Great! — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#179 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZaQoAbeBHMh8BtDJKdauKjs_XGGYtNks5uaVUmgaJpZM4Wl8Nq> .

MaskRay · 2018-09-13T04:32:46Z

https://microsoft.github.io/language-server-protocol/specification

Do you mean export class DocumentSymbol by "hierarchical document symbols"? AFAIK lsp-mode does not support DocumentSymbol[]-typed return values yet. Do you have a workaround?

MaskRay · 2018-09-13T04:47:03Z

lsp-ui-imenu.el

@@ -112,62 +149,120 @@
        (when (overlayp lsp-ui-imenu-ov)
          (delete-overlay lsp-ui-imenu-ov))))))

+(defun lsp-ui-imenu--move-to-line-beginning ()
+  (-when-let* ((padding (get-char-property (point) 'padding))
+               (bars-count (get-char-property (point) 'bars-count)))


I don't fully understand the code, but it seems to me depth is a better name than bars-count (which counts confusing "invisible ones")

depth is a good name. Done.

MaskRay · 2018-09-13T04:47:58Z

lsp-ui-imenu.el

@@ -112,62 +149,120 @@
        (when (overlayp lsp-ui-imenu-ov)
          (delete-overlay lsp-ui-imenu-ov))))))

+(defun lsp-ui-imenu--move-to-line-beginning ()


Is there a better name for this function? This actually moves to an indented position.

MaskRay · 2018-09-13T04:53:47Z

lsp-ui-imenu.el

+Return the updated COLOR-INDEX."
+  (let ((len (length items)))
+    (--each-indexed items
+      (let ((is-last (eq (1+ it-index) len)))


I think eq is more efficient than = when comparing two fixnums but the latter seems more common.

MaskRay · 2018-09-13T04:55:40Z

lsp-ui-imenu.el

+                (setq color-index (1+ color-index))))
+          (insert (lsp-ui-imenu--make-line title it-index it
+                                           padding bars bars-count color-index
+                                           (eq (1+ it-index) len)))))))


This can be just is-last

MaskRay · 2018-09-13T05:02:16Z

I wonder, rather than use bars and bars-count (I'm accustomed to depth), can you use a list for the string chunks? See @amosbird's https://github.com/amosbird/ccutils/blob/master/src/dump.hpp for an example how to render a tree.

LSP has added support for hierarchical document symbols. lsp-ui-imenu will throw error if the imenu index has more than 2 nested alists. This change aims at keeping the rendering for 2-level indexes unchanged, while supporting more level indexes. For multi-level indexes, left title placement doesn't make much sense for all levels, so I only keep the left title placement for the first level title.

tigersoldier · 2018-09-13T05:56:16Z

https://microsoft.github.io/language-server-protocol/specification

Do you mean export class DocumentSymbol by "hierarchical document symbols"? AFAIK lsp-mode does not support DocumentSymbol[]-typed return values yet. Do you have a workaround?

I have a PR for lsp-mode to support DocumentSymbol: emacs-lsp/lsp-mode#421. It's better to add hierarchical support to lsp-ui-imenu before the PR is merged and breaks it. Besides, lsp-ui-imenu doesn't depend on anything about LSP. The screenshots in this PR are actually taken in typescript-mode, which doesn't use LSP.

I wonder, rather than use bars and bars-count (I'm accustomed to depth), can you use a list for the string chunks? See @amosbird's https://github.com/amosbird/ccutils/blob/master/src/dump.hpp for an example how to render a tree.

I've considered using string chunks, but elisp doesn't have a built-in deque data structure. I can use a reverse list but that would require rendering in reverse order. I think bool vector is quite intuitive for this use case. Only drawback is it's fix length.

MaskRay · 2018-09-13T06:41:41Z

The bars are organized as a stack (LIFO; operations are only done to the top element) so I think a list will be a perfect fit. When rendering, you'll reverse the list then join them. I don't fully understand the UI code so I might miss something.

tigersoldier · 2018-09-13T18:47:01Z

True that the bars are organized as a stack, but that doesn't mean using a list-simulated-stack to implement it is the best option. Consider the usage here:

;; initialization
(make-bool-vector 8 t)
;; manipulate for last node
(when is-last
 (aset bars depth nil))
(when is-last
 (aset bars depth t))
;; rendering
(dotimes (i depth)
  (get-bar bars i is-title is-last))

As opposed to:

;; initialization
nil
;; manipuate for next depth
(cons
 (get-bar is-title is-last) bars)
;; rendering
(when depth > 1 (render-bar (cdr bars) (1- depth)))
(car bars)
;; rendering, alternative
(concat (reverse bars))

I don't think the stack-based version is easier to read than the bool vector version.

MaskRay · 2018-09-13T20:23:22Z

What if you don't allocate bars beforehand but prepend an element when you do a recursive call?

i.e rather than

allocate bars

dfs(depth) = do
  if end condition is met
    compute bars with lsp-ui-imenu--get-bar
...
  modify bars[depth]
    dfs(depth+1)
  recover bars[depth]

use

dfs(depth, bars) = do
  if end condition is met
    print join(reverse(bars))
...
  dfs(depth+1, bars prepended with an element (what lsp-ui-imenu--get-bar does))

tigersoldier · 2018-09-13T20:34:03Z

What if you don't allocate bars beforehand but prepend an element when you do a recursive call?

i.e rather than
allocate bars

dfs(depth) = do
...
  modify bars[depth]
    dfs(depth+1)
  recover bars[depth]
use
dfs(depth, bars) = do
...
  dfs(depth+1, bars prepended with an element)

It's not as simple as you showed. The logic for stack would look like this:

if last:
  push(bar, 'L')
  render this node
  pop(bar)
  push(bar, ' ')
  dfs(children, bars)
else:
  push(bar, '|')
  render this node
  dfs(children, bars)

MaskRay · 2018-09-13T21:18:32Z

This can be written as:

if last:
  render this node with cons('L', bars)
  dfs(children, cons(' ', bars))
else:
  render this node with cons('|', bars)
  dfs(children, cons('|', bars))

tigersoldier · 2018-09-13T23:27:21Z

It's not better than the bool vector, and people reading the code have to keep in mind that the vector is in reversed order, which is less intuitive than bool vector

MaskRay · 2018-09-14T00:00:33Z

That way you can get rid of the hard limit lsp-ui-imenu--max-bars. Recursion with an accumulating parameter is an established idiom in functional programming. You may save some parameters from the function signatures. I believe in this case the shape of the bar can be pre-computed, instead of wrapping the recursion with set+unset and using a separate function to compute the bar.

MaskRay · 2018-09-14T00:07:56Z

Can you dump some data for me to test? I'll take a stab to improve it.

tigersoldier · 2018-09-14T00:19:22Z

I would agree with you if I don't need to render the bars with recursion or reverse.

Here is the data I used to generate the screenshot:

(("<function>"
  (#("<function> function" 11 19
     (face tide-imenu-type-face))
    . 616)
  (#("<function> function" 11 19
     (face tide-imenu-type-face))
    . 798)
  ("<function>"
   (#("<function> function" 11 19
      (face tide-imenu-type-face))
     . 1207)
   ("<function>"
    (#("<function> function" 11 19
       (face tide-imenu-type-face))
      . 1270)
    (#("doesContains var" 13 16
       (face tide-imenu-type-face))
      . 1311))))
 (#("args var" 5 8
    (face tide-imenu-type-face))
   . 295)
 (#("cp alias" 3 8
    (face tide-imenu-type-face))
   . 164)
 (#("fs alias" 3 8
    (face tide-imenu-type-face))
   . 202)
 ("jake"
  (#("jake var" 5 8
     (face tide-imenu-type-face))
    . 549)
  (#("<function> function" 11 19
     (face tide-imenu-type-face))
    . 584))
 ("tsc"
  (#("tsc function" 4 12
     (face tide-imenu-type-face))
    . 326)
  (#("<function> function" 11 19
     (face tide-imenu-type-face))
    . 488)
  ("tsc"
   (#("tsc var" 4 7
      (face tide-imenu-type-face))
     . 402)
   (#("<function> function" 11 19
      (face tide-imenu-type-face))
     . 453))))

MaskRay · 2018-09-14T05:47:10Z

I tried but it is too troublesome.. There are still some issues..

diff --git a/lsp-ui-imenu.el b/lsp-ui-imenu.el
index 465ca54..70f4fa7 100644
--- a/lsp-ui-imenu.el
+++ b/lsp-ui-imenu.el
@@ -67,36 +67,32 @@
 (declare-function imenu--subalist-p 'imenu)
 (defvar imenu--index-alist)
 
-(defun lsp-ui-imenu--pad (s len bars depth color-index for-title is-last)
+(defun lsp-ui-imenu--pad (entry s len bars color-index for-title is-last)
+  (message (format "=== %s %s" bars entry))
   (let ((n (- len (length s))))
     (apply #'concat
            (make-string n ?\s)
            (propertize s 'face `(:foreground ,(lsp-ui-imenu--get-color color-index)))
            (let (bar-strings)
-             (dotimes (i depth)
+             (dolist (bar bars)
                (push
-                (propertize (lsp-ui-imenu--get-bar bars i depth for-title is-last)
+                (propertize (lsp-ui-imenu--get-bar bar (not (consp bar)) for-title is-last)
                             'face `(:foreground
-                                    ,(lsp-ui-imenu--get-color (+ color-index i))))
-                bar-strings))
+                                    ,(lsp-ui-imenu--get-color color-index)))
+                bar-strings)
+               (cl-incf color-index))
              (reverse bar-strings)))))
 
-(defun lsp-ui-imenu--get-bar (bars index depth for-title is-last)
+(defun lsp-ui-imenu--get-bar (bar hang for-title is-last)
   (cond
-   ((>= index lsp-ui-imenu--max-bars)
-    ;; Exceeding maximum bars
-    "   ")
-   ((not (aref bars index))
+   ((not bar)
     ;; No bar for this level
     "   ")
-   ((and (= depth 1) (not for-title))
+   ((and hang (not for-title))
     ;; For the first level, the title is rendered differently, so leaf items are
     ;; decorated with the full height bar regardless if it's the last item or
     ;; not.
     " ┃ ")
-   ((< (1+ index) depth)
-    ;; Full height bar for levels other than the rightmost one.
-    " ┃ ")
    (is-last
     ;; The rightmost bar for the last item.
     " ┗ " )
@@ -110,14 +106,15 @@
 (defun lsp-ui-imenu--get-color (index)
   (nth (mod index (length lsp-ui-imenu-colors)) lsp-ui-imenu-colors))
 
-(defun lsp-ui-imenu--make-line (title index entry padding bars depth color-index is-last)
-  (let* ((prefix (if (and (= index 0) (eq lsp-ui-imenu-kind-position 'left)) title " "))
-         (text (concat (lsp-ui-imenu--pad prefix padding bars depth color-index nil is-last)
+(defun lsp-ui-imenu--make-line (title index entry padding bars color-index is-last)
+  (let* ((prefix (if (and (null bars) (eq lsp-ui-imenu-kind-position 'left))
+                     title " "))
+         (text (concat (lsp-ui-imenu--pad (car entry) prefix padding bars color-index nil is-last)
                        (propertize (car entry) 'face 'default)
                        "\n"))
          (len (length text)))
     (add-text-properties 0 len `(index ,index title ,title marker ,(cdr entry)
-                                       padding ,padding depth, depth)
+                                       padding ,padding depth ,(length bars))
                          text)
     text))
 
@@ -173,16 +170,16 @@
     ;; Left placement, title is put with the first sub item. Only put a separator here.
     (lsp-ui-imenu--put-separator)))
 
-(defun lsp-ui-imenu--put-subtitle (title padding bars depth color-index is-last)
+(defun lsp-ui-imenu--put-subtitle (title padding bars color-index is-last)
   (let ((ov (make-overlay (point) (point)))
-        (title-color (lsp-ui-imenu--get-color (+ color-index depth))))
+        (title-color (lsp-ui-imenu--get-color (+ color-index (length bars)))))
     (overlay-put
      ov 'after-string
-     (concat (lsp-ui-imenu--pad " " padding bars depth color-index t is-last)
+     (concat (lsp-ui-imenu--pad title " " padding bars color-index t is-last)
              (propertize title 'face `(:foreground ,title-color))
              (propertize "\n" 'face '(:height 1))))))
 
-(defun lsp-ui-imenu--insert-items (title items padding bars depth color-index)
+(defun lsp-ui-imenu--insert-items (title items padding bars color-index)
   "Insert ITEMS for TITLE.
 
 PADDING is the length of whitespaces to the left of the first bar.
@@ -190,8 +187,6 @@ PADDING is the length of whitespaces to the left of the first bar.
 BARS is a bool vector of length `lsp-ui-imenu--max-bars'. The ith
 value indicates whether the ith bar from the left is visible.
 
-DEPTH is the depth of the items in the index tree, starting from 0.
-
 COLOR-INDEX is the index of the color of the leftmost bar.
 
 Return the updated COLOR-INDEX."
@@ -200,25 +195,25 @@ Return the updated COLOR-INDEX."
       (let ((is-last (= (1+ it-index) len)))
         (if (imenu--subalist-p it)
             (-let* (((sub-title . entries) it))
-              (if (= depth 0)
+              (if (null bars)
                   (lsp-ui-imenu--put-toplevel-title sub-title color-index)
-                (lsp-ui-imenu--put-subtitle sub-title padding bars depth color-index is-last))
-              (when (and is-last (> depth 0))
-                (aset bars (1- depth) nil))
-              (let ((lsp-ui-imenu-kind-position (if (> depth 0) 'top
+                (when is-last
+                    (setq bars (cons nil (cdr bars)))
+                    )
+                (lsp-ui-imenu--put-subtitle sub-title padding
+                                            bars
+                                            color-index is-last))
+              (let ((lsp-ui-imenu-kind-position (if (consp bars) 'top
                                                   lsp-ui-imenu-kind-position)))
                 (lsp-ui-imenu--insert-items sub-title
                                             entries
                                             padding
-                                            bars
-                                            (1+ depth)
+                                            (cons t bars)
                                             color-index))
-              (when (and is-last (> depth 0))
-                (aset bars (1- depth) t))
-              (when (= depth 0)
+              (when (null bars)
                 (setq color-index (1+ color-index))))
           (insert (lsp-ui-imenu--make-line title it-index it
-                                           padding bars depth color-index
+                                           padding bars color-index
                                            is-last))))))
   color-index)
 
@@ -243,15 +238,14 @@ Return the updated COLOR-INDEX."
       (let* ((padding (lsp-ui-imenu--get-padding list))
              (grouped-by-subs (-partition-by 'imenu--subalist-p list))
              (color-index 0)
-             (bars (make-bool-vector lsp-ui-imenu--max-bars t))
              buffer-read-only)
         (remove-overlays)
         (erase-buffer)
         (dolist (group grouped-by-subs)
           (if (imenu--subalist-p (car group))
-              (setq color-index (lsp-ui-imenu--insert-items "" group padding bars 0 color-index))
+              (setq color-index (lsp-ui-imenu--insert-items "" group padding nil color-index))
             (lsp-ui-imenu--put-separator)
-            (lsp-ui-imenu--insert-items "" group padding bars 1 color-index)
+            (lsp-ui-imenu--insert-items "" group padding '(t) color-index)
             (setq color-index (1+ color-index))))
         (lsp-ui-imenu-mode)
         (setq mode-line-format '(:eval (lsp-ui-imenu--win-separator)))

bizzyman · 2018-09-14T15:34:23Z

Which language server did you use to generate the hierarchical data?

tigersoldier · 2018-09-14T15:59:49Z

@bizzyman The data was not generated by any language server. The data was generated from imenu--index-alist in a typescript-mode buffer.

With emacs-lsp/lsp-mode#421, I can generate the following data from JavaComp:

(("DocumentSymbolHandler"
  ("(Class)" . #<marker at 1072 in DocumentSymbolHandler.java>)
  ("<init> (Method)" . #<marker at 1893 in DocumentSymbolHandler.java>)
  ("addDocumentSymbolsInScope"
   ("(Method)" . #<marker at 3756 in DocumentSymbolHandler.java>)
   ("childScope (Variable)" . #<marker at 4769 in DocumentSymbolHandler.java>)
   ("children (Variable)" . #<marker at 4090 in DocumentSymbolHandler.java>)
   ("entity (Variable)" . #<marker at 4131 in DocumentSymbolHandler.java>)
   ("newSymbol (Variable)" . #<marker at 4030 in DocumentSymbolHandler.java>)
   ("symbolKind (Variable)" . #<marker at 4196 in DocumentSymbolHandler.java>))
  ("addSymbolInformationsInScope"
   ("(Method)" . #<marker at 5002 in DocumentSymbolHandler.java>)
   ("entity (Variable)" . #<marker at 5308 in DocumentSymbolHandler.java>)
   ("newContainerName (Variable)" . #<marker at 5867 in DocumentSymbolHandler.java>)
   ("symbol (Variable)" . #<marker at 5488 in DocumentSymbolHandler.java>)
   ("symbolKind (Variable)" . #<marker at 5372 in DocumentSymbolHandler.java>))
  ("getSymbolKind"
   ("(Method)" . #<marker at 6219 in DocumentSymbolHandler.java>)
   ("parentEntity (Variable)" . #<marker at 6343 in DocumentSymbolHandler.java>)
   ("symbolKind (Variable)" . #<marker at 6303 in DocumentSymbolHandler.java>))
  ("handleRequest"
   ("(Method)" . #<marker at 2103 in DocumentSymbolHandler.java>)
   ("clientCapabilities (Variable)" . #<marker at 2407 in DocumentSymbolHandler.java>)
   ("documentSymbolCapabilities (Variable)" . #<marker at 2491 in DocumentSymbolHandler.java>)
   ("fileScope (Variable)" . #<marker at 2206 in DocumentSymbolHandler.java>)
   ("supportDocumentSymbol (Variable)" . #<marker at 2613 in DocumentSymbolHandler.java>)
   ("supportedSymbolKinds (Variable)" . #<marker at 2546 in DocumentSymbolHandler.java>)
   ("symbols (Variable)" . #<marker at 3220 in DocumentSymbolHandler.java>)
   ("symbols (Variable)" . #<marker at 3449 in DocumentSymbolHandler.java>))
  ("DEFAULT_SUPPORTED_SYMBOL_KINDS (Field)" . #<marker at 1265 in DocumentSymbolHandler.java>)
  ("logger (Field)" . #<marker at 1172 in DocumentSymbolHandler.java>)
  ("server (Field)" . #<marker at 1882 in DocumentSymbolHandler.java>)))

MaskRay reviewed Sep 13, 2018

View reviewed changes

tigersoldier force-pushed the imenu-document-symbol branch from fa37e5c to 953c3c0 Compare September 13, 2018 05:48

MaskRay mentioned this pull request Sep 13, 2018

Support hierarchical document symbols emacs-lsp/lsp-mode#421

Merged

MaskRay merged commit 636d834 into emacs-lsp:master Sep 14, 2018

MaskRay mentioned this pull request Sep 18, 2018

Make lsp-ui-imenu hierarchical #73

Closed

MaskRay mentioned this pull request Nov 7, 2018

Make imenu hierarchical joaotavora/eglot#145

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multi-level imenu indexes #179

Support multi-level imenu indexes #179

tigersoldier commented Sep 12, 2018

tigersoldier commented Sep 12, 2018

yyoncho commented Sep 12, 2018

MaskRay commented Sep 12, 2018 via email

MaskRay commented Sep 13, 2018

MaskRay Sep 13, 2018

tigersoldier Sep 13, 2018

MaskRay Sep 13, 2018

tigersoldier Sep 13, 2018

MaskRay Sep 13, 2018

tigersoldier Sep 13, 2018

MaskRay Sep 13, 2018

tigersoldier Sep 13, 2018

MaskRay commented Sep 13, 2018

tigersoldier commented Sep 13, 2018

MaskRay commented Sep 13, 2018 •

edited

tigersoldier commented Sep 13, 2018

MaskRay commented Sep 13, 2018 •

edited

tigersoldier commented Sep 13, 2018

MaskRay commented Sep 13, 2018

tigersoldier commented Sep 13, 2018

MaskRay commented Sep 14, 2018 •

edited

MaskRay commented Sep 14, 2018

tigersoldier commented Sep 14, 2018

MaskRay commented Sep 14, 2018

bizzyman commented Sep 14, 2018

tigersoldier commented Sep 14, 2018

Support multi-level imenu indexes #179

Support multi-level imenu indexes #179

Conversation

tigersoldier commented Sep 12, 2018

tigersoldier commented Sep 12, 2018

yyoncho commented Sep 12, 2018

MaskRay commented Sep 12, 2018 via email

MaskRay commented Sep 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaskRay commented Sep 13, 2018

tigersoldier commented Sep 13, 2018

MaskRay commented Sep 13, 2018 • edited

tigersoldier commented Sep 13, 2018

MaskRay commented Sep 13, 2018 • edited

tigersoldier commented Sep 13, 2018

MaskRay commented Sep 13, 2018

tigersoldier commented Sep 13, 2018

MaskRay commented Sep 14, 2018 • edited

MaskRay commented Sep 14, 2018

tigersoldier commented Sep 14, 2018

MaskRay commented Sep 14, 2018

bizzyman commented Sep 14, 2018

tigersoldier commented Sep 14, 2018

MaskRay commented Sep 13, 2018 •

edited

MaskRay commented Sep 13, 2018 •

edited

MaskRay commented Sep 14, 2018 •

edited