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

Please make it to work with eglot #63

Closed
arbv opened this issue Feb 14, 2021 · 8 comments · Fixed by #65
Closed

Please make it to work with eglot #63

arbv opened this issue Feb 14, 2021 · 8 comments · Fixed by #65

Comments

@arbv
Copy link

arbv commented Feb 14, 2021

When being used with Eglot the current version fails with the following error message:

imenu-list--current-entry: Wrong type argument: number-or-marker-p, [(:children [(:kind 10 :name "UDP" :range (:end (:character 18 :line 33) :start (:character 15 :line 33)) :selectionRange (:end (:character 18 :line 33) :start (:character 15 :line 33)) :containerName "(anonymous enum)") (:kind 10 :name "TCP" :range (:end (:character 23 :line 33) :start (:character 20 :line 33)) :selectionRange (:end (:character 23 :line 33) :start (:character 20 :line 33)) :containerName "(anonymous enum)") (:kind 10 :name "DOT" :range (:end (:character 28 :line 33) :start (:character 25 :line 33)) :selectionRange (:end (:character 28 :line 33) :start (:character 25 :line 33)) :containerName "(anonymous enum)") (:kind 10 :name "DOH" :range (:end (:character 33 :line 33) :start (:character 30 :line 33)) :selectionRange (:end (:character 33 :line 33) :start (:character 30 :line 33)) :containerName "(anonymous enum)")] :kind 10 :name "(anonymous enum)" :range (:end (:character 35 :line 33) :start (:character 8 :line 33)) :selectionRange (:end (:character 12 :line 33) :start (:character 8 :line 33)))] [3 times]

I needed to write my own custom position translator. Here are the relevant parts of my config:

(defun eglot/position-translator ()
  (lambda (e)
    (let* ((range (plist-get (aref e 0) :range))
           (start (plist-get range :start)))
      (when (fboundp #'eglot--lsp-position-to-point)
        (eglot--lsp-position-to-point start t)))))

(defun imenu-list/enable-eglot-position-translator ()
  (interactive)
  (when (and (fboundp #'eglot-managed-p)
             (eglot-managed-p))
    (setq-local imenu-list-custom-position-translator
                'eglot/position-translator)))

(defun imenu-list/eglot-toggle ()
  (interactive)
  (imenu-list/enable-eglot-position-translator)
  (imenu-list-smart-toggle))

;; imenu-list to browse tags
(global-set-key (kbd "<f9>") 'imenu-list/eglot-toggle)

Please adapt the code to include it into a forthcoming version.

@muffinmad
Copy link

Maybe related to #58

@arbv
Copy link
Author

arbv commented Feb 15, 2021

It seems so, especially taking into account this reply from eglot author in another issue.

@bmag
Copy link
Owner

bmag commented Mar 2, 2021

Thanks for raising this issue. Indeed it is a continuation of #58.

I'd like to add a case for Eglot in imenu-list-position-translator, however I'm not sure how to correctly detect when Eglot is used. Eglot doesn't set imenu-create-index-function to eglot-imenu. Instead, it relies on imenu-create-index-function being set to its default value (imenu-default-create-index-function), and adds eglot-imenu as a local :before-until advice to imenu-default-create-index-function. This means that if eglot-imenu returns nil, then the default non-Eglot index is created.

I think for detection, I can use the condition @arbv suggested:

(and (fboundp #'eglot-managed-p) (eglot-managed-p))

And the translator function will handle both Eglot and default imenu entries:

(defun get-eglot-position (pos)
  ;; when Eglot is in charge of Imenu, then the index is created by `eglot-imenu', with a fallback to
  ;; `imenu-default-create-index-function' when `eglot-imenu' returns nil. If POS is an array, it means
  ;; it was created by `eglot-imenu' and we need to extract its position. Otherwise, it was created by
  ;; `imenu-default-create-index-function' and we should return it as-is.
  (if (arrayp pos)
      (eglot--lsp-position-to-point (plist-get (plist-get (aref pos 0) :range) :start) t)
    pos))

@bmag
Copy link
Owner

bmag commented Mar 2, 2021

I don't use Eglot. Can both of you test that #65 solves this issue? The code is liable to break with future changes of Eglot, but hopefully it won't happen often and the fixes will be simple.

@muffinmad
Copy link

@bmag Thanks! But why you rely on eglot? I think you can just call function with arguments if the element is looks like (INDEX-NAME POSITION FUNCTION ARGUMENTS...).

@bmag
Copy link
Owner

bmag commented Mar 2, 2021

@muffinmad that function jumps to the entry's location, so using it means jumping to each entry and then jumping back, which I suspect will have a performance impact in buffers with a large index (especially when point is towards the end of the buffer). If you want to try this approach, evaluate the following code:

(defun imenu-list--current-entry-via-excursion ()
  (let ((point-pos (point-marker))
        (offset (point-min-marker))
        match-entry)
    (dolist (entry imenu-list--line-entries match-entry)
      (unless (imenu--subalist-p entry)
        (let ((entry-pos (save-mark-and-excursion
                           (imenu entry)
                           (point))))          
          (when (imenu-list-<= offset entry-pos point-pos)
            (setq offset entry-pos)
            (setq match-entry entry)))))))

(advice-add #'imenu-list--current-entry :override #'imenu-list--current-entry-via-excursion)

;; to disable:
;; (advice-remove #'imenu-list--current-entry #'imenu-list--current-entry-via-excursion)

@muffinmad
Copy link

@bmag Right, I've completely forgot about the current function highlighting.

@arbv
Copy link
Author

arbv commented Mar 4, 2021

@bmag

I have not tested the whole package with the changes that you have suggested in #65, but the function imenu-list--translate-eglot-position works fine when being used in my configuration file.

So, I would say it is safe to merge.

Thanks for addressing this issue!

@bmag bmag closed this as completed in #65 Mar 4, 2021
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 a pull request may close this issue.

3 participants