Skip to content

LSP Document Content Sync#1887

Merged
tom-ludwig merged 24 commits intoCodeEditApp:mainfrom
thecoolwinter:lsp/document-content-sync
Oct 7, 2024
Merged

LSP Document Content Sync#1887
tom-ludwig merged 24 commits intoCodeEditApp:mainfrom
thecoolwinter:lsp/document-content-sync

Conversation

@thecoolwinter
Copy link
Collaborator

@thecoolwinter thecoolwinter commented Sep 17, 2024 •
edited
Loading

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
tom-ludwig
tom-ludwig requested changes Oct 7, 2024
View reviewed changes
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

thecoolwinter commented Oct 7, 2024

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
@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

Reviewers

@austincondiff austincondiff austincondiff approved these changes

@tom-ludwig tom-ludwig tom-ludwig approved these changes

@0xWDG 0xWDG Awaiting requested review from 0xWDG

@FastestMolasses FastestMolasses Awaiting requested review from FastestMolasses

@matthijseikelenboom matthijseikelenboom Awaiting requested review from matthijseikelenboom

@MarcoCarnevali MarcoCarnevali Awaiting requested review from MarcoCarnevali

@Wouter01 Wouter01 Awaiting requested review from Wouter01

@KaiTheRedNinja KaiTheRedNinja Awaiting requested review from KaiTheRedNinja

@bombardier200 bombardier200 Awaiting requested review from bombardier200

Assignees

No one assigned

Labels

language server Issues or Pull Requests related to language servers.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@thecoolwinter @austincondiff @tom-ludwig