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

Provide a way to refresh the items #2263

Closed
yyoncho opened this issue Oct 8, 2019 · 11 comments
Closed

Provide a way to refresh the items #2263

yyoncho opened this issue Oct 8, 2019 · 11 comments

Comments

@yyoncho
Copy link

yyoncho commented Oct 8, 2019

This is follow up from emacs-lsp/lsp-mode#1010

The easiest way to reproduce the issue is by using https://github.com/emacs-lsp/lsp-docker using the vanilla recipe - https://github.com/emacs-lsp/lsp-docker#vanilla (lsp-ivy has to be installed separately).

git clone https://github.com/emacs-lsp/lsp-docker
cd lsp-docker
xhost local:root
sh start-emacs-vanilla.sh

This will download relativelly big docker image (3bg+) containing Emacs27 and several language servers preinstalled + demo projects mounted in /Projects/.
Alternatively, you may install the pyls server

pip install ‘python-language-server[all]’

... and then go to python file and do M-x lsp and select the proper project root or current directory.

@abo-abo
Copy link
Owner

abo-abo commented Oct 8, 2019

Thanks for the info. I've gotten to the point where M-x lsp works for Python.

But for lsp-ivy-workspace-symbol I get:

lsp-send-request-async: No workspace could handle workspace/symbol

@yyoncho
Copy link
Author

yyoncho commented Oct 8, 2019

Ah, sorry about that, forgot to check whether pyls supports that method:

npm i -g javascript-typescript-langserver ; npm i -g typescript

This will install javascript language server and then import https://github.com/emacs-lsp/lsp-docker/tree/master/demo-projects/Javascript/myapp project .

@abo-abo
Copy link
Owner

abo-abo commented Oct 9, 2019

Thanks for the example. I was finally able to get it to work.

An issue I found was that lsp-send-request-async wouldn't call the callback for some reason when I tried to call it manually. While lsp-send-request was working properly. So I wasn't able to fully refactor lsp-ivy--workspace-symbol.

But I came up with an example of how Ivy should be used in async (and not via counsel--async-command). Have a look at counsel-google. Two points of interest here:

  • the collection function should return 0 after it starts a request, so that ivy--exhibit doesn't update the minibuffer
  • the callback to the request should use ivy-update-candidates

Let me know if that's enough for lsp-ivy.

@yyoncho
Copy link
Author

yyoncho commented Oct 10, 2019

CC @sebastiansturm

abo-abo added a commit that referenced this issue Oct 10, 2019
It may set `ivy--all-candidates' to 0.

Set `ivy--old-re' to nil, so we get highlighting with
`ivy--highlight-default'.

Re #2263
@sebastiansturm
Copy link

many thanks for your PR over at emacs-lsp/lsp-ivy! As far as I can tell, the new implementation works very well.
As for the API, do you think it makes sense to also have ivy handle translation between abstract candidates and human-readable strings internally (say, by providing a :formatter) argument to ivy-read (my apologies if that option exists already and I missed it)?

That way, ivy-using packages relying on metadata (such as the source code location of a lsp-provided workspace symbol) but simple-minded enough so they don't need to keep around the candidate set for other reasons could directly forward candidates to ivy, and be invoked with a candidate of the expected type (a dictionary object in this case) on exit (or nil on abort).

@abo-abo
Copy link
Owner

abo-abo commented Oct 12, 2019

many thanks for your PR over at emacs-lsp/lsp-ivy! As far as I can tell, the new implementation works very well.

Great.

As for the API, do you think it makes sense to also have ivy handle translation between abstract
candidates and human-readable strings internally (say, by providing a :formatter) argument to
ivy-read (my apologies if that option exists already and I missed it)?

That way, ivy-using packages relying on metadata (such as the source code location of a
lsp-provided workspace symbol) but simple-minded enough so they don't need to keep around the
candidate set for other reasons could directly forward candidates to ivy, and be invoked with a
candidate of the expected type (a dictionary object in this case) on exit (or nil on abort).

Ivy has an API for transforming a candidate to a user-readable string. It's ivy-format-functions-alist.

ivy-read collection argument can be most of the collection types, as long as it can be coerced
to a string list for purposes of filtering. If you pass it a dictionary, Ivy doens't care what the
values are as long as the keys are strings. There are also options like putting data into string properties.

Let me know if this fits your use case.

@sebastiansturm
Copy link

actually yes, I think that's just what I was looking for. Thanks for the pointer! I had had a brief look at ivy's source code but what threw me off was ivy--format's docstring which says that the list of candidates passed to it by ivy-update-candidates must be a list of strings. I'm now using the following modification of your PR, will be testing it for a while:

(defun lsp-ivy--workspace-symbol (workspaces prompt initial-input)
  "Search against WORKSPACES with PROMPT and INITIAL-INPUT."
  (let ((candidates nil)
        (current-request-id nil))
    (ivy-read
     prompt
     (lambda (user-input)
       (with-lsp-workspaces workspaces
         (let ((request (lsp-make-request
                         "workspace/symbol"
                         (list :query user-input))))
           (when current-request-id
             (lsp--cancel-request
              current-request-id))
           (setq current-request-id
                 (plist-get request :id))
           (lsp-send-request-async
            request
            #'ivy-update-candidates
            :mode 'detached)))
       0)
     :dynamic-collection t
     :require-match t
     :initial-input initial-input
     :action #'lsp-ivy--workspace-symbol-action
     :caller 'lsp-ivy-workspace-symbol)))

(ivy-configure 'lsp-ivy-workspace-symbol
  :display-transformer-fn #'lsp-ivy--format-symbol-match)

abo-abo added a commit that referenced this issue Oct 13, 2019
@abo-abo
Copy link
Owner

abo-abo commented Oct 13, 2019

@sebastiansturm Thanks, looks even simpler than my PR. You can also remove (candidates nil) :)

@sebastiansturm
Copy link

ah yes, thanks :)

@abo-abo
Copy link
Owner

abo-abo commented Oct 18, 2019

Can this issue be closed?

@abo-abo
Copy link
Owner

abo-abo commented Mar 3, 2020

Closing this as solved.

@abo-abo abo-abo closed this as completed Mar 3, 2020
astoff pushed a commit to astoff/swiper that referenced this issue Jan 1, 2021
astoff pushed a commit to astoff/swiper that referenced this issue Jan 1, 2021
It may set `ivy--all-candidates' to 0.

Set `ivy--old-re' to nil, so we get highlighting with
`ivy--highlight-default'.

Re abo-abo#2263
astoff pushed a commit to astoff/swiper that referenced this issue Jan 1, 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

No branches or pull requests

3 participants