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 #2228

Conversation

daytonellwanger
Copy link

This PR adds Live Share support to LaTeX Workshop. General info on Live Share here. Extensibility API here.

Previously, certain features such as hover, completion, and snippets were supported by virtue of Live Share's remoting of VS Code primitives. Others were functional because they were able to run locally, such as the Snippet Panel. However, many critical features of LaTeX Workshop require custom Live Share integration to support.

Most importantly this PR downloads the generated PDFs on the host's machine to the guest's machine, which makes the various vieiwing functionality supported (with the exception of SyncTeX, which will require also downloading the SyncTeX files).

Screenshots throughout will have the host on the left and the guest on the right

sharePdf

This PR also remotes those commands (build, clean, ..) that need to be run on the host's machine.

remoteCommands

In order to be able to see the output of these commands and others, this PR also forwards the logs and the compiler logs to the guest. Lines from the host machine are prefixed with [Remote].

remoteLogs

There are a few places in this PR where I update the assumption that the .tex file exists on disc, which is not the case for the Live Share guest which uses a virtualized file system provider. E.g. these updates make structure shortcuts functional for the guest.

structure

Note that the guest need not have LaTeX installed for any of this to work.

Of course all the regular Live Share features (coediting, independent file navigation, language features, shared terminals, ...) work as usual.

liveshareFeatures

With this, I hope the majority of LaTeX Workshop's features now work for a Live Share guest. Surely there will be bugs and more features to add, but I think what is in this PR gives a super valuable offering.

@tamuratak
Copy link
Contributor

Thank you for your contribution.

Please convert the newline code fromCRLF to LF in src/components/liveshare.ts.

I am sorry that I am against merging this PR because it makes the maintenance of the extension much difficult as we can see code fragments for Live Share in multiple files. I think the architecture of LiveShare does not allow the maintenance to be easy.

@jlelong What do you think?

@James-Yu
Copy link
Owner

Just my 2c, I'm quite positive to this PR as it is a long requested and popular feature. The code seems to split in many places, yet mostly in a new liveshare.ts file. Will look deeper into the code and see if it is possible to further aggregate but in all I'd say I'm good with the current form.

@daytonellwanger
Copy link
Author

Thanks for the feedback @tamuratak and @James-Yu . I'm definitely agreed that this introduces coupling between Live Share and other components in the extension that would generally be considered bad design. But it seemed like this was the pattern followed by other components in the extension - virtually every one receives an instance of Extension in its constructor, which couples it to all other components.

I'd be happy to refactor this PR, as well as other parts of the extension, to decouple the Live Share component from the others, but it'll increase the size and risk of the change.

