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

Live share integration #4308

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Live share integration #4308

wants to merge 19 commits into from

Conversation

quoc-ho
Copy link
Contributor

@quoc-ho quoc-ho commented Jul 1, 2024

Continue the discussion #4305 (reply in thread).

@hoang06kx1
Copy link

Awesome! Great contribution @quoc-ho . Let's wait for review.

@quoc-ho
Copy link
Contributor Author

quoc-ho commented Jul 8, 2024

The following functionalities have been implemented on guests (on the host, everything is the same as before):

  • View pdf by clicking on the pdf file
  • View pdf by the view command*
  • Auto refresh pdf view when the pdf file changes
  • Inverse and forward* synctex

Here, * means doesn't work when the guest is on a Windows computer. This is because on Windows, getPdfPath always returns smth with c:\ at the beginning, which does not make sense. For guests, the input will be something like vsls:/file.tex regardless of the system. I only have limited access to a Windows computer so at least for now, I cannot really troubleshoot this. For now, Windows users can double click on the pdf file to open it instead. Forward synctex is still a problem.

Build command cannot be triggered from the guest yet (but it's not hard to implement). For now, the host can just turn on build on save.


Notes on the implementation

  1. On the host, everything should function exactly the same as before.
  2. There are two minor but very frequent changes involving URI schemes:
    a. Previously, vscode.Uri.file was used, which has file scheme and which doesn't work on the guest side since the appropriate scheme there should be vsls. I implemented fileUriFromPath in file.ts which handles schemes more appropriately.
    b. Previously, there were many checks .scheme === 'file'. I implemented isAcceptedScheme which allows vsls as well and replaced the checks by using this function.
  3. On the guest, the HTTP server now switches to proxy mode during a live share session which pipes the requests to the host via the shared port (the share is facilitated by the live share extension). The guest also opens a WebSocket connection to the host WebSocket server and uses this to handle refresh and synctex.
  4. When starting a new session, the host will automatically share its port via the live share extension and the guest will automatically obtain that port when joining. Since live share doesn't expose an API to acquire the port (on the guest), it's done via triggering the appropriate command and accessing the clipboard. If the initial sharing fails for some reason (for example, when the host doesn't click allow for the share request), the user can initiate share (on the host) and acquire port (on the guest) via two new commands.

@James-Yu
Copy link
Owner

I'm back. On it in the next week(s).

Copy link
Owner

@James-Yu James-Yu left a comment

Choose a reason for hiding this comment

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

I did not check carefully (next step will do so), yet it seems that currently synctex from guest to host then back to guest is adding a lot of lines to the code, which is making the code quite convoluted.

I would recommend removing all host-guest communications related to synctex. For guest, rely on its local optionally-installed synctex and fallback to the bundled synctexjs implementation. This will make the code much more maintainable.

src/completion/latex.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/core/commands.ts Outdated Show resolved Hide resolved
src/extras/liveshare.ts Outdated Show resolved Hide resolved
src/locate/synctex.ts Outdated Show resolved Hide resolved
src/preview/host-connection.ts Outdated Show resolved Hide resolved
src/preview/server.ts Outdated Show resolved Hide resolved
src/preview/viewer.ts Show resolved Hide resolved
src/preview/viewer.ts Outdated Show resolved Hide resolved
@quoc-ho
Copy link
Contributor Author

quoc-ho commented Jul 31, 2024

Thanks for looking into this. I think the changes are quite minimal. The large changes in synctex.ts are just refactoring. I didn't use the local synctex or synctex.js because the tar.gz file is also unavailable on the guest side due to binary file limitation imposed by LiveShare.

I'm traveling for quite a while so I will be available only for comments if you have questions but probably not actually writing code.

@James-Yu
Copy link
Owner

I've done some refactoring to follow minimal intrusion. May you have a check if the new code is equivalent when available?

Further, as there are some limited functionalities on Windows, it would be good to have sort of notices when related functions are invoked on win.

@quoc-ho
Copy link
Contributor Author

quoc-ho commented Aug 10, 2024

Great! I'll go over the refactor by Tuesday.

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