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

ide/lsp.client fonts-colors #4057

Merged
merged 1 commit into from May 10, 2022

Conversation

vieiro
Copy link
Contributor

@vieiro vieiro commented Apr 30, 2022

ide/lsp.client threading and fonts-colors improvements

  • lsp.client module acknoledge whatever font-colors have been registered for a given mimme-type.
  • As a performance plus, the lookup of FontsAndColors is moved out of the loop over the tokens.getData() list.

Let the lsp.client module acknoledge whatever font-colors have been registered for a given mimme-type.
As a performance plus, the lookup of FontsAndColors is moved out of the loop over the tokens.getData() list.
@vieiro vieiro mentioned this pull request May 1, 2022
@vieiro
Copy link
Contributor Author

vieiro commented May 1, 2022

Hi @matthiasblaesing , mind taking a look at this? These are the commits for ide/lsp.client extracted from from #3978.

Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idea looks sane to me, but I left a few inline comments, where the threading issue is my main headache.

@vieiro
Copy link
Contributor Author

vieiro commented May 2, 2022

Thanks for the throrough review, @matthiasblaesing!
You're right that getBindings may indeed block, so we need to schedule these calls to getBindings in a common worker, but keep on using LSPBindings server-specific RequestProcessor (the bindings.worker). This getBindings looks like a good candidate to return a CompletableFuture, I think.
Let me give a second though during the next days.

@vieiro vieiro force-pushed the feature/lsp-coloring-threading branch from af70600 to e647625 Compare May 9, 2022 17:15
@vieiro vieiro changed the title ide/lsp.client fonts-colors and threading. ide/lsp.client fonts-colors May 9, 2022
Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok - you reduced the scope of this slightly ;-)

Looked sane already on the first pass.

@vieiro
Copy link
Contributor Author

vieiro commented May 9, 2022

@matthiasblaesing yes, rearranging the threading is going to take longer than expected, worth a full PR in the future. Let's move along with cnd.

@vieiro vieiro merged commit 793a135 into apache:master May 10, 2022
@neilcsmith-net neilcsmith-net added this to the NB15 milestone Jul 11, 2022
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 this pull request may close these issues.

None yet

3 participants