I think the following would accomplish this:

  • For forwarding logs from host to guest, we can create a lower-level component that exposes an event to report messages. The logger component can subscribe to this event and place the messages in the output window. The Live Share component will also subscribe to this event and forward the messages to the guest.
  • For the places where I read the file from VS Code instead of the file system, we should actually do this everywhere (in general, you shouldn't assume the file exists on disk), instead of conditioning it just for a Live Share guest. This was just done to decrease the risk of the change.
  • Re-implement the file watcher within the Live Share component, rather than reusing the one in the manager component. (Duplicate code vs. decoupling? Actually I guess we should refactor the file watcher into its own component that both the manager component and the Live Share component can depend on).
  • Refactor the view logic to remove the assumption that the PDF already exists when calling the view commands. Then Live Share can register itself as a "PDF provider," or whatever abstraction we decide on.
  • I don't think anything needs to be done about wrapping certain commands to make them remotable when you're a Live Share guest.

I think the trade-off here is between a cleaner design and a larger/riskier change. Let me know which you prefer!

@James-Yu
Copy link
Owner

I'm good with the current implementation. However as I have been away from the table for quite a loooong time, @tamuratak and @jlelong have higher priority.

@tamuratak
Copy link
Contributor

I have found that we can debug the extension with Live Share as a host and a guest on the same machine. It could make maintenance much easy.

I am not sure this PR works well with multiple file projects. @jlelong is in charge of the function. I want to wait for the opinion.

The architecture of Live Share still makes me nervous about the maintenance cost of this PR.

@daytonellwanger
Copy link
Author

The architecture of Live Share still makes me nervous about the maintenance cost of this PR.

@tamuratak could you elaborate on your specific concerns? We could probably refactor the changes to ease/remove them.

@jlelong
Copy link
Collaborator

jlelong commented Aug 5, 2020

@tamuratak I was on vacation and disconnected. I will need a few more days to look at this PR in depth and investigate the possible issues.

@jlelong
Copy link
Collaborator

jlelong commented Aug 7, 2020

First, I would like to say that I am very enthusiastic about VSCode's Live Share feature. Yet, I share @tamuratak's concerns about the maintenance: fully supporting Live Share requires a lot of small changes/adaptations at many places, which makes maintenance and evolution more costly.

  1. The current PR breaks with subfiles at

    const file = utils.resolveFile([path.dirname(vscode.window.activeTextEditor.document.fileName)], result[1])

    This is because in
    export function resolveFile(dirs: string[], inputFile: string, suffix: string = '.tex'): string | undefined {
    if (inputFile.startsWith('/')) {
    dirs.unshift('')
    }
    for (const d of dirs) {
    let inputFilePath = path.resolve(d, inputFile)
    if (path.extname(inputFilePath) === '') {
    inputFilePath += suffix
    }
    if (!fs.existsSync(inputFilePath) && fs.existsSync(inputFilePath + suffix)) {
    inputFilePath += suffix
    }
    if (fs.existsSync(inputFilePath)) {
    return inputFilePath
    }
    }
    return undefined
    }

    the function fs.existsSync does not work with Live Share. This function is also used at many other places.

  2. This .fls file is not available on the guest. See the wiki for explanations on how we use this file for discovering tricky dependencies

  3. It seems that the file watchers are not working because they want to monitor files on disk. On the guest, the paths of the files inside the live share session look like absolute paths but are not and yet they are considered as such inside

    async findRoot(): Promise<string | undefined> {

  4. The getDirtyContent function

    private getDirtyContent(file: string, reload: boolean = false): string {
    for (const cachedFile of Object.keys(this.cachedContent)) {
    if (reload) {
    break
    }
    if (path.relative(cachedFile, file) !== '') {
    continue
    }
    return this.cachedContent[cachedFile].content
    }
    const fileContent = utils.stripComments(fs.readFileSync(file).toString(), '%')
    this.cachedContent[file] = {content: fileContent, element: {}, children: [], bibs: []}
    return fileContent
    }

    crashes because it tries to open a file which does not exist on the current file system (similar issue as 3 actually).

I am afraid that fixing all these issues will spread live share specific code at even more places and therefore increase the maintenance cost.

@tamuratak
Copy link
Contributor

This PR does not work now. LiveShare.shareService() always returns null. Related to microsoft/live-share/issues/2896. If we change the extension id and the publisher as in microsoft/live-share#2896 (comment), we can make it work well.

Error handling in this PR is incomplete. When errors occur, Logger.addLogMessage should be called with an appropriate error message.

@tamuratak
Copy link
Contributor

tamuratak commented Aug 8, 2020

I have found that, on the guest machine, we can read files on the host machine with vscode.workspace.fs.readFile. So, we do not have to copy PDF files from the host to the guest.

I think that it is a more appropriate approach making LaTeX Workshop use the API of VS Code FileSystem instead of using Node.js API directly. Then, the extension works seamlessly both on the guest and the host.

Other than Live Share, using the API of VS Code FileSystem seems promising to benefit from the ecosystem of VS Code.

@tamuratak
Copy link
Contributor

I have found that FileSystemProvider by the Live Share extension does not support binary files. I don't know why. Related to microsoft/live-share/issues/1895

@tamuratak
Copy link
Contributor

tamuratak commented Aug 9, 2020

The following is an approach that I think is appropriate.

Implement a kind of the fs module with vscode.workspace.fs and the fs module of Node.js.

We can see a prototype. With this approach, all we have to do is replace the fs module with our own fs instance.

The paths of files on the host machine are expressed as vsls://path/filename. We can read these files calling vscode.workspace.fs.readFile for the Uri on the guest machine. If we have to call fs.existsSync for the Uri, we implement fs.exists and call it.

With this approach, we can also make LaTeX Workshop work with other virtual file systems provided by other extensions, e.g., sshfs.

However, as mentioned above, this approach does not work with Live Share now because FileSystemProvider by the Live Share extension does not support binary files. Anyone interested in this feature, please upvote microsoft/live-share/issues/1895 and microsoft/live-share/issues/3791.

I don't know why it is restricted only for text files. VS Code FileSystemProvider is not intended only for text files.

@tamuratak
Copy link
Contributor

I opened an issue microsoft/live-share/issues/3791

@cmprmsd
Copy link

cmprmsd commented Feb 4, 2021

@daytonellwanger
Wow. I see, you already did a lot of implementation for this pull request.
This is exactly our main use case, that we want to use in our company to be able to work on our pentest reports together.

@tamuratak @jlelong
I tried to push the LiveShare support for binary files in microsoft/live-share#1895 but it seems there are some concerns allowing pdfs, pngs etc.

I'm really eager to see this combination of Live Share and Latex Reporting in this extension.

The main use cases, where binary support is needed, would be:

  • Getting generated PDFs to the participants
  • Pasting images from the clipboard of participants (sending them to the server and caching them locally)

I see your point, when you want to stay extension neutral and would like to support SSH FS, but I think Live Share offers a far better way for collaborative editing, as SSHFS does not support for file-change-notifications, which is a limitation by the underlying SFTProtocol.

While I see the benefits of the pure LiveShare-FS implementation on MS-side, I think the project would benefit from implementing this pull request as there is no progress sadly.

@daytonellwanger
Copy link
Author

Hey @cmprmsd, I'd love to add Live Share integration to LaTeX Workshop the right way. I haven't forgotten about this pull request 😊. Unfortunately there are higher priority items on the Live Share side to be working on at the moment. But I do intend to get back to this as soon as possible, and really want to see this thing completed.

cc @Davsterl

@septatrix
Copy link

Would this also allow the guests to initiate the compile process or is this limited to the host? If the former is the case this seems like a big security problem as a guest could insert arbitrary commands into the source code which would be executed during compilation

@cmprmsd
Copy link

cmprmsd commented Jun 28, 2021

Yup that's the case. It's based on trust who you work with. As this whole story never really got pushed forward I decided to look for alternatives and ended up with the self hosted version of Overleaf. It's great. I modified the server a bit in order to allow custom snippets and am happy with it :)

@tamuratak
Copy link
Contributor

Close in favor of the approach #2228 (comment).

@tamuratak tamuratak closed this Jun 28, 2021
@septatrix
Copy link

Yup that's the case. It's based on trust who you work with.

While you definitely should trust who you work with the underlying security issue is still relevant and the reason why terminals for example are still read-only by default. So regardless which approach gets implement this should be considered.

Close in favor of the approach #2228 (comment).

This seems like a good and clean solution. However based on the above security consideration I would propose to only give the host the ability to initiate a compilation and have the clients fetch the PDF (or generate them locally)

Repository owner locked as resolved and limited conversation to collaborators Jun 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants