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

LSP Document Content Sync #1887

Merged
merged 24 commits into from
Oct 7, 2024

Conversation

thecoolwinter
Copy link
Collaborator

@thecoolwinter thecoolwinter commented Sep 17, 2024

Description

Note

Requires #1879 to be merged, based on that branch. This will look like it's a massive change until that is merged.
Requires CodeEditApp/CodeEditSourceEditor#265 to be merged as it provides the required APIs to make this work.

Changes:

  • Implements Language Server document content syncing.
  • Adds a LSPContentCoordinator which manages forwarding content change notifications to the correct language server.
  • The language server has been updated to send the correct document sync notifications depending on what the language server supports.

Misc related changes:

  • Fixed bugs with the cursor position label, uses the tab change subject instead of a variety of notification sources. Also split it into two views for ease in maintenance.

Related Issues

  • N/A

Checklist

  • I read and understood the contributing guide as well as the code of conduct
  • The issues this PR addresses are related to each other
  • My changes generate no new warnings
  • My code builds and runs on my machine
  • My changes are all related to the related issue above
  • I documented my code

Screenshots

N/A

@thecoolwinter thecoolwinter marked this pull request as draft September 17, 2024 21:16
@thecoolwinter thecoolwinter added the language server Issues or Pull Requests related to language servers. label Sep 18, 2024
@thecoolwinter thecoolwinter marked this pull request as ready for review September 27, 2024 18:51
Copy link
Member

@tom-ludwig tom-ludwig left a comment

Choose a reason for hiding this comment

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

This is almost good to go. Only one thing needs to be changed.

Also while these changes are great, some of them seem unrelated to the main focus and might be better in a separate PR. I get that it's faster to put everything in one, but it makes reviewing and testing a bit harder (and I need to work on this too 😅). Thanks!

@thecoolwinter
Copy link
Collaborator Author

This is almost good to go. Only one thing needs to be changed.

Also while these changes are great, some of them seem unrelated to the main focus and might be better in a separate PR. I get that it's faster to put everything in one, but it makes reviewing and testing a bit harder (and I need to work on this too 😅). Thanks!

Yeah this one was particularly tangled with a few things since I did #1892 on this branch and had to split it off to make it its own PR. I left a few things here that's my bad.

@tom-ludwig tom-ludwig merged commit f705595 into CodeEditApp:main Oct 7, 2024
2 checks passed
@thecoolwinter thecoolwinter deleted the lsp/document-content-sync branch October 7, 2024 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
language server Issues or Pull Requests related to language servers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants